Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

12x faster implementation for Mac OS #32

Merged
merged 1 commit into from
Jul 8, 2019
Merged

Conversation

fcheung
Copy link
Contributor

@fcheung fcheung commented Jul 6, 2019

For context see #31.

This implements the approach mentioned here: https://stackoverflow.com/a/23379216/147390

Not sure about the platform selection logic - should using this codepath by compulsory for macs or should it fallback to the ps version ? (but then how would people ever know to install ffi)

This seems to produce the same results in my testing as ps, is it worth trying to enforce that in specs ?

Lastly, not sure what needs to go into .travis.yml for specs to run against the same set of rubies but on mac os.

@schneems
Copy link
Member

schneems commented Jul 8, 2019

Thanks a ton for this!

should using this codepath by compulsory for macs or should it fallback to the ps version

I'm thinking that we should make ffi a required dependency. While it does increase the foot print for linux-only use cases my gut says that most people who will have this gem in their Gemfile will also have another gem that requires it as well. In our case we're never loading it unless you're on a mac, so there's no runtime penalty, just the overhead of installing the extra dependency if it's not needed.

If it becomes an issue we could perhaps break out this gem into another micro gem that does not require FFI and does not have this optimization.

This seems to produce the same results in my testing as ps, is it worth trying to enforce that in specs ?

That is a concern for me, we could maybe introduce a method that returned both results and let people check their own systems.

I'm going to take your patch, make FFI required and try to get Travis working with OSX. Thanks again for the patch, you rock!

@schneems schneems merged commit 01d8580 into zombocom:master Jul 8, 2019
@schneems schneems changed the title Implement faster version for Mac OS 12x faster implementation for Mac OS Jul 8, 2019
@schneems schneems mentioned this pull request Jul 8, 2019
schneems added a commit that referenced this pull request Aug 25, 2020
Currently when you call `GetProcessMem.new` with a different pid value than the current process it calls `GetProcessMem::Darwin.resident_size this uses FFI to get data from the mach kernel about the current process.

- #32
- https://stackoverflow.com/questions/18389581/memory-used-by-a-process-under-mac-os-x/23379216#23379216

This PR adds a test to ensure that memory results from different processes is correctly reported.

This PR addresses the different PID problem by checking if a different pid other than Process.pid is being passed in. If that's the case then we will fall back to determining memory based off of shelling out to `ps`.

There was a performance concern over using `Process.pid` but it appears to be relatively fast. It is approximately the speed of allocating 3 string object.
@schneems
Copy link
Member

There's a bug reported where different PIDs don't return the correct information. I've got a workaround and a test in #41. It detects this scenario and then falls back. Do you have any ideas on how to pull task info from different processes from mach?

schneems added a commit that referenced this pull request Aug 25, 2020
Currently when you call `GetProcessMem.new` with a different pid value than the current process it calls `GetProcessMem::Darwin.resident_size this uses FFI to get data from the mach kernel about the current process.

- #32
- https://stackoverflow.com/questions/18389581/memory-used-by-a-process-under-mac-os-x/23379216#23379216

This PR adds a test to ensure that memory results from different processes is correctly reported.

This PR addresses the different PID problem by checking if a different pid other than Process.pid is being passed in. If that's the case then we will fall back to determining memory based off of shelling out to `ps`.

There was a performance concern over using `Process.pid` but it appears to be relatively fast. It is approximately the speed of allocating 3 string object.
@fcheung
Copy link
Contributor Author

fcheung commented Aug 25, 2020

Well there is task_for_pid() which does what the name suggests but apparently you need elevated privileges for that (http://os-tres.net/blog/2010/02/17/mac-os-x-and-task-for-pid-mach-call/) since you can use that for injecting code into a process and so on.

I suspect another approach is needed. One starting point might be the source code to ps (https://opensource.apple.com/source/adv_cmds/adv_cmds-168/ps/ps.c.auto.html) although that is installed setuid, so may not be helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants