Conversation
| { | ||
| if (HeaderMatchesKey(currentHeaders[index], key)) | ||
| { | ||
| currentHeaders[index] = string.Concat(key, "=", value); |
There was a problem hiding this comment.
Note: Tried making it a variable, IL adds an extra allocation for that, so kept in three places on purpose.
There was a problem hiding this comment.
That sounds suspicious. Are you counting locals (stack slots) from IL? Those aren't the kind of allocations (GC heap) to be worried about.
There was a problem hiding this comment.
Yep. ldloc.1 and similar. Memory hit here is fine, i'd like to avoid extra instructions if possible :)
Not to the point of one allocation of course, but no allocation simply sounded better than "an allocation" upon review of IL here. Not a problem, can move to a variable if you feel it's absolutely necessary.
There was a problem hiding this comment.
IL instruction count isn't really that important. Sure, fewer is generally better (you can fit more on a cache line) but, of course, it's the generated machine code that really matters. An IL "slot" is required for local variables (and args) and they'll usually map to a register or a BP-relative memory location. We might call the latter "stack allocation", but it's nowhere near the same as a GC allocation. In this case, I'd expect that the total IL instruction count for this method when hoisting the expression to a local variable (common subexpression elimination) will be less overall -- because of the saved duplicate code. I doubt the C# compiler will do the hoisting itself -- and if it did, it'd use a stack slot.
This is all a long explanation for why you shouldn't worry about IL slots for perf.
For this case, I don't mind how you write it. I just wanted to explain why "IL adds an extra allocation" raised my Spock-like eyebrow and, hopefully, educate the team :-)
There was a problem hiding this comment.
Cool! Great to know!
| if (environment != null && !telemetry.Context.Properties.ContainsKey(AspNetCoreEnvironmentPropertyName)) | ||
| { | ||
| telemetry.Context.GlobalProperties.Add(AspNetCoreEnvironmentPropertyName, environment.EnvironmentName); | ||
| telemetry.Context.Properties.Add(AspNetCoreEnvironmentPropertyName, environment.EnvironmentName); |
There was a problem hiding this comment.
If we can avoid touching properties at all in default scenarios, then that'll also save the perf hit from instantiating the Properties Conc.Dictionary.
Can you try if changes similar to this(https://github.com/Microsoft/ApplicationInsights-dotnet/pull/929/files) can be applied he as well? We'll need to modify all telemetry types to have this special internal field.
https://github.com/Microsoft/ApplicationInsights-dotnet/pull/929/files
There was a problem hiding this comment.
Actually, don't we stamp each request with Pre-Aggregation's "_MS.AggregatedByMetricExtractors" or similar? Which dictionary does it use - we should just use that one since it's initialized for each request anyway.
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
Have left a comment about avoiding .Properties completely
4 participants
Initialize()toInitializeInsturmenationKey()to prevent second round of Telemetry InitializersGetUriupdates, 2x gain (this bug is still an issue, though)SetHeaderKeyValueupdates, 10x gainPropertiesfor a while again to avoid second concurrent dictionary initialization forGlobalProperties.For significant contributions please make sure you have completed the following items: