Skip to content

Link more assemblies (type granularity linking for real this time) #18165

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

Merged

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Jan 7, 2020

This uses @marek-safar's technique for enabling type-granularity linking using the existing XML config capabilities.

It reduces the out-of-box standalone app size to around 1.93MB. This isn't the theoretical minimum we could achieve. We could get it down to about 1.85MB if we were willing to make more fine-grained choices about which assemblies from Microsoft.AspNetCore.* and Microsoft.Extensions.* require type-granularity linking and which don't. That's a risk because if we're wrong then it could fail at runtime if you're using some obscure API that we didn't anticipate. So, in this PR, I'm making a risk-averse choice of saying all those in Microsoft.AspNetCore.* and Microsoft.Extensions.* require type-granularity linking.

Why there's a new MSBuild task that generates XML configs during build

It's necessary to generate the config for this dynamically during the build (and not just embed xml files into the assemblies) because:

  1. We need this type-granularity rule to apply to some assemblies that we don't own in this repo (e.g., things in Microsoft.Extensions.*, because they contains DI services, e.g., for options). So we'd have to put the rules for those in some other assembly.
  2. Sometimes the application might not reference those other assemblies
  3. The linker throws if you try to provide config for an assembly you're not referencing

As such it's not enough to have a fixed set of XML configs. We have to generate some config during the build based on which assemblies are in the transitive closure of your references.

@Pilchie Pilchie added the area-blazor Includes: Blazor, Razor Components label Jan 7, 2020
@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review January 8, 2020 17:20
@SteveSandersonMS SteveSandersonMS requested review from pranavkm, rynowak and javiercn and removed request for pranavkm January 8, 2020 17:20
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

Seems pretty straight forward

<PropertyGroup>
<_TypeGranularityLinkingConfig>$(BlazorIntermediateOutputPath)linker.typegranularityconfig.xml</_TypeGranularityLinkingConfig>
</PropertyGroup>
<GenerateTypeGranularityLinkingConfig Assemblies="@(_BlazorAssemblyToLink)" OutputPath="$(_TypeGranularityLinkingConfig)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Invoking this target should be incremental. For most users, the contents of this file would almost never change. If you'd like to do less MSBuild, you can check this in and make an issue to make it so.

Copy link
Member Author

Choose a reason for hiding this comment

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

This runs if and only if we're going to run the linker. Since the linker is orders of magnitude more expensive than this anyway, I didn't think it would achieve anything to make this bit incremental.

If you still think it's not right, please either file an issue so we track changing it in the future, or just let me know and I will do so. Thanks!

@rynowak
Copy link
Member

rynowak commented Jan 9, 2020

@SteveSandersonMS - did you delete a comment? I saw a big comment from you in my notifications, but now it's gone.

@SteveSandersonMS
Copy link
Member Author

did you delete a comment

Yes, previously this PR contained a completely different approach to solving this problem, and I wrote a comment saying "I realised this approach won't work, here's why, and what I will change". Then I changed the implementation completely based on what I had learned. Then I deleted that comment in an attempt to avoid confusion, and ironically created confusion by doing so.

@rynowak
Copy link
Member

rynowak commented Jan 9, 2020

Then I changed the implementation completely based on what I had learned. Then I deleted that comment in an attempt to avoid confusion, and ironically created confusion by doing so.

Cool. It worked.

@SteveSandersonMS SteveSandersonMS merged commit a181fd5 into blazor-wasm Jan 10, 2020
@SteveSandersonMS SteveSandersonMS deleted the stevesa/type-granularity-linking-for-real branch January 10, 2020 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants