Skip to content

perf: Speed up SDK init#2784

Merged
bruno-garcia merged 17 commits intogetsentry:mainfrom
jairbubbles:speed-up-sdk-init
Nov 8, 2023
Merged

perf: Speed up SDK init#2784
bruno-garcia merged 17 commits intogetsentry:mainfrom
jairbubbles:speed-up-sdk-init

Conversation

@jairbubbles
Copy link
Contributor

@jairbubbles jairbubbles commented Nov 3, 2023

In some scenarios the time spent in initializing the SDK can be too important. What is tricky is that the init of the SDK is one of the first thing the app do and we're JITing almost everything. Creating a HttpClient, a Regex is costly...

In this pull request I'm experimenting with lazy instanciation so all that JIT time for these types will be spent afterwards.

I'm disabling almost everything to be in the best scenario possible:

await Task.Run(() =>
{
    SentrySdk.Init(o =>
    {
        o.Dsn = "<sentry_dsn>";
        o.Release = "1.0.0";
        o.DisableAppDomainProcessExitFlush();
        o.DisableAppDomainUnhandledExceptionCapture();
        o.DisableUnobservedTaskExceptionCapture();
        o.DisableDuplicateEventDetection();
        o.DisableDiagnosticSourceIntegration();
        o.DisableWinUiUnhandledExceptionIntegration(); // this one is new!
        o.AutoSessionTracking = false;
        o.IsGlobalModeEnabled = true;
        o.DetectStartupTime = StartupTimeDetectionMode.Fast;
        o.TracesSampleRate = 0;
    });
});

Here is the current status:

image

Compared to main branch:

image

Let me know what you think!

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for this PR, the idea makes sense. I realize it's a draft but left some notes already.

Could you please add some benchmarks in https://github.com/getsentry/sentry-dotnet/tree/main/benchmarks ?

@jairbubbles jairbubbles changed the title Speed up SDK init perf: Speed up SDK init Nov 4, 2023
@jairbubbles
Copy link
Contributor Author

jairbubbles commented Nov 4, 2023

Could you please add some benchmarks in https://github.com/getsentry/sentry-dotnet/tree/main/benchmarks ?

I struggled a bit to add the benchmarks as I needed a specific "ColdStart" configuration but here we go:

Method Mean Error StdDev Min Max Median Methods JITted Methods Tiered JIT allocated memory Allocated
'Init SDK (Full)' 47.02 ms 2.512 ms 1.661 ms 45.32 ms 50.16 ms 46.63 ms 1,051 1 192 KB 43.83 KB
'Init SDK (Slim)' 28.04 ms 2.237 ms 1.479 ms 27.01 ms 32.18 ms 27.61 ms 900 1 165 KB 39.87 KB

But it's not so convincing when running it on main to compare:

Method Mean Error StdDev Min Max Median Methods JITted Methods Tiered JIT allocated memory Allocated
'Init SDK (Full)' 53.49 ms 4.709 ms 3.115 ms 51.52 ms 61.17 ms 52.12 ms 1,060 1 191 KB 49.09 KB
'Init SDK (Slim)' 44.01 ms 3.667 ms 2.426 ms 41.61 ms 50.16 ms 43.20 ms 962 1 175 KB 33.35 KB

@jairbubbles
Copy link
Contributor Author

Some notes:

  • Dns.Parse is still taking a bit of time (20ms), maybe we could do a simple verification in the constructor and lazy init it?
  • Some features which I disabled in my use case were taking a lot of time : in particular AutoSession is writing / reading a file for the persistant id.

@vaind
Copy link
Contributor

vaind commented Nov 4, 2023

Dns.Parse is still taking a bit of time (20ms),

that's not a bit of time, that's a lot of time for such a simple operation (seemingly). Could use a bit more investigation.

@jairbubbles
Copy link
Contributor Author

Dns.Parse is still taking a bit of time (20ms),

that's not a bit of time, that's a lot of time for such a simple operation (seemingly). Could use a bit more investigation.

Yeah but it's due to JIT.

@bruno-garcia
Copy link
Member

Dns.Parse is still taking a bit of time (20ms),

that's not a bit of time, that's a lot of time for such a simple operation (seemingly). Could use a bit more investigation.

Yeah but it's due to JIT.

We looked at it with @bitsandfoxes when profiling this and IIRC it was mainly the Uri class, most time spent.
If that's the case we could prob stop using it and take a simpler approach.
We parse it in other SDKs, e.g in C: https://github.com/getsentry/sentry-native/blob/406d41881111d15b1144b443d51760c1415c8029/src/sentry_utils.c#L216-L270

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

This is quite promising.Once it's out of draft we can test it against feat/4.0.0 to make sure it works fine with AOT

@bruno-garcia
Copy link
Member

But it's not so convincing when running it on main to compare:

Would be more noticible if we had some "application code" right after Init. That now would get a chance to run before some gets sent into Sentry vs before where it would need everything to bootstrap

@jairbubbles
Copy link
Contributor Author

With latest changes, here are the benchmarks results:

Method Mean Error StdDev Min Max Median Methods JITted Methods Tiered JIT allocated memory Allocated
'Init SDK (Full)' 42.91 ms 1.679 ms 1.110 ms 41.51 ms 44.63 ms 42.52 ms 1,028 1 186 KB 43.05 KB
'Init SDK (Slim)' 25.45 ms 0.981 ms 0.649 ms 24.51 ms 26.25 ms 25.56 ms 891 1 161 KB 27.22 KB

@jairbubbles jairbubbles marked this pull request as ready for review November 5, 2023 09:49
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Great stuff! Thanks @jairbubbles
Prob needs @bitsandfoxes @vaind or @jamescrosswell as some extra check before we can merge but lgtm

@bitsandfoxes
Copy link
Contributor

Those are some serious gains! I'll follow up with a review/merge this tomorrow.

Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

Let me run this against 4.0.0 before merging to avoid any heartache.

@bruno-garcia
Copy link
Member

Lets merge this and figure out AOT after

@bruno-garcia bruno-garcia merged commit 0f8c491 into getsentry:main Nov 8, 2023
@bruno-garcia
Copy link
Member

Thanks @jairbubbles

@bitsandfoxes
Copy link
Contributor

Already tested. Works like a charm. No hickups there!

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.

4 participants