fix virtual_memory computation on windows in refresh_pids

[vthib]

Following #1299, I started
using refresh_pids_specifics to avoid the macOS cpu usage computation bug.
However, this change brought another bug, where the virtual_memory field
of a process returns 2TB of ram when first refreshed.

When refreshing a process' memory usage, we use WorkingSetSize for
memory and PrivateUsage for virtual_memory.

However, when calling refresh_pids, another codepath refreshes those
values, and use VirtualBytes for virtual_memory. This is not the
right field and is generally equal to 2TB, making this measurement
completely wrong.

The fix is to use the same field as when refreshing a single process,
PrivatePageCount (which afaict reading the documentation, should be
the same thing as PrivateUsage).

I tested it on the 0.30 branch and it fixed the bug.

I targeted master as i'm ensure what your stable releases process is like,
but I would really appreciate a backport and a new 0.30 release with this
bugfix if possible. Thanks!

[GuillaumeGomez]

Private pages and virtual memory are not the same thing as far as I can see. I think this is related to this issue.

[vthib]

I am simply aligning this code with the one done when refresh the memory of a single process. This means calling refresh_pid or refresh_pids no longer leads to differences in values (with a 2TB value returned on the first call to refresh_pids which is clearly buggy).

There is a question of whether PrivateUsage should be returned in virtual_memory, but that's out of the scope of this PR yes

[GuillaumeGomez]

Ok I just figured out why I was confused. It seems like PROCESS_MEMORY_COUNTERS_EX doesn't have the virtual memory size information. So you're unifying things, but not fixing them in this case. It'd need to fix how refresh_pid is doing things to align it with the rest instead of making everything switched to PrivatePageCount (which is wrong).

[vthib]

Using VirtualSize is definitely not what should be done though, it's always equal to 2TB afaict.

This for example this for explorer.exe:

Virtual size is 2TB. Private bytes is 64MB, and working set is 330MB.

It is true that right now, the memory/virtual_memory values feel reversed on windows. In fact, in our code, we explicitly use virtual_memory on windows and memory on other systems to avoid this issue. In fact, the windows task manager even uses PrivateBytesWS as the "memory usage" measurement for a process.

So while it is true that a fix would be nice to fix those issues, it's not really related to mine. Ideally for me, the best solution would be to:

• Replace VirtualBytes with PrivatePageCount, as I did, on all stable branches. This ensures that calling refresh_pid and refresh_pids give the same value and cannot return a 2TB value. This also does not change any other behavior, so code that depended on memory or virtual_memory calls on Windows are not broken.
• Fix the memory computation for Windows on master, as it is a breaking change. The best would probably be PrivateBytesWS for memory and WorkingSet for virtual_memory? I suppose.

What do you think?

[GuillaumeGomez]

Unfortunately it doesn't seem to be as you describe it. PrivateMemory seems to be a subset of WorkingSetSize (which includes "shared" resources). The issue I linked above was a discussion about adding API in sysinfo to query this information (the private memory) across systems. The virtual memory is very abstract and unless you know what you're doing, generally it's not a good idea to use it.

So for the current situation, the correct fix would be to use the correct function in refresh_pid, not to replace it with PrivateMemory.

[GuillaumeGomez]

It was fixed in #1316.