-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Size regression in Minimal API NativeAOT app #82732
Comments
Tagging subscribers to this area: @dotnet/area-system-collections Issue DetailsDescriptionBetween builds Configuration
Regression?Yes. From a previous build. DataHere are the crank commands to repro:
and
AnalysisBy diffing the And drilling into the And inspecting the diff between the two builds: ff7c19f...5ba867f, this leads me to believe the regression was caused by #82499. (cc @stephentoub) I've zipped the before and after mstat files, along with a text dump in BasicMinimalApi.zip, if anyone wants to take a look.
|
What would you suggest be done here, @eerhardt? The PR makes it so that enumerating an empty collection (for our core collection types) returns a singleton enumerator, saving the associated allocation costs. Is 140KB in the minimal API app worth the tradeoff? |
I'm not sure. I mostly wanted to have a conversation on if there is anything we could do here. Is there something smarter we can do either in IL, or in the ILC, to share/reuse code here? cc @MichalStrehovsky A lot of the entries appear to be
It represents about 1% of the app right now, and hopefully will be more like 1.5% of the app by the time .NET 8 ships. So, not huge. |
The SZArray types are unshareable. NativeAOT uses different array enumerators that are size optimized: runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs Line 1301 in eccbe90
The SZArray enumerator type implements array enumerators in CoreCLR only. It would be good not to use array enumerators for this in general (not just "don't use CoreCLR's array enumerator in NativeAOT"). Array enumerators are covariant and have a bunch of magic pixie dust that makes them harder to optimize. |
I meant "array covariant" - so |
Does that suggest that if we changed the call sites I added in corelib from: SZGenericArrayEnumerator<T>.Empty to ((IEnumerable<T>)EmptyArray<T>.Value).GetEnumerator() or something similar, it would do the right thing? Or does that open its own can of worms due to the possibly additional reference to |
The problem is that each
There is no good reason for this duplication. We should be able to unify these implementations. |
That might be the best solution. |
WIP: #82751 |
Is this what you had in mind? I wouldn't expect that to help with the diff Eric showed, though; all of those extra instantiations are from a new use of the array enumerator type, rather than from |
Yes, something like that. I would also move methods that do not depend on generics to non-generic base type, similar to how it is done for regular array iterator after my PR. I have done some ad-hoc analysis to find array types instantiated just for the empty enumerator in BasicMinimalApi. Here is the list:
So I would say that special casing the empty enumerator is worth it for KeyValuePair (75% of the uses above). |
This saves 56kB in BasicMinimalApi app on top of the 68kB saving from #82751. I think it is worth taking. |
Ok, I'll wait until yours goes in and then put up a PR for mine. |
* Unify Array enumerators between CoreCLR and NativeAOT Contributes to #82732 Reduces BasicMinimalApi native AOT size on Windows x64 by 68kB * Update AOT compiler * Naming convention Co-authored-by: Stephen Toub <[email protected]>
Description
Between builds
8.0.0-preview.2.23123.4+ff7c19f
and8.0.0-preview.2.23123.5+5ba867f
there was a ~140KB size regression on linux-x64 Minimal API NativeAOT app.Configuration
Regression?
Yes. From a previous build.
Data
Here are the crank commands to repro:
and
Analysis
By diffing the
mstat
files between these two builds, it appears the majority of the size regression comes from theSystem
namespace.And drilling into the
System
namespace, I see a bunch ofSZGenericArrayEnumerator`1
types that weren't there before, but are now.And inspecting the diff between the two builds: ff7c19f...5ba867f, this leads me to believe the regression was caused by #82499. (cc @stephentoub)
I've zipped the before and after mstat files, along with a text dump in BasicMinimalApi.zip, if anyone wants to take a look.
The text was updated successfully, but these errors were encountered: