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

Regression in R2R compilation time #100995

Closed
sebastienros opened this issue Apr 12, 2024 · 21 comments
Closed

Regression in R2R compilation time #100995

sebastienros opened this issue Apr 12, 2024 · 21 comments
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners tenet-performance Performance related issue

Comments

@sebastienros
Copy link
Member

Linux and Windows, PlaintextPlatform benchmark in this example, Single file, Trimmed and Self-contained

image

Binlogs before/after show

image

msbuild.binlog.zip

@sebastienros sebastienros added the tenet-performance Performance related issue label Apr 12, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 12, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Apr 12, 2024
@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2024

@sebastienros is there a commit range? (I don't see an ability to view it on the e.g. Single File tab)

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2024

Likely, ffb2578...e6c1a49

@sebastienros
Copy link
Member Author

That's what I have yes, we could probably go shorter by trying builds from within the range if you don't find anything

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/plaintext.benchmarks.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --scenario plaintext --profile intel-lin-app --profile intel-load-load --application.buildArguments "/p:PublishReadyToRun=true /p:PublishSingleFile=true" --application.framework net9.0 --application.options.collectCounters true --load.options.reuseBuild true --application.aspNetCoreVersion 9.0.0-preview.4.24205.2 --application.runtimeVersion 9.0.0-preview.4.24204.3 --application.sdkVersion 9.0.100-preview.4.24208.11
crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/plaintext.benchmarks.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --scenario plaintext --profile intel-lin-app --profile intel-load-load --application.buildArguments "/p:PublishReadyToRun=true /p:PublishSingleFile=true" --application.framework net9.0 --application.options.collectCounters true --load.options.reuseBuild true --application.aspNetCoreVersion 9.0.0-preview.4.24208.6 --application.runtimeVersion 9.0.0-preview.4.24209.8 --application.sdkVersion 9.0.100-preview.4.24211.4

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2024

TP regressed on NativeAOT too:

image

@sebastienros
Copy link
Member Author

Haven't had time to file this one, are you saying it's related? I wouldn't think so

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2024

Haven't had time to file this one, are you saying it's related? I wouldn't think so

Dates match

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2024

The thing is - when something goes wrong for AOT'd code - JIT can just re-jit it, while NAOT cannot, so it explains why TP only got hurt for NAOT.

@sebastienros
Copy link
Member Author

Here are the commands for the AOT one:

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/goldilocks.benchmarks.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/steadystate.profile.yml --scenario todosapipublishaot --profile intel-win-app --profile intel-lin-load --profile amd-lin2-db --application.packageReferences Microsoft.DotNet.ILCompiler=9.0.0-preview.4.24204.3 --application.environmentVariables DOTNET_GCDynamicAdaptationMode=1 --application.framework net9.0 --application.options.collectCounters true --load.options.reuseBuild true --application.aspNetCoreVersion 9.0.0-preview.4.24205.2 --application.runtimeVersion 9.0.0-preview.4.24204.3 --application.sdkVersion 9.0.100-preview.4.24208.11
crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/goldilocks.benchmarks.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/steadystate.profile.yml --scenario todosapipublishaot --profile intel-win-app --profile intel-lin-load --profile amd-lin2-db --application.packageReferences Microsoft.DotNet.ILCompiler=9.0.0-preview.4.24209.5 --application.environmentVariables DOTNET_GCDynamicAdaptationMode=1 --application.framework net9.0 --application.options.collectCounters true --load.options.reuseBuild true --application.aspNetCoreVersion 9.0.0-preview.4.24208.6 --application.runtimeVersion 9.0.0-preview.4.24209.5 --application.sdkVersion 9.0.100-preview.4.24209.32

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2024

The main suspect is #99982

@sebastienros
Copy link
Member Author

Minimal changeset identified for the nativeaot benchmark:

a8c2a5e...404b286

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2024

Oh, so #100728 then?

@sebastienros
Copy link
Member Author

nativeaot may be special, not sure if moving the runtime version without the corresponding sdk might change the outcome. But at least I set the framework reference and Microsoft.DotNet.ILCompiler to the same runtime. On 9.0.0-preview.4.24208.9+a8c2a5e5bbc2 we get 398K RPS and on 9.0.0-preview.4.24208.10+404b286b2309 only 178K RPS.

@stephentoub
Copy link
Member

Oh, so #100728 then?

Really??

@MichalPetryka
Copy link
Contributor

MichalPetryka commented Apr 12, 2024

Oh, so #100728 then?

Really??

@SingleAccretion suspects this to be the reason since CastCache is a mutable struct.

Feels like there should be a builtin analyzer warning on readonly fields and in parameters with non-readonly structs for this.

@stephentoub
Copy link
Member

stephentoub commented Apr 12, 2024

Ugh. I looked for each such type I wasn't sure about, and I swear I saw it was a class, but I probably incorrectly mistook

class CastCache
for it as I was searching.

@stephentoub
Copy link
Member

Feels like there should be a builtin analyzer warning on readonly fields and in parameters with non-readonly structs for this.

#33773

dotnet/roslyn-analyzers#2811

dotnet/roslyn-analyzers#2831

#50389

@jkotas
Copy link
Member

jkotas commented Apr 13, 2024

Fixed by #100996

@jkotas jkotas closed this as completed Apr 13, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Apr 13, 2024
@EgorBo
Copy link
Member

EgorBo commented Apr 15, 2024

It doesn't look like it's solved, e.g. NAOT run from today is still slow and build time is still slow for R2R

@EgorBo
Copy link
Member

EgorBo commented Apr 15, 2024

@sebastienros can you take a look if it's solved

@sebastienros
Copy link
Member Author

I confirm the NAOT one is fixed, I had to set --application.packageReferences Microsoft.DotNet.ILCompiler=9.0.0-preview.4.24215.2 as I assume it hasn't been ingested by the installer yet. But the build times are still slow. We should check again when we know this is in installer builds.

@sebastienros
Copy link
Member Author

sebastienros commented Apr 17, 2024

All fixed, thank you!

R2R

image

NAOT

image

@github-actions github-actions bot locked and limited conversation to collaborators May 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

5 participants