Measure USS on Windows #746

Merged
merged 5 commits into from Feb 4, 2016

Conversation

Projects
None yet
3 participants
@EricRahm
Contributor

EricRahm commented Jan 28, 2016

This adds a USS (unique set size, aka private working set) measurement to memory_info_ex on Windows.

Performance before patch:

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

Performance after patch:

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

So slower, but not as bad as Linux and OSX.

@EricRahm

This comment has been minimized.

Show comment
Hide comment
Contributor

EricRahm commented Jan 28, 2016

@EricRahm

This comment has been minimized.

Show comment
Hide comment
@EricRahm

EricRahm Jan 29, 2016

Contributor

It would appear AppVeyor is unhappy about the usage of stdint.h on Windows, I'm guessing Visual C++ for Python 9 does not include it, but it was in my path due to having a full-fledged modern msvc install.

Contributor

EricRahm commented Jan 29, 2016

It would appear AppVeyor is unhappy about the usage of stdint.h on Windows, I'm guessing Visual C++ for Python 9 does not include it, but it was in my path due to having a full-fledged modern msvc install.

@fbenkstein

This comment has been minimized.

Show comment
Hide comment
@fbenkstein

fbenkstein Jan 29, 2016

Collaborator

Python 2.7 still has to be compiled with Visual Studio 2008 which does not support stdint.h.

Collaborator

fbenkstein commented Jan 29, 2016

Python 2.7 still has to be compiled with Visual Studio 2008 which does not support stdint.h.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo Feb 3, 2016

Owner

Related: #752

Owner

giampaolo commented Feb 3, 2016

Related: #752

psutil/_psutil_windows_uss.c
+ }
+
+ return result;
+}

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

Indentation should be 4 spaces. Also, there's no need to put this in a separate file. You can use psutil/arch/windows/process_info.c instead.

@giampaolo

giampaolo Feb 3, 2016

Owner

Indentation should be 4 spaces. Also, there's no need to put this in a separate file. You can use psutil/arch/windows/process_info.c instead.

psutil/_psutil_windows_uss.c
+ SYSTEM_INFO si;
+
+ proc = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ,
+ FALSE, target);

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

You should use psutil_handle_from_pid defined in psutil/arch/windows/process_info.c; that will take care of raising NoSuchProcess and AccessDenied exception if needed.

@giampaolo

giampaolo Feb 3, 2016

Owner

You should use psutil_handle_from_pid defined in psutil/arch/windows/process_info.c; that will take care of raising NoSuchProcess and AccessDenied exception if needed.

psutil/_psutil_windows_uss.c
+
+ /* Determine how many entries we need. */
+ memset(&tmp, 0, tmpSize);
+ QueryWorkingSet(proc, &tmp, tmpSize);

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

You should check the return value; if 0 use PyErr_SetFromWindowsErr(0);

@giampaolo

giampaolo Feb 3, 2016

Owner

You should check the return value; if 0 use PyErr_SetFromWindowsErr(0);

psutil/_psutil_windows_uss.c
+ memset(&tmp, 0, tmpSize);
+ QueryWorkingSet(proc, &tmp, tmpSize);
+
+ /* Fudge the size in case new entries are added between calls. */

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

Please use // comments instead

@giampaolo

giampaolo Feb 3, 2016

Owner

Please use // comments instead

psutil/_psutil_windows_uss.c
+ }
+
+ infoArraySize = tmpSize + (entries * sizeof(PSAPI_WORKING_SET_BLOCK));
+ infoArray = (PSAPI_WORKING_SET_INFORMATION*)malloc(infoArraySize);

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

You should check malloc return value; if NULL then do PyErr_NoMemory(); and return NULL

@giampaolo

giampaolo Feb 3, 2016

Owner

You should check malloc return value; if NULL then do PyErr_NoMemory(); and return NULL

psutil/_psutil_windows_uss.c
+ }
+ }
+
+ GetSystemInfo(&si);

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

please add a comment saying that this function does not return any value; also I would rename si to system_info for clarity.

@giampaolo

giampaolo Feb 3, 2016

Owner

please add a comment saying that this function does not return any value; also I would rename si to system_info for clarity.

psutil/_psutil_windows_uss.c
+#include <psapi.h>
+
+BOOL
+calc_uss(DWORD target, unsigned long long* 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_windows.c as proc_memory_uss.
From the python file (psutil/_pswindows.py) after you call cext.psutil_proc_memory_info (or cext.psutil_proc_memory_info_2 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_windows.c as proc_memory_uss.
From the python file (psutil/_pswindows.py) after you call cext.psutil_proc_memory_info (or cext.psutil_proc_memory_info_2 you will call cext.proc_memory_uss to get USS stats separately.

EricRahm added some commits Feb 3, 2016

@EricRahm

This comment has been minimized.

Show comment
Hide comment
@EricRahm

EricRahm Feb 3, 2016

Contributor

@giampaolo The latest pushes should cover your code review comments.

Contributor

EricRahm commented Feb 3, 2016

@giampaolo The latest pushes should cover your code review comments.

psutil/_psutil_windows.c
+ DWORD info_array_size;
+ PSAPI_WORKING_SET_INFORMATION* info_array;
+ SYSTEM_INFO system_info;
+ PyObject* result = NULL;

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

please rename this to py_result

@giampaolo

giampaolo Feb 3, 2016

Owner

please rename this to py_result

psutil/_psutil_windows.c
+ HANDLE proc;
+ PSAPI_WORKING_SET_INFORMATION tmp;
+ DWORD tmp_size = sizeof(tmp);
+ size_t entries, private_pages, i;

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

please only one variable declaration per line

@giampaolo

giampaolo Feb 3, 2016

Owner

please only one variable declaration per line

psutil/_psutil_windows.c
+ memset(&tmp, 0, tmp_size);
+ if (!QueryWorkingSet(proc, &tmp, tmp_size)) {
+ // NB: QueryWorkingSet is expected to fail here due to the
+ // buffer being too small.

This comment has been minimized.

@giampaolo

giampaolo Feb 3, 2016

Owner

unnecessary spaces?

@giampaolo

giampaolo Feb 3, 2016

Owner

unnecessary spaces?

@EricRahm

This comment has been minimized.

Show comment
Hide comment
@EricRahm

EricRahm Feb 3, 2016

Contributor

@giampaolo ee6024f should address the final comments.

Contributor

EricRahm commented Feb 3, 2016

@giampaolo ee6024f should address the final comments.

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

@giampaolo giampaolo merged commit 75859a2 into giampaolo:master Feb 4, 2016

2 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
coverage/coveralls Coverage decreased (-1.4%) to 88.446%
Details
Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment