Populate PrivateMemorySize64 on macOS#131244
Conversation
Process.PrivateMemorySize64 always returned 0 on macOS because proc_taskallinfo (via proc_pidinfo/PROC_PIDTASKALLINFO) only exposes virtual and resident memory size; it has no private memory field. Populate it instead from proc_pid_rusage(RUSAGE_INFO_V4).ri_phys_footprint, the same value Activity Monitor's Memory column reports. Verified this call succeeds cross-process for processes owned by the current user (including sandboxed apps) without requiring task_for_pid()/root, and fails cleanly with EPERM for processes owned by other users, matching the existing permission behavior of proc_pidinfo already used in this file. Fix dotnet#105665
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
There was a problem hiding this comment.
Pull request overview
This PR updates the macOS implementation of System.Diagnostics.Process to populate ProcessInfo.PrivateBytes (and thus Process.PrivateMemorySize64) using proc_pid_rusage’s physical footprint value, and updates tests to assert the value is now non-zero on macOS.
Changes:
- Add a new macOS libproc interop helper to retrieve physical footprint via
proc_pid_rusage. - Populate
ProcessInfo.PrivateBytesinProcessManager.OSX.CreateProcessInfo. - Update
ProcessTeststo assertPrivateMemorySize64/PrivateMemorySizeare now > 0 on macOS.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Adds the new interop source to CoreLib build items on OSX/MacCatalyst. |
| src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs | Updates assertions for private memory size to be non-zero (including on macOS). |
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.OSX.cs | Sets ProcessInfo.PrivateBytes from the new physical-footprint interop call. |
| src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj | Includes the new interop file for the Process library build on OSX/MacCatalyst. |
| src/libraries/Common/src/Interop/OSX/Interop.libproc.GetProcessPhysicalFootprint.cs | Introduces new proc_pid_rusage/rusage_info_v4 interop and a helper to get ri_phys_footprint. |
Address review feedback: Interop.libproc.cs already declared rusage_info_v3 (with ri_phys_footprint) and a proc_pid_rusage P/Invoke. The previous commit introduced a separate rusage_info_v4 struct and a second proc_pid_rusage binding for the same native symbol, risking a struct layout/size mismatch between the two. v4 only appends fields after ri_serviced_system_time; the field this fix needs, ri_phys_footprint, is already at the same offset in v3. Remove the v4 struct and duplicate binding entirely and add TryGetProcessPhysicalFootprint, a small non-throwing wrapper around the existing proc_pid_rusage(V3) call, matching the existing GetProcessInfoById's null/false-on-failure convention rather than the existing proc_pid_rusage(int) overload's throwing one.
| // PrivateMemorySize is unchecked((int)PrivateMemorySize64) and can be negative on | ||
| // high-memory processes - assert the cast relationship rather than a value range, | ||
| // matching TestVirtualMemorySize. |
There was a problem hiding this comment.
| // PrivateMemorySize is unchecked((int)PrivateMemorySize64) and can be negative on | |
| // high-memory processes - assert the cast relationship rather than a value range, | |
| // matching TestVirtualMemorySize. |
Unnecessary comment - match style of the rest of the file
…eMemorySize64 casting
adamsitnik
left a comment
There was a problem hiding this comment.
Overall it LGTM, but the Windows vs Linux vs macOS differences need to be documented in XML docs, preferably using <remarks> section.
Thank you for your contribution @nwoolls !
| procInfo.SessionId = sessionId; | ||
| } | ||
|
|
||
| // Get the process's physical memory footprint (private/unique memory usage). This can fail |
| CreateDefaultProcess(); | ||
|
|
||
| AssertNonZeroAllZeroDarwin(_process.PrivateMemorySize64); | ||
| Assert.InRange(_process.PrivateMemorySize64, 1, long.MaxValue); |
There was a problem hiding this comment.
The existing test just checks whether the returned value is not zero:
Ensuring that it's not negative value is an improvement 👍
Another improvement we could make would be merging the TestPrivateMemorySize64 and TestPrivateMemorySize test into one, so our test suite could spawn one less process (and run slightly faster). Not a blocker, just something to consider.
…morySize with platform-specific remarks
…rySize and consolidate assertions for PrivateMemorySize64
…surement and update comments for consistency
…ement details on Linux and macOS
Process.PrivateMemorySize64 always returned 0 on macOS because proc_taskallinfo (via proc_pidinfo/PROC_PIDTASKALLINFO) only exposes virtual and resident memory size; it has no private memory field.
Populate it instead from proc_pid_rusage(RUSAGE_INFO_V3).ri_phys_footprint, the same value Activity Monitor's Memory column reports. Verified this call succeeds cross-process for processes owned by the current user (including sandboxed apps) without requiring task_for_pid()/root, and fails cleanly with EPERM for processes owned by other users, matching the existing permission behavior of proc_pidinfo already used in this file.
Fix #105665