This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Improve the performance of Environment.WorkingSet in Windows #26522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stephentoub
suggested changes
Sep 5, 2019
jkotas
reviewed
Sep 5, 2019
src/System.Private.CoreLib/src/System/Diagnostics/Eventing/RuntimeEventSourceHelper.Windows.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Sep 5, 2019
src/System.Private.CoreLib/shared/Interop/Windows/Kernel32/Interop.GetProcessMemoryInfo.cs
Outdated
Show resolved
Hide resolved
…timeEventSourceHelper
sywhang
changed the title
jkotas
reviewed
Sep 5, 2019
src/System.Private.CoreLib/shared/Interop/Windows/Kernel32/Interop.GetProcessMemoryInfo.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Sep 5, 2019
src/System.Private.CoreLib/shared/Interop/Windows/Kernel32/Interop.GetProcessMemoryInfo.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Sep 5, 2019
src/System.Private.CoreLib/shared/Interop/Windows/Kernel32/Interop.GetProcessMemoryInfo.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Sep 5, 2019
src/System.Private.CoreLib/shared/Interop/Windows/Kernel32/Interop.GetProcessMemoryInfo.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Sep 5, 2019
src/System.Private.CoreLib/shared/Interop/Windows/Kernel32/Interop.GetProcessMemoryInfo.cs
Outdated
Show resolved
Hide resolved
danmoseley
reviewed
Sep 5, 2019
src/System.Private.CoreLib/shared/System.Private.CoreLib.Shared.projitems
Show resolved
Hide resolved
noahfalk
reviewed
Sep 5, 2019
src/System.Private.CoreLib/src/System/Diagnostics/Eventing/RuntimeEventSource.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Sep 10, 2019
src/System.Private.CoreLib/shared/Interop/Windows/Kernel32/Interop.GetProcessMemoryInfo.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Sep 10, 2019
| get | ||
| { | ||
| Type? processType = Type.GetType("System.Diagnostics.Process, System.Diagnostics.Process, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", throwOnError: false); | ||
| if (processType?.GetMethod("GetCurrentProcess")?.Invoke(null, BindingFlags.DoNotWrapExceptions, null, null, null) is IDisposable currentProcess) |
There was a problem hiding this comment.
Do we need to fix the perf of this one too? How much more expensive is this compared to Windows? Does the cost of this show up in the profiles when you turn on performance counters that call this again and again?
There was a problem hiding this comment.
Ideally, yes. But my benchmarks using BenchmarkDotNet suggested that Linux is at a better state than Windows (>10x) so for now I'm fixing Windows first.
Linux:
| Method | N | Mean | Error | StdDev | Rank |
|----------- |------ |---------:|---------:|---------:|-----:|
| workingset | 10000 | 143.4 us | 2.520 us | 2.801 us | 1 |
Windows (before the fix):
| Method | N | Mean | Error | StdDev | Rank |
|----------- |------ |---------:|----------:|----------:|-----:|
| workingset | 10000 | 1.620 ms | 0.0153 ms | 0.0127 ms | 1 |
In addition to this, the customer requesting this fix is Bing and they are running on Windows.
That being said, the Linux overhead is also huge compared to what we have on Windows after this fix so ideally we'd like to fix it on Linux too:
| Method | N | Mean | Error | StdDev | Median | Rank |
|----------- |------ |---------:|----------:|----------:|---------:|-----:|
| workingset | 10000 | 851.6 ns | 16.916 ns | 34.556 ns | 835.9 ns | 1 |
jkotas
reviewed
Sep 10, 2019
src/System.Private.CoreLib/shared/Interop/Windows/Kernel32/Interop.GetProcessMemoryInfo.cs
Outdated
Show resolved
Hide resolved
jkotas
approved these changes
Sep 10, 2019
jkotas
approved these changes
Sep 11, 2019
stephentoub
approved these changes
Sep 11, 2019
noahfalk
approved these changes
Sep 12, 2019
|
In corefx we currently assert on UWP that |
|
Yes, the changed behavior is fine. |
sywhang
added a commit
to sywhang/coreclr
that referenced
this pull request
Oct 15, 2019
…26522) * Use win32 api directly for workingset counter * Fix build warnings * Removing useless code * more cleanup * remove size annotation * remove useless comment * Move all the changes to Environment.WorkingSet and remove it from RuntimeEventSourceHelper * removing useless usings * Use kernel32.dll instead of psapi.dll * Code review feedback * Remove newline change * More code review nits
sywhang
added a commit
that referenced
this pull request
Oct 16, 2019
…#27212) * Use win32 api directly for workingset counter * Fix build warnings * Removing useless code * more cleanup * remove size annotation * remove useless comment * Move all the changes to Environment.WorkingSet and remove it from RuntimeEventSourceHelper * removing useless usings * Use kernel32.dll instead of psapi.dll * Code review feedback * Remove newline change * More code review nits
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This improves the performance of Environment.WorkingSet by calling Win32 API
GetProcessMemoryInfodirectly to fetch the working set size instead of doing reflection overSystem.Diagnostics.Processand calling get_workingSet64 which does a bunch of allocations and additional API calls.On Linux, there is no simple native API to call and we need to parse procfs. However, from the same benchmark, the Linux is much, much faster than Window when we call
Environment.WorkingSetso I don't think we need to replace it on Linux. (From my bare bones benchmark, it was ~1000x faster on a same machine).This fixes #23736.