X Tutup
The Wayback Machine - https://web.archive.org/web/20200911071232/https://github.com/npgsql/npgsql/pull/2758
Skip to content

/ npgsql

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

Only register cancellation token callbacks if needed #2758

Open
wants to merge 1 commit into
base: dev
from
Open

Only register cancellation token callbacks if needed #2758

wants to merge 1 commit into from

Conversation

@roji
Copy link
Member

roji commented Dec 4, 2019

Rather than always doing registrations on cancellation tokens, do it only if the cancellation token supports it (i.e. isn't CancellationToken.None). This is probably not very meaningful for Npgsql, but why not do it (in hot paths).

Method Mean Error StdDev
Register 4.620 ns 0.1257 ns 0.1761 ns
Skip 1.674 ns 0.0089 ns 0.0079 ns
SkipWithNullable 10.556 ns 0.1327 ns 0.1241 ns

Benchmark code:

public class CancellationRegistrationBenchmarks
{
    static readonly CancellationToken _token = CancellationToken.None;

    [Benchmark]
    public void Register()
    {
        using (_token.Register(x => _token.Register(() => Thread.Sleep(10)), this))
        {
        }
    }

    [Benchmark]
    public void Skip()
    {
        CancellationTokenRegistration registration = default;
        if (_token.CanBeCanceled)
            registration = _token.Register(x => _token.Register(() => Thread.Sleep(10)), this);

        try
        {

        }
        finally
        {
            registration.Dispose();
        }
    }

    [Benchmark]
    public void SkipWithNullable()
    {
        CancellationTokenRegistration? registration = default;
        if (_token.CanBeCanceled)
        {
            registration = _token.Register(x => _token.Register(() => Thread.Sleep(10)), this);
        }

        try
        {

        }
        finally
        {
            registration?.Dispose();
        }
    }
}
@roji
Only register cancellation token callbacks if needed
Loading status checks…
983be5d
@roji roji requested review from austindrenski and YohDeadfall as code owners Dec 4, 2019
@roji roji added the performance label Dec 4, 2019
@YohDeadfall
Copy link
Member

YohDeadfall commented Dec 5, 2019

Interesting... Could you post the generated assembly? Register and Skip should produce near the same result because CanBeCanceled implemented as _source != null, and Register does the same thing internally.
@YohDeadfall
Copy link
Member

YohDeadfall commented Jan 25, 2020

Can I use SynchronizationContext.SetSynchronizationContext to avoid using ConfigureAwait(false)?, it's near the end of article.
@roji
Copy link
Member Author

roji commented Jan 25, 2020

Yeah. BTW the way things are going with the multiplexing work, we will probably be able to get rid of SingleThreadSynchronizationContext altogether and just do ConfigureAwait(false) as normal. SingleThreadSynchronizationContext was done to avoid deadlocking when executing batches synchronously, and so the same codepaths need to sometimes run with ConfigureAwait(false) (normal async operations) and sometimes on special threads outside the TP (to avoid the deadlock).

Am not sure yet, we'll see.
@roji
Copy link
Member Author

roji commented Jan 25, 2020

BTW are you OK with merging this? If you want to investigate the assembly go ahead :)
@roji roji force-pushed the dev branch from a3bb792 to 4e700d5 Feb 5, 2020
@roji roji force-pushed the dev branch from dccaced to 8eb495f May 13, 2020
@NinoFloris
Copy link
Member

NinoFloris commented May 24, 2020 •
edited

@YohDeadfall https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABABgAJiBGAOgBUALWbAEwEsA7AcwG4BYAFDEATFUFlKtAEoBXDhjb4YNAMIR8ABzYAbGFADKegG5swMXPyEBmSsPIrsHM9u3YFEDlJhc2uDFDc2DwAhGCcGfGwoAGtcQQBvQXJk8mYWD20AT3tHZ1d3DjoIaLDyAH0MYtKAXhynGBdAjyKSjhoAOQ8YSxTUmFYM7OphAB4IYAArGDAMAD5y2C5yWq8uYZ6UtMHJOzLcXRgNZfJ9A40N5KSU4htqJEoUE7OACgBKZfnGNJpTmEPnqikV4XSi3Kj3YiPVbDZ7jKYzchGV5XXrJarzZ7PBz1RoFVa+fxNDihcKRGK4V5ImgVKptfF+PSYrG5Br5IKebwEgIFElgCJRWKU17U/Z/DTAlHkSU3B7kekYRnIgS9RLK1HJCTPGmtGjyxllRZocgYBi+V5K9XJVWW8gAX0l9oE0tuj300TYGjekutluxeSJLTC8u57L6Pj8IY8xxYMAAZtgZNoMCDemxY+QtZUddjQn6GjAWBabZtORGicdtWFdaWFVAtYbjaaKZZJb1/JlWylVZ3ko6bbHONgXB21eqfcWw1yiTQACK+DQQXAwN4plJ9teCZ2yt0egDqbBN7UTrmAui9o/I49Redx7MDHPDhIKAH5J2WCtG4wmk6vkmmM5WbQ5jAN4FkWE6LFOH61IB1aPvqDYmmaLYXm2UAjja3aoRu2HJAOHBDlkPaXsRvSQe+7LPrO86LsuEq4XaDqCLaQA==

(it's not a 100% copy, sharplab doesn't allow static constructors in the asm view, roslyn generates these by default for lambdas)

So the main thing here is that it seems the private Register method (4 args) doesn't get inlined, probably because it contains argument validation (throws). That fact alone can explain the difference of ~3ns. Would be interesting to see what .NET 5 does here.
@roji
Copy link
Member Author

roji commented May 24, 2020 •
edited

Not quite sure which preview this is but:

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.3.20216.6
  [Host]     : .NET Core 5.0.0 (CoreCLR 5.0.20.21406, CoreFX 5.0.20.21406), X64 RyuJIT
  DefaultJob : .NET Core 5.0.0 (CoreCLR 5.0.20.21406, CoreFX 5.0.20.21406), X64 RyuJIT
Method Mean Error StdDev
Register 4.471 ns 0.0344 ns 0.0288 ns
Skip 1.392 ns 0.0283 ns 0.0236 ns
SkipWithNullable 1.647 ns 0.0348 ns 0.0291 ns

SkipWithNullable performs better
@NinoFloris
Copy link
Member

NinoFloris commented May 24, 2020

Yep, nullable got quite some tweaks, cool to see the culmination. I assume register is still not inlined though. As far as I know one of the latest previews should contain rudimentary auto throw helper jitting, that would then free Register itself for inlining.
@roji
Copy link
Member Author

roji commented May 24, 2020

Cool, thanks for the info. We can wait with this PR and revisit in another preview or two for sure.
@roji
Copy link
Member Author

roji commented May 24, 2020

(your responsibility to ping me for more benchmarking 😆)
@YohDeadfall
YohDeadfall approved these changes May 25, 2020
View changes
Copy link
Member

YohDeadfall left a comment

Okay, let's make it a bit prettier and merge.
src/Npgsql/ConnectorPool.cs
CancellationTokenRegistration registration = default;
if (cancellationToken.CanBeCanceled)
registration = cancellationToken.Register(s => ((TaskCompletionSource<NpgsqlConnector?>)s!).SetCanceled(), tcs);
try
{
await tcs.Task;
}
finally
{
registration.Dispose();
}
Comment on lines +333 to +343

This comment has been minimized.

Sign in to view
@YohDeadfall

YohDeadfall May 25, 2020

Member
Suggested change
CancellationTokenRegistration registration = default;
if (cancellationToken.CanBeCanceled)
registration = cancellationToken.Register(s => ((TaskCompletionSource<NpgsqlConnector?>)s!).SetCanceled(), tcs);
try
{
await tcs.Task;
}
finally
{
registration.Dispose();
}
using (var registration = cancellationToken.CanBeCanceled
? cancellationToken.Register(s => ((TaskCompletionSource<NpgsqlConnector?>)s!).SetCanceled(), tcs)
: default)
{
await tcs.Task;
}
src/Npgsql/NpgsqlCommand.cs
using (cancellationToken.Register(cmd => ((NpgsqlCommand)cmd!).Cancel(), this))
CancellationTokenRegistration registration = default;
if (cancellationToken.CanBeCanceled)
registration = cancellationToken.Register(cmd => ((NpgsqlCommand)cmd!).Cancel(), this);

This comment has been minimized.

Sign in to view
@YohDeadfall

YohDeadfall May 25, 2020

Member

And the same here.
@roji
Copy link
Member Author

roji commented May 27, 2020

@YohDeadfall looks good, but I propose waiting a bit - if @NinoFloris is right, changes in the runtime/BCL would make all of this superfluous anyway... If the current code as-is runs eventually runs just as fast, better to keep it that way...
@NinoFloris
Copy link
Member

NinoFloris commented Jul 13, 2020

From https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-5/

ThrowHelper contains non-inlinable methods like ThrowArgumentNullException, which contains the actual throw and avoids the associated code size at every call site; the JIT currently isn’t capable of “outlining”, the opposite of “inlining”, so it needs to be done manually in cases where it matters.

This was the original relevant issue dotnet/runtime#34522 (comment):

With this optimization we don't need to throw helperize, it more or less does that automagically. Note there are pass-through benefits in many cases as prologs are simpler and we have less zeroing, since the string formatting done in many throws creates structs; these costs are now "deferred" to the remainder method that is jitted only if the exceptional path is reached.

It has been moved to after 5.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YohDeadfall YohDeadfall

@austindrenski austindrenski

Assignees
No one assigned
Labels
performance
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@roji @YohDeadfall @NinoFloris
You can’t perform that action at this time.
X Tutup