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.

Sign up for GitHub

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

Jump to bottom

PublishTrimmed build time regression. #40060

Closed
sebastienros opened this issue Apr 8, 2024 · 5 comments · Fixed by dotnet/runtime#100897
Closed

PublishTrimmed build time regression. #40060

sebastienros opened this issue Apr 8, 2024 · 5 comments · Fixed by dotnet/runtime#100897
Labels
Area-Workloads untriaged Request triage from a team member

Comments

@sebastienros
Copy link
Member

image

Not happening with all applications. Reproduced on Windows and Linux.

Between SDKs (installer) 9.0.100-preview.2.24112.28 and 9.0.100-preview.2.24113.7

Before:

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-win-app --profile intel-load2-load --application.buildArguments "/p:PublishReadyToRun=true /p:PublishSingleFile=true /p:PublishTrimmed=true" --application.framework net9.0 --application.aspNetCoreVersion 9.0.0-preview.2.24112.24 --application.runtimeVersion 9.0.0-preview.2.24112.16 --application.sdkVersion 9.0.100-preview.2.24112.28 --profile noload

After

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-win-app --profile intel-load2-load --application.buildArguments "/p:PublishReadyToRun=true /p:PublishSingleFile=true /p:PublishTrimmed=true" --application.framework net9.0 --application.aspNetCoreVersion 9.0.0-preview.2.24112.24 --application.runtimeVersion 9.0.0-preview.2.24112.16 --application.sdkVersion 9.0.100-preview.2.24113.7 --profile noload
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Workloads untriaged Request triage from a team member labels Apr 8, 2024
@baronfel
Copy link
Member

baronfel commented Apr 8, 2024

@sebastienros is it possible to collect and provide binlogs for these test runs? That would be the minimum to do any analysis here.

@sebastienros
Copy link
Member Author

msbuild.zip

I used a different machine with less cores and it was actually quicker. Still noticeable difference.

@sebastienros
Copy link
Member Author

sebastienros commented Apr 8, 2024 •
edited
Loading

Before

image

After

image

@vitek-karas
Copy link
Member

@sbomer @jtschuster - probably some of the recent changes.

@sbomer sbomer mentioned this issue Apr 9, 2024
Merged
@sbomer
Copy link
Member

sbomer commented Apr 9, 2024

Linker diff between those package versions:
6ac1edf0b2b Mark most specific static DIM for types marked RelevantToVariantCasting (#97487)
8af0b009868 Don't use IEnumerable and yield return for GetDefaultInterfaceImplementations (#98183)

@jtschuster jtschuster mentioned this issue Apr 11, 2024
Merged
jtschuster added a commit to dotnet/runtime that referenced this issue Apr 12, 2024
@jtschuster
Improve performance on virtual method processing (#100897)
7b18be5 
The _virtual_methods cache had duplicates due to the Scope item in the tuple, which made ProcessVirtualMethods do significantly more work. This PR makes the cache a dictionary, with TryAdd's instead of Add to only add each method once. These changes made caused the overrides cache in TypeMapInfo to be modified during iteration, so a for loop is used instead of a foreach loop. This should be find since the list will always be append-only. I noticed we copy _typesWithInterfaces to an array for the same reason, though since that's also append-only we can iterate with a for loop there, too.

Fixes dotnet/sdk#40060.
matouskozak pushed a commit to matouskozak/runtime that referenced this issue Apr 30, 2024
@jtschuster @matouskozak
Improve performance on virtual method processing (dotnet#100897)
8aec7c1 
The _virtual_methods cache had duplicates due to the Scope item in the tuple, which made ProcessVirtualMethods do significantly more work. This PR makes the cache a dictionary, with TryAdd's instead of Add to only add each method once. These changes made caused the overrides cache in TypeMapInfo to be modified during iteration, so a for loop is used instead of a foreach loop. This should be find since the list will always be append-only. I noticed we copy _typesWithInterfaces to an array for the same reason, though since that's also append-only we can iterate with a for loop there, too.

Fixes dotnet/sdk#40060.
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this issue May 30, 2024
@jtschuster @Ruihan-Yin
Improve performance on virtual method processing (dotnet#100897)
d42122d 
The _virtual_methods cache had duplicates due to the Scope item in the tuple, which made ProcessVirtualMethods do significantly more work. This PR makes the cache a dictionary, with TryAdd's instead of Add to only add each method once. These changes made caused the overrides cache in TypeMapInfo to be modified during iteration, so a for loop is used instead of a foreach loop. This should be find since the list will always be append-only. I noticed we copy _typesWithInterfaces to an array for the same reason, though since that's also append-only we can iterate with a for loop there, too.

Fixes dotnet/sdk#40060.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Workloads untriaged Request triage from a team member
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve performance on virtual method processing jtschuster/runtime
4 participants
@baronfel @sbomer @sebastienros @vitek-karas