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

Eliminate boxing when entering critical sections #21230

Merged
merged 5 commits into from
Jun 12, 2020
Merged

Conversation

roji
Copy link
Member

@roji roji commented Jun 12, 2020

Closes #21229

…sposer.cs

Co-authored-by: Smit Patel <smitpatel@users.noreply.github.com>
@smitpatel
Copy link
Member

no tests? 😢

Co-authored-by: Andriy Svyryd <AndriySvyryd@users.noreply.github.com>
@roji
Copy link
Member Author

roji commented Jun 12, 2020

@smitpatel I think all the existing ones in ConcurrencyDetectorTestBase are valid, no? Or you mean you want explicit testing for ExitCriticalSection?

/// A <see cref="IDisposable" /> returned by an <see cref="IConcurrencyDetector" />, which will exit the ongoing
/// critical section when disposed.
/// </summary>
public class ConcurrencyDetectorCriticalSectionDisposer : IDisposable
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a struct, so we will still be allocating. What's the purpose of this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, that was a typo (it's 3AM here). Changing to readonly struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

(maybe concurrency-related programming at 3AM isn't such a good idea?)

Copy link
Member

@AndriySvyryd AndriySvyryd Jun 12, 2020

Choose a reason for hiding this comment

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

I thought it was an intentional bug like what Arthur suggested to include in every PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah of course, that's what it was. Congrats for winning the prize @AndriySvyryd!

@@ -60,10 +51,16 @@ public virtual IDisposable EnterCriticalSection()
}

_refCount++;
return _disposer;
return new ConcurrencyDetectorCriticalSectionDisposer(this);
Copy link
Member

Choose a reason for hiding this comment

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

Does this not create a new struct every time? (rather than reusing like how previous did)

Copy link
Member

Choose a reason for hiding this comment

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

struct construction is cheap

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, but creating a struct (on the stack) is free - especially as here it's just wrapping a pointer. So it's the same as allocating a long, or a reference on the stack.

@roji
Copy link
Member Author

roji commented Jun 12, 2020

Just for fun, here's the benchmark to show this:

BenchmarkDotNet=v0.12.0, OS=ubuntu 20.04
Intel Xeon W-2133 CPU 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=5.0.100-preview.6.20266.3
  [Host]     : .NET Core 3.1.1 (CoreCLR 4.700.19.60701, CoreFX 4.700.19.60801), X64 RyuJIT
  DefaultJob : .NET Core 3.1.1 (CoreCLR 4.700.19.60701, CoreFX 4.700.19.60801), X64 RyuJIT

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
MyDisposable 0.0226 ns 0.0030 ns 0.0027 ns - - - -
Disposable 6.4407 ns 0.2355 ns 0.2202 ns 0.0056 - - 24 B
Benchmark code
[MemoryDiagnoser]
public class Program
{
    [Benchmark]
    public void MyDisposable()
    {
        using var _ = GetMyDisposable();
    }

    [Benchmark]
    public void Disposable()
    {
        using var _ = GetDisposable();
    }

    static IDisposable GetDisposable() => new MyDisposable();

    static MyDisposable GetMyDisposable() => new MyDisposable();

    static void Main(string[] args)
        => BenchmarkRunner.Run<Program>();
}

readonly struct MyDisposable : IDisposable
{
    public void Dispose() {}
}

Now am off to fix the test failures...

@roji roji merged commit a132b90 into master Jun 12, 2020
@roji roji deleted the ConcurrencyDetector branch June 12, 2020 21:49
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.

Remove allocation from ConcurrencyDetector critical section management
3 participants