Measure USS on OSX #745

Merged
merged 2 commits into from Feb 3, 2016

Conversation

Projects
None yet
2 participants
@EricRahm
Contributor

EricRahm commented Jan 27, 2016

This adds a USS (unique set size) measurement to memory_info_ex on OSX. It is based on the implementation in Firefox's memory measurement subsystem.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo Jan 28, 2016

Owner

I would be interested in seeing the impact on performances. Could you please measure it before and after the patch and paste the result?

python -m timeit -s "import psutil; p = psutil.Process()" "p.memory_info_ex()"

...also, while we're at it, is it possible to provide also PSS stats?

Owner

giampaolo commented Jan 28, 2016

I would be interested in seeing the impact on performances. Could you please measure it before and after the patch and paste the result?

python -m timeit -s "import psutil; p = psutil.Process()" "p.memory_info_ex()"

...also, while we're at it, is it possible to provide also PSS stats?

@EricRahm

This comment has been minimized.

Show comment
Hide comment
@EricRahm

EricRahm Jan 28, 2016

Contributor

before (master):

python -m timeit -s "import psutil; p = psutil.Process()" "p.memory_info_ex()"
100000 loops, best of 3: 2.17 usec per loop

after:

python -m timeit -s "import psutil; p = psutil.Process()" "p.memory_info_ex()"
10000 loops, best of 3: 113 usec per loop

So slower, but not as slow as on Linux.

Contributor

EricRahm commented Jan 28, 2016

before (master):

python -m timeit -s "import psutil; p = psutil.Process()" "p.memory_info_ex()"
100000 loops, best of 3: 2.17 usec per loop

after:

python -m timeit -s "import psutil; p = psutil.Process()" "p.memory_info_ex()"
10000 loops, best of 3: 113 usec per loop

So slower, but not as slow as on Linux.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo Jan 28, 2016

Owner

Any idea how to determine PSS?

Owner

giampaolo commented Jan 28, 2016

Any idea how to determine PSS?

@EricRahm

This comment has been minimized.

Show comment
Hide comment
@EricRahm

EricRahm Jan 28, 2016

Contributor

I do not know of a method for calculating PSS on OSX.

Contributor

EricRahm commented Jan 28, 2016

I do not know of a method for calculating PSS on OSX.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo Feb 3, 2016

Owner

Sorry for replying late. I was looking back at this and I wonder how this relates to Proces.memory_maps(). Currently memory_maps() implementation on OSX provides a "private" field. As such we should already be able to determine USS by doing:

>>> sum([x.private for x in psutil.Process().memory_maps()])

...but on the other hand the way you are determining USS (I'm talking about the implementation) is completely different. What's the difference between sum([x.private for x in psutil.Process().memory_maps()]) and psutil.Process().memory_info_ex().uss results? Are they equal?

Owner

giampaolo commented Feb 3, 2016

Sorry for replying late. I was looking back at this and I wonder how this relates to Proces.memory_maps(). Currently memory_maps() implementation on OSX provides a "private" field. As such we should already be able to determine USS by doing:

>>> sum([x.private for x in psutil.Process().memory_maps()])

...but on the other hand the way you are determining USS (I'm talking about the implementation) is completely different. What's the difference between sum([x.private for x in psutil.Process().memory_maps()]) and psutil.Process().memory_info_ex().uss results? Are they equal?

psutil/_psutil_osx_uss.c
+
+ *aN = privatePages * pageSize;
+ return true;
+}

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner
  • indentation should be 4 spaces
  • I don't like this being in a separate file, you can use `psutil/arch/osx/process_info.c
  • use // for all comments (instead of /*)
@giampaolo

giampaolo Feb 3, 2016

Owner
  • indentation should be 4 spaces
  • I don't like this being in a separate file, you can use `psutil/arch/osx/process_info.c
  • use // for all comments (instead of /*)
psutil/_psutil_osx_uss.c
+#include "_psutil_osx_uss.h"
+
+bool
+InSharedRegion(mach_vm_address_t aAddr, cpu_type_t aType)

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

please rename this to psutil_in_shared_region; also add a comment "docstring" explaining what this function does and do not use camelCase for variable name but var_name instead.

@giampaolo

giampaolo Feb 3, 2016

Owner

please rename this to psutil_in_shared_region; also add a comment "docstring" explaining what this function does and do not use camelCase for variable name but var_name instead.

psutil/_psutil_osx_uss.c
+ cpu_type_t cpu_type;
+ size_t len = sizeof(cpu_type);
+ if (sysctlbyname("sysctl.proc_cputype", &cpu_type, &len, NULL, 0) != 0) {
+ return false;

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

Looks like you want to PyErr_SetFromErrno(PyExc_OSError); here. The calling function should then return NULL so that you'll get an exception at Python level.

@giampaolo

giampaolo Feb 3, 2016

Owner

Looks like you want to PyErr_SetFromErrno(PyExc_OSError); here. The calling function should then return NULL so that you'll get an exception at Python level.

psutil/_psutil_osx_uss.c
+ if (kr == KERN_INVALID_ADDRESS) {
+ /* Done iterating VM regions. */
+ break;
+ } else if (kr != KERN_SUCCESS) {

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

this should be indented as:

} 
else if (kr != KERN_SUCCESS) {
@giampaolo

giampaolo Feb 3, 2016

Owner

this should be indented as:

} 
else if (kr != KERN_SUCCESS) {
psutil/_psutil_osx_uss.c
+}
+
+bool
+calc_uss(mach_port_t target, int64_t* aN)

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

On a second thought I would prefer you define this in psutil/_psutil_osx.c as proc_memory_uss.
From the python file (psutil/_psosx.py) after you call cext.psutil_proc_memory_info you will call cext.proc_memory_uss to get USS stats separately.

@giampaolo

giampaolo Feb 3, 2016

Owner

On a second thought I would prefer you define this in psutil/_psutil_osx.c as proc_memory_uss.
From the python file (psutil/_psosx.py) after you call cext.psutil_proc_memory_info you will call cext.proc_memory_uss to get USS stats separately.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@EricRahm

This comment has been minimized.

Show comment
Hide comment
@EricRahm

EricRahm Feb 3, 2016

Contributor

What's the difference between sum([x.private for x in psutil.Process().memory_maps()]) and psutil.Process().memory_info_ex().uss results? Are they equal?

They are not equal, memory_maps seems very low:

>>>print p.memory_info_ex(); print sum([x.private for x in psutil.Process().memory_maps()])
pextmem(rss=7880704L, vms=2534256640L, pfaults=9531392, pageins=1564672, uss=5849088L)
622592

The primary difference I can tell is memory_maps uses vm_region_recurse_64 and skips all submaps.

I found this:
https://gitlab.phusion.nl/passenger/temp-passenger/blob/99bf9b590e2a5c3620d5fc34f540b749452448ca/ext/common/Utils/ProcessMetricsCollector.h#L381
...and here the same operation is referenced as "PSS" not "USS". Weird...

They seem to be calculating some sort of USS/PSS hybrid.

Contributor

EricRahm commented Feb 3, 2016

What's the difference between sum([x.private for x in psutil.Process().memory_maps()]) and psutil.Process().memory_info_ex().uss results? Are they equal?

They are not equal, memory_maps seems very low:

>>>print p.memory_info_ex(); print sum([x.private for x in psutil.Process().memory_maps()])
pextmem(rss=7880704L, vms=2534256640L, pfaults=9531392, pageins=1564672, uss=5849088L)
622592

The primary difference I can tell is memory_maps uses vm_region_recurse_64 and skips all submaps.

I found this:
https://gitlab.phusion.nl/passenger/temp-passenger/blob/99bf9b590e2a5c3620d5fc34f540b749452448ca/ext/common/Utils/ProcessMetricsCollector.h#L381
...and here the same operation is referenced as "PSS" not "USS". Weird...

They seem to be calculating some sort of USS/PSS hybrid.

@EricRahm

This comment has been minimized.

Show comment
Hide comment
@EricRahm

EricRahm Feb 3, 2016

Contributor

@giampaolo Thanks for the review, I'll update per your style requests and merge into the main file.

Contributor

EricRahm commented Feb 3, 2016

@giampaolo Thanks for the review, I'll update per your style requests and merge into the main file.

@EricRahm

This comment has been minimized.

Show comment
Hide comment
@EricRahm

EricRahm Feb 3, 2016

Contributor

@giampaolo Updated per requests, I think I covered everything:

  • 4 spaces, foo_bar naming, // for comments
  • Moved impl into _psutil_osx.c, renamed psutil_proc_memory_uss, exported in cext
  • Added function level docs
  • Use cext.proc_memory_uss to add uss to memory_info_ex result

If all is good I'd recommend squashing before merging if that's not in your standard workflow.

Contributor

EricRahm commented Feb 3, 2016

@giampaolo Updated per requests, I think I covered everything:

  • 4 spaces, foo_bar naming, // for comments
  • Moved impl into _psutil_osx.c, renamed psutil_proc_memory_uss, exported in cext
  • Added function level docs
  • Use cext.proc_memory_uss to add uss to memory_info_ex result

If all is good I'd recommend squashing before merging if that's not in your standard workflow.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo Feb 3, 2016

Owner

What do you mean by that?
On Feb 3, 2016 9:19 PM, "Eric Rahm" notifications@github.com wrote:

@giampaolo https://github.com/giampaolo Updated per requests, I think I
covered everything:

  • 4 spaces, foo_bar naming, // for comments
  • Moved impl into _psutil_osx.c, renamed psutil_proc_memory_uss,
    exported in cext
  • Added function level docs
  • Use cext.proc_memory_uss to add uss to memory_info_ex result

If all is good I'd recommend squashing before merging if that's not in
your standard workflow.


Reply to this email directly or view it on GitHub
#745 (comment).

Owner

giampaolo commented Feb 3, 2016

What do you mean by that?
On Feb 3, 2016 9:19 PM, "Eric Rahm" notifications@github.com wrote:

@giampaolo https://github.com/giampaolo Updated per requests, I think I
covered everything:

  • 4 spaces, foo_bar naming, // for comments
  • Moved impl into _psutil_osx.c, renamed psutil_proc_memory_uss,
    exported in cext
  • Added function level docs
  • Use cext.proc_memory_uss to add uss to memory_info_ex result

If all is good I'd recommend squashing before merging if that's not in
your standard workflow.


Reply to this email directly or view it on GitHub
#745 (comment).

@EricRahm

This comment has been minimized.

Show comment
Hide comment
@EricRahm

EricRahm Feb 3, 2016

Contributor

@giampaolo I pushed an update that covered your code review changes, just wanted to list out what I changed and make sure you noticed.

Contributor

EricRahm commented Feb 3, 2016

@giampaolo I pushed an update that covered your code review changes, just wanted to list out what I changed and make sure you noticed.

giampaolo added a commit that referenced this pull request Feb 3, 2016

@giampaolo giampaolo merged commit fe2c677 into giampaolo:master Feb 3, 2016

3 of 4 checks passed

coverage/coveralls Coverage decreased (-1.4%) to 88.418%
Details
Scrutinizer No new issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo Feb 3, 2016

Owner

Merged. Thanks a lot for this (I'll also update CREDITS).

Owner

giampaolo commented Feb 3, 2016

Merged. Thanks a lot for this (I'll also update CREDITS).

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo Feb 3, 2016

Owner

The primary difference I can tell is memory_maps uses vm_region_recurse_64 and skips all submaps.

This made me think of #289.
If you understand the issue maybe you could fix memory_maps so that it takes submaps into account?

Owner

giampaolo commented Feb 3, 2016

The primary difference I can tell is memory_maps uses vm_region_recurse_64 and skips all submaps.

This made me think of #289.
If you understand the issue maybe you could fix memory_maps so that it takes submaps into account?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment