djberg96 / sys-proctable

A cross-platform Ruby interface for gathering process information on your operating system
Apache License 2.0
150 stars 33 forks source link

[WIP] Memory improvements for Darwin #65

Closed NickLaMuro closed 6 years ago

NickLaMuro commented 6 years ago

This is a collection of proposed changes to decrease the memory usage of Sys::ProcTable.ps on Darwin (with the intent to do the same on other OS's in the future).

Changes

  1. Creates the Sys::ProcTable::PROC_STRUCT_FIELD_MAP. This takes the field calculation mapping work that was done in each invocation of .ps, and memoizes on class load to be a simple lookup instead. When .ps is called without a pid, this has substantial memory savings. In the number of objects allocated.
  2. Simplifies .ps to call different methods based on whether a pid is passed in. On the single pid case, this reduces the need to look up all of the pids and iterate through each one of the active processes on the machine.
    • In doing this, portions of .ps were split up into helper methods that could be called on their own on a per pid basis.
  3. Added Sys::ProcTable.rss and Sys::ProcTable.vsize convenience methods for grabbing each of those values for a single process with less ruby Object overhead.
  4. Adds :lazy option to .ps, which will take the .exe, .cmdline, and .environ values, and make them lazy loadable. When this option is present, this means that the values will not be fetched and calculated until the first invocation of the methods (doesn't work with the [] form unfortunately).

These were developed together, and split un into logical commits at the end, so opening this as a single PR was only out of convenience on my end. These changes could very well be their own individual pull requests if desired.

The last one (lazy loading) in particular is sort of "meh" in my book. It saves some memory when that data is not needed, but at the same time, it adds some instability to the overall usage since the methods could get called significantly after the first call to .ps, so the chances of the process being terminated before the data can be requested is definitely higher, and might cause confusion to the end user. I am open to finding a different way of accomplishing the same savings, or dropping the change all together.

Benchmarks

TODO: Get some actual before and after metrics here...

While I don't have any data put together at the time of opening this pull request, the general way I was testing my changes was by doing the following in a irb session:

irb> require 'sys-proctable'
irb> pid = Process.pid
irb> GC.stat
#=> { :total_allocated_objects => 123, ... }
irb> Sys::ProcTable.ps(pid)
irb> GC.stat
#=> { :total_allocated_objects => 234, ... }
irb> 234 - 123

I was manually subtracting the values from GC.stat[:total_objects_allocated] in a separate line (as shown) after the fact just to get the smallest number of object allocations possible when fetching the data (probably overkill... but ¯\(°_o)/¯ ). There will always be some object overhead by doing GC.stat by itself, so that is something to be aware of.

For some rough numbers though:

Change Type Memory change
Without changes: ~2k objects created per .ps(pid) call
With changes, not lazy: ~1.65k objects created per .ps(pid) call
With changes, lazy: ~1.2k objects created per .ps(pid) call
.rss(pid) call: ~650 objects created per call
NickLaMuro commented 6 years ago

@djberg96 I would call this more [WIP] than ready to merge at the moment, but wanted to get something up to see what you thought about some of the changes.

cc / @kbrock

NickLaMuro commented 6 years ago

Looks like I have some broken tests... oops.

Will mark this as [WIP] just to make sure we don't merge while I get that worked out.

djberg96 commented 6 years ago

Ok, I see where you're going, but let's split this into separate PR's. One for refactoring a single pid. One for the lazy bits. One for the new singleton methods (I want to think about this). And one for reducing symbol creation.

djberg96 commented 6 years ago

@NickLaMuro Any chance up updating this?

NickLaMuro commented 6 years ago

😮

@djberg96 OH! I forgot about this one. I will see if I can take a look at rebasing this one sometime shortly... sorry!

NickLaMuro commented 6 years ago

Looking at this more, I think there is just one part of this that I might need to extract to get the improvements we want. Will look into doing that.

djberg96 commented 6 years ago

@NickLaMuro I pushed up a change that partially incorporates some of the changes you made here. I'm going to close this issue and you (or I) can add the other changes later in a separate PR.