Skip to content
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

Avoid taking lock for empty bucket in ConcurrentDictionary.TryRemove #82004

Merged
merged 1 commit into from
Feb 12, 2023

Conversation

stephentoub
Copy link
Member

Even when uncontended, the lock represents the most expensive work performed by the method. We can avoid taking it if we see that the bucket's count is empty.

[MemoryDiagnoser(false)]
public partial class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

    private ConcurrentDictionary<MyKey, int> _empty = new ConcurrentDictionary<MyKey, int>();
    private ConcurrentDictionary<MyKey, int> _one = new ConcurrentDictionary<MyKey, int>(new KeyValuePair<MyKey, int>[] { default });

    [Benchmark]
    public bool TryRemoveEmpty() => _empty.TryRemove(default, out _);

    [Benchmark]
    public bool TryRemoveNotEmpty() => _one.TryRemove(default, out _);

    private struct MyKey : IEquatable<MyKey>
    {
        public override int GetHashCode() => 0;
        public bool Equals(MyKey other) => false;
    }
}
Method Toolchain Mean Error StdDev Ratio
TryRemoveEmpty \main\corerun.exe 20.806 ns 0.4297 ns 0.4598 ns 1.00
TryRemoveEmpty \pr\corerun.exe 6.220 ns 0.0567 ns 0.0530 ns 0.30
TryRemoveNotEmpty \main\corerun.exe 20.761 ns 0.2766 ns 0.2452 ns 1.00
TryRemoveNotEmpty \pr\corerun.exe 20.037 ns 0.1032 ns 0.0915 ns 0.97

Even when uncontended, the lock represents the most expensive work performed by the method.  We can avoid taking it if we see that the bucket's count is empty.
@ghost
Copy link

ghost commented Feb 12, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

Even when uncontended, the lock represents the most expensive work performed by the method. We can avoid taking it if we see that the bucket's count is empty.

[MemoryDiagnoser(false)]
public partial class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

    private ConcurrentDictionary<MyKey, int> _empty = new ConcurrentDictionary<MyKey, int>();
    private ConcurrentDictionary<MyKey, int> _one = new ConcurrentDictionary<MyKey, int>(new KeyValuePair<MyKey, int>[] { default });

    [Benchmark]
    public bool TryRemoveEmpty() => _empty.TryRemove(default, out _);

    [Benchmark]
    public bool TryRemoveNotEmpty() => _one.TryRemove(default, out _);

    private struct MyKey : IEquatable<MyKey>
    {
        public override int GetHashCode() => 0;
        public bool Equals(MyKey other) => false;
    }
}
Method Toolchain Mean Error StdDev Ratio
TryRemoveEmpty \main\corerun.exe 20.806 ns 0.4297 ns 0.4598 ns 1.00
TryRemoveEmpty \pr\corerun.exe 6.220 ns 0.0567 ns 0.0530 ns 0.30
TryRemoveNotEmpty \main\corerun.exe 20.761 ns 0.2766 ns 0.2452 ns 1.00
TryRemoveNotEmpty \pr\corerun.exe 20.037 ns 0.1032 ns 0.0915 ns 0.97
Author: stephentoub
Assignees: stephentoub
Labels:

area-System.Collections

Milestone: -

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was tempted to suggest putting the tables._countPerLock[lockNo] != 0 into GetBucketAndLock as a new out bool hasItems, but it doesn't look like any other callers would benefit from that.

Looks good. Thanks!

@stephentoub stephentoub merged commit 252018c into dotnet:main Feb 12, 2023
@stephentoub stephentoub deleted the cdtryremove branch February 12, 2023 23:53
@ghost ghost locked as resolved and limited conversation to collaborators Mar 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants