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

Add OptimizationPreference=Speed NativeAOT runs #1840

Merged
merged 3 commits into from
Apr 27, 2023

Conversation

MichalStrehovsky
Copy link
Contributor

This is now a documented option: https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/optimizing

I've put it on the same schedule as PGO runs since it's a similar thing but I don't know the motivation behind it. Should I tweak the modulus remainder to a different number than 1?

Cc @sebastienros @eerhardt @DamianEdwards @JamesNK

This is now a documented option: https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/optimizing

I've put it on the same schedule as PGO runs since it's a similar thing but I don't know the motivation behind it. Should I tweak the modulus remainder to a different number than 1?
@@ -40,6 +40,11 @@ parameters:
arguments: --scenario basicminimalapipublishaot $(goldilocksJobs) --property scenario=Stage1AotServerGC --property publish=nativeaot --application.packageReferences \"Microsoft.Dotnet.ILCompiler=$(MicrosoftNETCoreAppPackageVersion)\" --application.buildArguments \"/p:ServerGarbageCollection=true\"
condition: 'true'

- displayName: Goldilocks Stage 1 (NativeAOT - Optimize for Speed)
# workaround https://github.com/dotnet/runtime/issues/81382 by explicitly referencing a Microsoft.Dotnet.ILCompiler version
Copy link
Contributor

Choose a reason for hiding this comment

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

@agocke @sbomer @vitek-karas - do we have a story in place for this issue and dotnet/runtime#84372 yet?

One thing that is annoying about this workaround is that when you copy the crank command out of the dashboard, it still has Microsoft.Dotnet.ILCompiler=$(MicrosoftNETCoreAppPackageVersion) in it, and when you simply run the command you get:

MicrosoftNETCoreAppPackageVersion: The term 'MicrosoftNETCoreAppPackageVersion' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

cc @sebastienros

@sebastienros
Copy link
Member

@MichalStrehovsky

I don't know the motivation behind it

This pipeline is executed 2 times per day, but we don't need to run some of these jobs that often in order to limit how long each run takes. This property will send the command to crank only if the condition is true, in this case by specifying how many half-days it should run, every 6th one in this case. And then some of them run on the first 6th, the second one. This way the native-aot job will usually take the same amount of time even though not the same jobs run on each iteration.

@eerhardt
Copy link
Contributor

Should I tweak the modulus remainder to a different number than 1?

I think that would make sense. If we put the "OptimizationPreference=Speed" on a different day (but still every 6 half days).

@eerhardt
Copy link
Contributor

And then some of them run on the first 6th, the second one

note that in the current code in main, there are 4 of these conditions. 3 of them are == 1 and 1 of them is == 2. Maybe we should take this opportunity to split them up.

@sebastienros
Copy link
Member

@eerhardt sure, you don't have to for the ones that you really want to track as much as possible. But since we don't do changes more often than twice a day it doesn't really make sense to not do it.
Makes even more sense for the runs that take longer, like the native-aot ones where the build is slower.

Copy link
Contributor

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks! Let me know if you don't have permissions to merge, and I can.

@MichalStrehovsky
Copy link
Contributor Author

Yep, I don't have permissions to merge this. Thanks!

@eerhardt eerhardt merged commit f916dec into aspnet:main Apr 27, 2023
@MichalStrehovsky MichalStrehovsky deleted the patch-1 branch April 27, 2023 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants