Suppress heap allocations in TracerSettings.IsIntegrationEnabled#772
Conversation
| { | ||
| return true; | ||
| isAppInsight = AppDomainName.IndexOf("ApplicationInsights", StringComparison.OrdinalIgnoreCase) >= 0; | ||
| appDomain.SetData(IsAppInsightKey, isAppInsight); |
There was a problem hiding this comment.
Since we only use the current appdomain, I'm wondering if we can go even further and cache the value in a static field. With all those "domain neutral" thingies, I'm not sure anymore of whether this instance is shared across appdomains or not
There was a problem hiding this comment.
Static fields are not shared across AppDomains (even if the assembly is domain-neutral) so that's a good idea. There's some good information here on how domain-neutral assemblies are treated: https://docs.microsoft.com/en-us/dotnet/framework/app-domains/application-domains
There was a problem hiding this comment.
Accessing static fields is slower if the assembly is shared. Since all the AppDomains share the same code, but the field value is not shared, static field access requires an extra level of indirection. If we will be reading this field a lot in domain-neutral scenarios, making it static could be counter-productive.
On the other hand, I don't think we are ever loaded domain-neutral in AAS, so if this value is only read in AAS, it's probably fine.
There was a problem hiding this comment.
GetData does a bunch of checks, acquires a lock, then fetches the boxed value from a dictionary. My gut feeling is that whatever mechanism is involved in fetching static field in domain-neutral assemblies, it shouldn't be slower than that. Unfortunately, the assumption seems really hard to test :/
| public static bool ShouldAvoidAppDomain() | ||
| { | ||
| if (AppDomainName.IndexOf("ApplicationInsights", StringComparison.OrdinalIgnoreCase) >= 0) | ||
| var appDomain = AppDomain.CurrentDomain; |
There was a problem hiding this comment.
Please not the Slack discussion on using VAR. Depending on how that goes, this would be an example where var should not be used. :)
There was a problem hiding this comment.
I really wish, for the sake of my muscle memory, that we'll end-up allowing both :P
| @@ -100,12 +101,15 @@ public static int AppDomainId | |||
|
|
|||
| public static bool ShouldAvoidAppDomain() | |||
There was a problem hiding this comment.
@lucaspimentel For my education - why are we checking for AppInsights here?
There was a problem hiding this comment.
In azure app services there are AppInsights AppDomains which spin up underneath the web worker and those would either break or submit traces for AppInsights.
It is a best effort at harmonious co-existence.
|
Rewrote to use a static field + added a unit test (this bit of logic seems to critical not to have one). New benchmark: BenchmarkDotNet=v0.12.0, OS=Windows 10.0.19041
Intel Core i7-9750H CPU 2.60GHz, 1 CPU, 12 logical and 6 physical cores
[Host] : .NET Framework 4.8 (4.8.4180.0), X64 RyuJIT
Job-VTGAFA : .NET Framework 4.8 (4.8.4180.0), X64 RyuJIT
Job-MNUBEA : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
I also realized, while writing the test, that the whole thing could be eluded on .net core (since there are no appdomains), but I'd say it's cheap enough not to bother for now. |
5 participants
Benchmark: