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

Expiring set lock free #6577

Merged
merged 12 commits into from
Jul 29, 2019
Merged

Conversation

danielmarbach
Copy link
Contributor

@danielmarbach danielmarbach commented Jun 13, 2019

Related to Azure Service Bus

Lock-free alternative to #6576

  • Properly cancels the Task.Delay if cleanup was scheduled to not leak the task for the duration of the timeout
  • Removes the .Keys access that locks the whole concurrent dictionary
  • Makes sure to remove the key only if the snapshot matches by using collection remove

Caveat (but highly unlikely):

  • Due to the field exchange, we might miss a TrySetResult on the old TCS and therefore would not run the cleanup again until the next add or update comes. This spinning solution in the second latest commit might decrease that change even more but I let it to the team reviewing the PR to decide what they want to take
        [Benchmark(Baseline = true)]
        public void Old()
        {
            Parallel.For(0, 1000, i => { oldSet.AddOrUpdate(i, DateTime.UtcNow.AddSeconds(5)); });
        }

        [Benchmark]
        public void New()
        {
            Parallel.For(0, 1000, i => { newSet.AddOrUpdate(i, DateTime.UtcNow.AddSeconds(5)); });
        }

With spinning

image

Without

TBD
image

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@danielmarbach
Copy link
Contributor Author

After thinking about this a bit with my friend @Scooletz we concluded spinning is not necessary and simply changing the volatile field is good enough. Will benchmark this again tomorrow.

@danielmarbach
Copy link
Contributor Author

@nemakam I pushed an update. Let me know what you think

@danielmarbach danielmarbach force-pushed the expiring-set-lock-free branch from e9766a9 to 51c1026 Compare July 19, 2019 08:44
@danielmarbach danielmarbach force-pushed the expiring-set-lock-free branch from 51c1026 to febefba Compare July 19, 2019 08:48
{
await cleanupTaskCompletionSource.Task.ConfigureAwait(false);
this.cleanupTaskCompletionSource = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
await Task.Delay(delayBetweenCleanups, token).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this also throw ObjectDisposedException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it can given that dispose of the CTS only releases the timer and the linked registrations if any

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't completely know the inner details of CTS. I am going to trust your word that this would not throw ObjectDisposedException and also that this.dictionaryAsCollection.Remove(kvp); will not throw while iteration/dictionary.Clear() is in progress.

Copy link
Contributor

@nemakam nemakam left a comment

Choose a reason for hiding this comment

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

:shipit:

@nemakam nemakam merged commit 412acbe into Azure:master Jul 29, 2019
@nemakam
Copy link
Contributor

nemakam commented Jul 29, 2019

Thanks @danielmarbach for this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants