Skip to content
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

Use non-generic Array.Sort in EnumInfo on nativeaot #79473

Merged
merged 4 commits into from
Dec 14, 2022

Conversation

stephentoub
Copy link
Member

Contributes to #79437

@jkotas, worthwhile? We'll pay boxing costs as part of the sort, but it should be only once per enum and only if the values aren't already sorted.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@hez2010
Copy link
Contributor

hez2010 commented Dec 12, 2022

I personally would like to take the size regression rather than the unnecessary boxing overhead (it's important for game development)

@MichalPetryka
Copy link
Contributor

MichalPetryka commented Dec 12, 2022

I personally would like to take the size regression rather than the unnecessary boxing overhead (it's important for game development)

The boxing here is one time, in fact I feel like having to JIT multiple times would take more than boxing. Not sure about the AreSorted checks though.

@hez2010
Copy link
Contributor

hez2010 commented Dec 12, 2022

JIT multiple times would take more than boxing

NativeAOT won't JIT at runtime.

@jkotas
Copy link
Member

jkotas commented Dec 12, 2022

This does not actual help much (HelloWorld before this change 2,904,576, after this change 2,897,920). The non-generic Array.Sort is special-cased for primitive types

case CorElementType.ELEMENT_TYPE_I1:
. It means we still get the Sort specializations for most primitive types.

I have tried the following:

            if (!Enum.AreSorted(values))
            {
                object[] boxes = new object[values.Length];
                for (int i = 0; i < values.Length; i++)
                    boxes[i] = values[i];

                Array.Sort(keys: boxes, items: names); // using object overload to avoid generic expansion for every underlying enum type

                for (int i = 0; i < values.Length; i++)
                    values[i] = (TUnderlyingValue)boxes[i];
            }

It brought the size down to 2,819,072.

The nativeaot reflection stack reads the enum values as underlying value boxes first. We can save the potential boxing and avoid some of the code duplication by sorting the enum values in the reflection stack while we have the boxes around, pass it as object[] to EnumInfo constructor like what it was done before this change, and the EnumInfo constructor can just assert that the values are sorted.

The best solution for the sort footprint problem would be to sort the values at aot compilation time. It would make the sort go away completely.

@jkotas
Copy link
Member

jkotas commented Dec 12, 2022

For reference, this is my workflow for using local native AOT builds:

1. `build.cmd -s clr.aot+libs+packs -c Release` (it fails when building packs - ignore the error)

Create native AOT hello world that points to your artifacts directory:

1. `dotnet new console`
2. `dotnet new nugetconfig`
3. Replace the feed in nuget.config to point to your artifacts directory 
 
 <packageSources>
    <add key="nuget" value="C:\runtime\artifacts\packages\Release\Shipping" />
  </packageSources>


4. Add this to .csproj

<PropertyGroup>
  <PublishAot>true</PublishAot>
</PropertyGroup>

<ItemGroup>
  <PackageReference Include="Microsoft.DotNet.ILCompiler" Version="8.0.0-dev" />
</ItemGroup>


5. `dotnet publish -c Release -r win-x64 --packages pkg`. The native aot compiled .exe is going to be `bin\Release\net7.0\win-x64\publish\*.exe`
6. `rmdir /s /q bin obj pkg` to pickup a new local build

@stephentoub
Copy link
Member Author

We can close this then and I can revisit in January, unless you'd like to take it over before then.

@jkotas
Copy link
Member

jkotas commented Dec 12, 2022

I can take it over.

@stephentoub
Copy link
Member Author

stephentoub commented Dec 12, 2022

The non-generic Array.Sort is special-cased for primitive types

Those generic specializations won't actually be used here, though, as this check won't be met:

if (items == null || items.GetCorElementTypeOfElementType() == et)

I assume the nativeaot compiler can't see that?

@jkotas
Copy link
Member

jkotas commented Dec 12, 2022

I assume the nativeaot compiler can't see that?

Right. Our codegen does not perform global optimizations like this today. The codegen can only see things like this through aggressive inlining that is very limited.

@stephentoub
Copy link
Member Author

Thanks. I know that's true with the JIT; I incorrectly thought we did more whole-program optimization like that with nativeaot.

@jkotas
Copy link
Member

jkotas commented Dec 13, 2022

@MichalStrehovsky PTLA

This fixes some of the size regression by reverting to non-specialized sort, similar to what we have used before.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

For the "sort in the compiler TODO" - we would have to do this by sorting the fields themselves. Do we have any compat concerns around doing that? .NET Native used to come up with random order of metadata and I do recall it being a problem (not for enums, but in general).

@jkotas
Copy link
Member

jkotas commented Dec 14, 2022

we would have to do this by sorting the fields themselves. Do we have any compat concerns around doing that?

Good point. We have fixed reflection to return *Infos in metadata order for predictability, so this would regress the behavior. I will delete the TODO.

…Internal/Reflection/Execution/NativeFormatEnumInfo.cs
@jkotas jkotas merged commit 322f20c into dotnet:main Dec 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 13, 2023
@stephentoub stephentoub deleted the enumnongenericsort branch March 17, 2023 18:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants