-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Remove some allocation at "hello world" startup #44469
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.
Nice!
If these aren't commonly needed, could they be lazily created instead? |
There's the rub: they're public fields. 😦 |
Oh, right. Ugh, public fields. |
I would be ok with that. EventSource is tightly coupled with the rest of the runtime in multiple of ways, so one more does not make a huge difference. Make sure to sprinkle it with |
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.
LGTM! I like EventSource.WriteEventString
change especially, since it's only used for error conditions, but we pay for it all the time.
Ok, I'll add that. |
src/coreclr/src/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs
Outdated
Show resolved
Hide resolved
Removes ~80 allocations at startup.
It's only used on an error path. We don't need to allocate it for each EventSource that's created.
SyncTextWriter already overrides FormatProvider, in which case the t.FormatProvider passed to the base will never be used, so this call is incurring a virtual dispatch for no benefit. And NullTextWriter needn't access InvariantCulture and force it into existence if it isn't yet, as the formatting should never actually be used, and if it is, its FormatProvider override can supply the culture.
…ction with ALC AssemblyLoadContext.OnProcessExit gets called by EventSource, which in turn forces s_allContexts into existence in order to lock on it in order to enumerate all active contexts, and if there's been no interaction with AssemblyLoadContext, there won't be any to enumerate. So delay allocate the object.
Avoids the need to register with AppContext.ProcessExit, avoiding an EventHandler allocation, and avoids the need in the common case to fire AppContext.ProcessExit, which in turn avoids allocating an AppDomain and EventArgs if they weren't otherwise created, plus it avoids the delegate invocation.
c07257f
to
f0c3105
Compare
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
…acing/EventSource.cs
Is it feasible to turn them into properties on a major version of .NET? It seems that callers which recompile should keep working. I don't really see why anyone would access these using reflection. |
This looks odd, that would make every string used during init almost 1kb |
If they recompile against the new surface area, yes. But it's a binary breaking change. So, for example, any netstandard2.0 library that was compiled against the field will break with a MissingFieldException if it runs against a binary that has it instead as a property. |
It's not that every string is 1K. It's that most of the strings are small, but then there's one 30K-ish string (created by the char* passed to AppContext.Setup) for the TPA list. |
Measuring
dotnet helloworld.dll
that just doesConsole.WriteLine(“hello world”);
, on my machine this reduces startup allocation by ~200 objects / 12Kbytes, and reduces wall-clock time by ~8% (from ~80ms to ~73ms). Wall-clock measurement done withMeasure-Command { dotnet helloworld.dll }
. Allocation measurements done with VS .NET allocation profiler.@jkotas, please let me know if you think any of these aren't worthwhile or are problematic in some way and I can roll them back.
Contributes to #44598
cc: @brianrob, @DamianEdwards
For future reference, after this there's still quite a bit of allocation, but it's hard to get rid of a lot more without disabling whole swaths of functionality (e.g. disabling EventSource). Here's what remains:
A few notes on the remaining allocations:
object[]
s appear to be as well, coming from reportInliningDecision using GCHeapHash which grows an array.<>c
-related object.