-
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
Improve the performance of ConditionalWeakTable.TryGetValue #80059
Conversation
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsFixes #80032
|
...libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/ObjectHeader.cs
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/ObjectHeader.cs
Outdated
Show resolved
Hide resolved
src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs
Outdated
Show resolved
Hide resolved
It does not guarantee that hash codes are non-zero.
Co-authored-by: Jan Kotas <[email protected]>
...libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
I would not expect 20% perf regression with the current change. Is it repeatable? Have you looked at the generated code to see what caused it? |
My bad, the benchmark runner was not using the correct runtime, because I did not pass the |
I updated the benchmark to properly read arguments and try a few different number of objects. It now shows that trying to get a value that is not in the using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
namespace MyBenchmarks
{
public class TryGetHashCode
{
private readonly List<object> mRootedObjects = new();
private readonly ConditionalWeakTable<object, object> mWeakTable = new();
private object mAnObjectInTheTable = null!;
[Params(1, 100, 1000, 10000)]
public int NumberOfObjects;
[GlobalSetup]
public void Setup()
{
for (int i = 0; i < NumberOfObjects; i++)
{
var obj = new object();
mRootedObjects.Add(obj);
mWeakTable.Add(obj, new object());
mAnObjectInTheTable = obj;
}
}
[Benchmark]
public bool TryGetNonExistentValue()
{
return mWeakTable.TryGetValue(new object(), out object _);
}
[Benchmark]
public bool TryGetExistingValue()
{
return mWeakTable.TryGetValue(mAnObjectInTheTable, out object _);
}
}
public class Program
{
public static void Main(string[] args)
{
var summary = BenchmarkRunner.Run<TryGetHashCode>(null!, args);
}
}
} BenchmarkDotNet=v0.13.3, OS=ubuntu 22.04
AMD Ryzen Threadripper PRO 3955WX 16-Cores, 1 CPU, 32 logical and 16 physical cores
.NET SDK=7.0.101
[Host] : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2
Job-FOVPQH : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-WAHSHU : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
// * Warnings *
MultimodalDistribution
TryGetHashCode.TryGetExistingValue: Toolchain=merge-base -> It seems that the distribution is bimodal (mValue = 3.29)
|
…erServices/ConditionalWeakTable.cs Co-authored-by: Jan Kotas <[email protected]>
This reverts commit 6a13d23.
I added an implementation for Mono. One thing to note is that unlike CoreCLR, the hashcodes generated for objects are not deterministic. So the hash table could potentially have a different number of collisions run-to-run. I ran the benchmarks twice and confirmed the ratio between merge-base and PR were roughly the same. Here is the results for running Mono on x64 Linux. I used these directions to run the benchmarks. I used #80082 to include BenchmarkDotNet=v0.13.3, OS=ubuntu 22.04
AMD Ryzen Threadripper PRO 3955WX 16-Cores, 1 CPU, 32 logical and 16 physical cores
.NET SDK=7.0.101
[Host] : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2
Job-PGRQPW : .NET 8.0.0 (42.42.42.42424) using MonoVM, X64 VectorSize=128
Job-SWPJPT : .NET 8.0.0 (42.42.42.42424) using MonoVM, X64 VectorSize=128
I also benchmarked WASM running on V8. I followed these directions to build the runtime. I modfied my benchmark to run against the WASM runtime. See this branch: https://github.com/AustinWise/TryGetHashCodeBenchmark/tree/wasm BenchmarkDotNet=v0.13.3, OS=ubuntu 22.04
AMD Ryzen Threadripper PRO 3955WX 16-Cores, 1 CPU, 32 logical and 16 physical cores
.NET SDK=7.0.101
[Host] : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2
PR : .NET Core (Mono) 8.0.0-dev, Wasm AOT
merge-base : .NET Core (Mono) 8.0.0-dev, Wasm AOT
Runtime=Wasm IterationCount=3 LaunchCount=1
WarmupCount=3
V8 version 11.1.92
|
@vargaz @lambdageek Could you please review Mono changes? |
...libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs
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.
Mono changes LGTM
This is dead code because this method no longer lives on System.Object.
With the interpreter transforms actually kicking in, the improvments for Mono WASM are even better. At least I assume that is the reason, I could not figure out how run the interpreter under a debugger. BenchmarkDotNet=v0.13.3, OS=ubuntu 22.04
AMD Ryzen Threadripper PRO 3955WX 16-Cores, 1 CPU, 32 logical and 16 physical cores
.NET SDK=7.0.101
[Host] : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2
PR : .NET Core (Mono) 8.0.0-dev, Wasm AOT
merge-base : .NET Core (Mono) 8.0.0-dev, Wasm AOT
Runtime=Wasm IterationCount=3 LaunchCount=1
WarmupCount=3
V8 version 11.1.92
|
@AustinWise Could you please resolve the merge conflict? |
@AustinWise Thank you! |
@AustinWise Thanks for getting this through!! |
Thanks for all the help with reviewing! Perhaps this PR should be labeled-with |
I don't look at the labels. I look at every pr that comes through and track them on a list of prs to consider... this one is already on my list :) |
Also fixes Objective-C reference tracking in NativeAOT, which was broken by #79519.
Fixes #80032
Related issue: #77472