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

ConcurrentDictionary.Count bypass #11212

Merged
merged 3 commits into from
Jan 13, 2025
Merged

ConcurrentDictionary.Count bypass #11212

merged 3 commits into from
Jan 13, 2025

Conversation

SimaTian
Copy link
Member

@SimaTian SimaTian commented Jan 3, 2025

profiler was repeatedly complaining about ConcurrentDictionary.Count taking locks too often.
This PR introduces an approximate counter to remedy that

Context

concurrentDictionary
ConcurrentDictionary.Count locks all its internal locks to have the count accurate and up to date.
However we only use the count to check if there is a reason to clean up the cache - e.g. we should be fine with a variable that is almost-in-sync with the .Count, that we can update atomically and then read without locking.
The increment is atomic, the read is accurate enough and the "flush cache" section is already behind a lock.

Changes Made

Introduced _count variable that lists the same value as ConcurrentDictionary.Count would.

Testing

Now that I looked at this, we didn't have a test for the scavenge threshold. We only ever tested the .scavenge directly.
Do we want to remedy that or are we fine as is?

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Thanks!

src/StringTools/WeakStringCache.Concurrent.cs Outdated Show resolved Hide resolved
SimaTian and others added 3 commits January 8, 2025 13:33
@SimaTian SimaTian force-pushed the concurrentdictionary-count branch from 9002d58 to 053a431 Compare January 8, 2025 12:33
@SimaTian SimaTian merged commit 3898228 into main Jan 13, 2025
10 checks passed
@SimaTian SimaTian deleted the concurrentdictionary-count branch January 13, 2025 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants