-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
Added memory optimisations for GetLastActiveSpan #2642
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the wrong synchronization mechanism (RWLock seems to be a better fit) but since this isn't mostly just reads once data is written, in which case Immutable type might be better, how about ConcurrentStack
instead?
Worth noting; These microbenchmarks works on tightloop on a single thread. This is code that will run concurrently and contention is a big concern, so just as much test on a server scenario under a load test is worth going after. edit: This will be mainly used by a single thread on a server mode (non hub global mode) so shouldn't be a big concern with contention |
…m/getsentry/sentry-dotnet into get-last-active-span-optimization
I thought about ConcurrentStack but I need a lock around both the sentry-dotnet/src/Sentry/TransactionTracer.cs Lines 335 to 352 in 78871ba
So I think just a vanilla |
@bruno-garcia off the back of our conversation about upgradable locks, I took a look at For example: sequenceDiagram
Thread1->>LastActiveSpanTracker: EnterUpgradeableReadLock
Thread2->>LastActiveSpanTracker: EnterUpgradeableReadLock
Thread1->>LastActiveSpanTracker: EnterWriteLock (Waits for reads to finish)
Thread2->>LastActiveSpanTracker: EnterWriteLock (Also waits for reads to finish -> Deadlock!)
And I noticed this in the in the remarks:
I assume that's to prevent the above deadlock scenario from playing out. If I understand correctly and if we only have blocks of code leveraging either |
Overview
When load testing an ASP.NET Core application that creates a lot of spans,
GetLastActiveSpan
was responsible for a significant proportion of memory allocation:This appears to be due to the call to
Spans.OrderByDescending
:sentry-dotnet/src/Sentry/TransactionTracer.cs
Line 381 in bbe4fa9
Iteration 1
By replacing this with a for loop, memory consumption in our benchmark dropped by 65.68% from 2109.85 KB to 724.02 KB
Before
After
Iteration 2
By instead keeping all the spans on a Stack and inspecting the most recent one(s) to return the last active span when required we were able to reduce memory usage for the 100 spans scenario drops by a further 63.25% from 724.02 KB to 266.09 KB. So about an ~87% reduction vs the original benchmarks.
After
And finally, a sanity check on a memory snapshot from the same load tests run on the new code: