-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use ArrayPool instead of having a private buffer cache #45690
use ArrayPool instead of having a private buffer cache #45690
Conversation
Tagging subscribers to this area: @eiriktsarpalis Issue DetailsContributes to #45315
|
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
… objects), switch to Marshal.AllocHGlobal instead
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
@adamsitnik Now that ArrayPool was fixed to pool unlimited size arrays, I think this PR can be resurrect and updated to use ArrayPool unconditionally. |
@jkotas done, PTAL |
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jan Kotas <[email protected]>
5e56129
to
fc4378f
Compare
It looks good to me. I think it would be useful to verify that there is advantage in using the array pool instead of a simpler NativeMemory.Alloc in this situation - #45690 (comment) . |
I did some measurements using modified benchmarks from performance repo: [Benchmark]
public void GetProcessesByName()
{
foreach (var process in Process.GetProcessesByName(_nonExistingName))
{
process.Dispose();
}
}
[Benchmark(OperationsPerInvoke = 10 * 24)]
public void GetProcesses_Parallel()
{
Parallel.For(0, 24, _ => // my PC has 24 cores
{
for (int i = 0; i < 10; i++)
{
foreach (var process in Process.GetProcesses())
{
process.Dispose();
}
}
});
}
[Benchmark(OperationsPerInvoke = 10 * 24)]
public void GetProcessesByName_Parallel()
{
Parallel.For(0, 24, _ =>
{
for (int i = 0; i < 10; i++)
{
foreach (var process in Process.GetProcessesByName(_nonExistingName))
{
process.Dispose();
}
}
});
} And the results were following: BenchmarkDotNet=v0.13.0.1559-nightly, OS=Windows 10.0.19043.1165 (21H1/May2021Update)
AMD Ryzen Threadripper PRO 3945WX 12-Cores, 1 CPU, 24 logical and 12 physical cores
.NET SDK=6.0.100-rc.1.21417.19
@stephentoub @jkotas I don't have a strong opinion here. What are your thoughts on this? |
Nit: It can be just NativeLibrary.Alloc. It guarantees sufficient alignment. AlignedAlloc is unnecessary.
I would lean towards using I do not have a strong opinion on this either. Thank you for collecting the numbers! |
I've switched to
@jkotas thank you for a lot of good hints and great discussion! |
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Contributes to #45315
Citrine (28 cores):
Perf (12 cores):
No difference for micro-benchmarks (this was expected):