Reduce closure allocations in TokenizerBackedParser.ConfigureSpanContext#12238
Conversation
…eSpanContext Move the closure allocation to it's own method which should slightly reduce allocations when there isn't an existing SpanContextConfig. Will do a speedometer run with this change to make sure it effects things the way I think it does.
DustinCampbell
left a comment
DustinCampbell
left a comment
There was a problem hiding this comment.
Move the closure allocation to it's own method which should slightly reduce allocations when there isn't an existing SpanContextConfig.
I must be missing something here. Instead of creating a closure directly, the code now calls a method that creates a closure. I'm not seeing any new conditions where the closure would be created less often.
| : GetNewSpanContextConfigAction(config, SpanContextConfig); | ||
| InitializeContext(); | ||
|
|
||
| static SpanContextConfigAction GetNewSpanContextConfigAction(SpanContextConfigActionWithPreviousConfig config, SpanContextConfigAction? prev) |
There was a problem hiding this comment.
This is uncommon enough that it I think it probably warrants a comment that it needed to avoid allocations.
|
|
||
| protected void ConfigureSpanContext(SpanContextConfigActionWithPreviousConfig? config) | ||
| { | ||
| var prev = SpanContextConfig; |
There was a problem hiding this comment.
Can this line be kept? It seems a bit harder to read when SpanContextConfig is set to an expression that uses the previous value of SpanContentConfig?
DustinCampbell
left a comment
DustinCampbell
left a comment
There was a problem hiding this comment.
I see why this reduces allocations now. I wasn't seeing that the compiler unconditionally creates the closure class outside of the ternary, even if it's not used. I had assumed that we'd get more efficient code, but we don't. 😞
3 participants
Reduce closure allocations in TokenizerBackedParser.ConfigureSpanContext
Move the closure allocation to it's own method which should slightly reduce allocations when there isn't an existing SpanContextConfig.
Speeedometer run failed for some reason and I don't want to wait another 8 hours to get verify what I'm pretty confident in already.
Test insertion pipeline run (failed): https://dev.azure.com/dnceng/internal/_build/results?buildId=2795099&view=results