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

Consider not passing framework references to UnpackLibraryResources #15030

Closed
KirillOsenkov opened this issue May 13, 2022 · 3 comments · Fixed by #21305
Closed

Consider not passing framework references to UnpackLibraryResources #15030

KirillOsenkov opened this issue May 13, 2022 · 3 comments · Fixed by #21305
Assignees
@rolfbjarne
Labels
enhancement The issue or pull request is an enhancement performance If an issue or pull request is related to performance
Milestone
.NET 10

Comments

@KirillOsenkov
Copy link
Member

I'm seeing UnpackLibraryResources take over a second for a minimal project, and I'm guessing the most time is taken by reading all the reference assemblies:

image

Perhaps if we know that none of the in the box assemblies come with resources, we should exclude them from the list being passed to the task? I think this should shave off a second from each project build.

This is dotnet 6.0.202
@KirillOsenkov
Copy link
Member Author

Perhaps fits in #10251

@rolfbjarne
Copy link
Member

We already have logic to skip framework assemblies, but maybe it's not working for .NET projects:

xamarin-macios/msbuild/Xamarin.MacDev.Tasks.Core/Tasks/UnpackLibraryResourcesTaskBase.cs

Lines 55 to 56 in 0ec49ff

if (IsFrameworkAssembly (asm)) {
Log.LogMessage (MessageImportance.Low, MSBStrings.M0168, asm.ItemSpec);

@rolfbjarne rolfbjarne added enhancement The issue or pull request is an enhancement performance If an issue or pull request is related to performance labels May 13, 2022
@rolfbjarne rolfbjarne added this to the .NET 7 milestone May 13, 2022
@KirillOsenkov
Copy link
Member Author

Maybe it would still be better to filter them out on the MSBuild side (instead of in C#) because in MSBuild we know for sure what the framework references are.

@rolfbjarne rolfbjarne modified the milestones: .NET 7, .NET 8 Aug 26, 2022
@rolfbjarne rolfbjarne modified the milestones: .NET 8, .NET 9 Sep 7, 2023
@github-project-automation github-project-automation bot moved this to Optimizations in .NET 9 Aug 27, 2024
@rolfbjarne rolfbjarne mentioned this issue Sep 26, 2024
Merged
@rolfbjarne rolfbjarne modified the milestones: .NET 9, .NET 10 Sep 26, 2024
@rolfbjarne rolfbjarne removed this from .NET 9 Sep 26, 2024
@rolfbjarne rolfbjarne self-assigned this Sep 26, 2024
rolfbjarne added a commit that referenced this issue Oct 2, 2024
@rolfbjarne
[msbuild] Don't try to look for library resources to unpack from BCL …
0478e26 
…assemblies. Fixes #19511. (#21305)

We know BCL won't have library resources to unpack, so there's no need to
spend any time looking at them.

This also required porting the UnpackLibraryResources task to use
System.Reflection.Metadata, because MetadataLoadContext requires the reference
assemblies to be available to resolve assembly dependencies (and the idea is
to not have to pass any reference assemblies to the task).

Fixes #19511.
Fixes #15030.
rolfbjarne added a commit that referenced this issue Oct 22, 2024
@rolfbjarne
[msbuild] Don't try to look for library resources to unpack from BCL …
6b34f26 
…assemblies. Fixes #19511. (#21305)

We know BCL won't have library resources to unpack, so there's no need to
spend any time looking at them.

This also required porting the UnpackLibraryResources task to use
System.Reflection.Metadata, because MetadataLoadContext requires the reference
assemblies to be available to resolve assembly dependencies (and the idea is
to not have to pass any reference assemblies to the task).

Fixes #19511.
Fixes #15030.
rolfbjarne added a commit that referenced this issue Oct 28, 2024
@rolfbjarne
[msbuild] Don't try to look for library resources to unpack from BCL …
78fc91b 
…assemblies. Fixes #19511. (#21305)

We know BCL won't have library resources to unpack, so there's no need to
spend any time looking at them.

This also required porting the UnpackLibraryResources task to use
System.Reflection.Metadata, because MetadataLoadContext requires the reference
assemblies to be available to resolve assembly dependencies (and the idea is
to not have to pass any reference assemblies to the task).

Fixes #19511.
Fixes #15030.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rolfbjarne rolfbjarne

Labels
enhancement The issue or pull request is an enhancement performance If an issue or pull request is related to performance
Projects
None yet
Milestone
.NET 10
2 participants
@rolfbjarne @KirillOsenkov