Trimming BlazorWeather app is slow

[marek-safar]

Repro:

• Clone https://github.com/danroth27/blazorweather
• dotnet build BlazorWeather.Maui -t:Run /bl
• Wait...

For me, the linker takes ~20 seconds:

      489 ms  Csc                                        3 calls
     1073 ms  ACTool                                     1 calls
     1848 ms  MSBuild                                   22 calls
     5429 ms  RestoreTask                                1 calls
    19462 ms  ILLink                                     1 calls

For other people (with different hardware), it can be much worse:

      323 ms  _ComputeManagedAssemblyToLink              1 calls
      352 ms  _CodesignAppBundle                         1 calls
      463 ms  GeneratePublishDependencyFile              1 calls
      611 ms  XamlC                                      1 calls
      638 ms  ResolveProjectReferences                   3 calls
    73156 ms  _RunILLink                                 1 calls

This is a "don't link" scenario (LinkMode=None), so it shouldn't take this long.

[marek-safar]

I suspect this is due to the new dataflow/diagnostics work but didn't validate that.

/cc @vitek-karas

[vitek-karas]

Still no luck trying to repro this on Windows.

I was able to build dotnet build BlazorWeather.Maui without errors using .NET 6 Preview 5 (latest build). But this does not seem to run linker at all. The ILLink target is skipped because PublishTrimmed property is not set.
It did build the project for ios, android and maccatalyst.

Trying to run the command line from the repro:
dotnet build BlazorWeather.Maui -t:Run fails with:

 error MSB4057: The target "Run" does not exist in the project.

I also tried running a simple console app to maybe determine if this is a wider non-MAUI related issue. By default linker takes almost 2000ms to run:

• LinkMode doesn't exist
• TrimMode=none fails since it's not a valid value
• TrimMode=skip works, and it makes linker run about 2.5x faster, but it still costs almost 700ms

Can't tell if "skip" is the mode used by Maui here though. If we can confirm that, I will look into why linker takes 700ms to run in that mode.

Would it be possible to get the msbuild binlog for the run where linker is actually used?

[filipnavara]

I had to add --framework net6.0-ios to get the Run target to work: dotnet build --framework net6.0-ios BlazorWeather.Maui -t:Run /bl.

msbuild.binlog.zip

[vitek-karas]

Thank a lot Filip. Still can't make it work on Windows (it builds but I don't see any linker invocation anywhere). But the binlog you sent helps a lot.

The linker is running in the "copy" mode for all assemblies (TrimMode=copy) which means that nothing is removed from the output, but it also means that all assemblies are fully analyzed. That's why it's so slow. Even a simple console app takes 7+ seconds to process in this mode (as compared to 2 seconds when actually trimming).

It still doesn't fully explain the 67 seconds runtime in the MAUI case though. I will have to play with that a little bit more. It's also true that the MAUI invocation is running some custom steps which might add to the runtime.

It's unclear to me why MAUI enforces full copy mode on all assemblies in this case.

[filipnavara]

There are custom steps for sure. The copy mode comes from https://github.com/xamarin/xamarin-macios/blob/045ccaf3a6ac9759eb9166fb01815b0e49f981ef/dotnet/targets/Xamarin.Shared.Sdk.targets#L267-L270. The iOS Xamarin linker: https://github.com/xamarin/xamarin-macios/tree/045ccaf3a6ac9759eb9166fb01815b0e49f981ef/tools/dotnet-linker.

[vitek-karas]

Thanks Filip. I was writing it down here more as a "documentation" of what's going on.

I don't know if we can turn off the expensive analysis though. Even if all assemblies are "copy" the analysis might still add more assemblies which would otherwise be removed from the output. So from a correctness perspective most of the analysis still has to happen. But I guess we could try to disable parts which don't have a way to expand the assembly closure.

[filipnavara]

It's non-trivial to untangle which of the custom steps may have some dependencies on the analysis. Doesn't the new data flow analysis only reduce the possible assembly closure as opposed to expanding it?

If there's any data that I can gather I'd be happy to do so. The MSBuild log was generated with the shipped .NET 6 P4 but it would likely make sense to retry running it with P6 once all repositories branch and get the dependencies. Notably xamarin/xamarin-macios#11739 changed how the custom steps are called which may result in slightly different data.

[vitek-karas]

In most cases the data flow should make linker more precise, but it also started to recognize patterns which linker didn't know about before.

For example it now correctly handles Type.GetType("type, assembly"). Before this while recognized could not expand the assembly closure. Now it can. And transitively anything which makes linker scan through more code can lead to assembly closure expansion since the new code may contain a call to Type.GetType. So from a purely correctness perspective, it would be wrong to disable large parts of the analysis, even when running in copy mode.

@sbomer for comments on the interactions between full copy mode and the new custom steps.

[filipnavara]

For example it now correctly handles Type.GetType("type, assembly").

Thanks for explanation.

In the "Don't link" mode of Xamarin.iOS (which results in TrimMode=copy) it may actually be fine to assume the pessimistic closure of all assemblies that were in the published output. There are some steps which look for P/Invokes called from the code and others to look for classes with certain attributes. I'm not sure how much of the context is available/necessary to perform these steps.

[sbomer]

It sounds like running the linker is necessary even with LinkMode=None so that custom steps get a chance to gather information from the app for things like the static registrar and native symbols (thanks @spouliot). Maybe there's opportunity to reduce the custom step work that happens with LinkMode=None, but the main issue is that the linker will do the work to process everything in the app as @vitek-karas pointed out.

I took a look at how this works with the legacy macios linker with LinkMode=None:

• The action for framework assemblies is copy: https://github.com/xamarin/xamarin-macios/blob/ccd769bd9d995baf357117417d089584a0994454/tools/mtouch/Tuning.mtouch.cs#L69
• The action for user assemblies appears to be link: https://github.com/xamarin/xamarin-macios/blob/2b7dc07d20fc06344d7f164af8934f5e755572c2/tools/common/DerivedLinkContext.cs#L104

So one difference is that the legacy linker wouldn't set all assemblies to copy, while the new linker does.

More importantly, the legacy linker doesn't even run MarkStep when LinkMode=None: https://github.com/xamarin/xamarin-macios/blob/e19591f47b27f34881b94402d66cc6b69a142877/tools/mtouch/Tuning.mtouch.cs#L144-L154. They were able to customize this behavior by compiling the linker sources into their own tool. But this approach doesn't work with the .NET SDK's linker.

Some possible ways we may be able to fix this:

1. Add a linker API that allows SDKs to take control of the linker pipeline
2. Add a new linker option which doesn't do any trimming or analysis (copy, but don't analyze)
   • These would let it serve as a host for custom logic, but makes "trimming" a bit of a misnomer in that case.
   • This could be a new assembly action (similar to what "copy" used to mean), or a new flag that skips marking.
3. Rework the macios logic to not invoke the linker when LinkMode=None
   • They would need to invoke an alternate tool that runs the custom logic
   • Since the custom logic uses the linker APIs, the custom tool would likely look similar to 1. anyway

[rolfbjarne]

For example it now correctly handles Type.GetType("type, assembly"). Before this while recognized could not expand the assembly closure. Now it can.

A consequence of this seems to be that running the linker on an assembly can actually make the app larger... and then you run into a different correctness issue: the app might require the linker to run, to pull any additional assemblies into the app.

For Apple platforms (iOS, tvOS, Mac Catalyst, macOS), I believe we can say that this is not something we need to support, we can require that all assemblies must be passed to the linker.

[marek-safar]

@rolfbjarne what information you need to extract when running LinkMode=None ?

I believe we can say that this is not something we need to support, we can require that all assemblies must be passed to the linker.

How will the developers know they need to include an assembly manually in Debug mode?

[rolfbjarne]

@rolfbjarne what information you need to extract when running LinkMode=None ?

We need to:

1. Find all binding assemblies, extract any embedded native libraries from them, and get any required flags to pass to the native linker.
2. Look at all the P/Invokes, and figure out if there are any native symbols that we must tell the native linker to not link away. We also use this information to figure out which native frameworks we need to ask the native linker to link with.
3. It's also possible to refer to native symbols that aren't functions - we list all such native symbols and in this case too we must tell the native linker to not link these native symbols away.

If we run the static registrar (which is optional, but results in faster startup time), then the static registrar may need to inspect every assembly. It's not really feasible to limit exactly what the static registrar may or may not need to inspect.

I believe we can say that this is not something we need to support, we can require that all assemblies must be passed to the linker.

How will the developers know they need to include an assembly manually in Debug mode?

I'm not sure I follow. I'm assuming that by Debug mode you mean that PublishTrimmed = false?

My point is that because user code such as Type.GetType("type, assembly") may make the linker load additional assemblies, it follows that:

• PublishTrimmed = true: the app works, because the additional assemblies are loaded by the linker, and thus copied to the app.
• PublishTrimmed = false: the app does not work, because the additional assemblies are not loaded / copied to the app, because the linker isn't run.

My suggestion is to disallow this scenario by asking the linker to not load any assembly the linker doesn't already know about. This way we get the same behavior between PublishTrimmed = true and PublishTrimmed = false (neither will work for code that relies on the linker loading additional assemblies).

The end result here is that if the linker can assume that Type.GetType("type, assembly") won't expand the assembly closure, the linker won't need to process such expressions.

In other words, it's not wrong anymore to disable large parts of the analysis, like this statement says:

So from a purely correctness perspective, it would be wrong to disable large parts of the analysis, even when running in copy mode.

[sbomer]

I'm not sure the Type.GetType behavior is the most important bit here.

For example it now correctly handles Type.GetType("type, assembly"). Before this while recognized could not expand the assembly closure. Now it can.

This is true, but it can only expand the closure up to the set of assemblies passed into it by the SDK. So the PublishTrimmed = false output should always be a superset of the PublishTrimmed = true output (and if not, it's a bug). The copy settings for LinkMode=None also ensure that the entire input set is kept, even if not statically referenced by the app.

The end result here is that if the linker can assume that Type.GetType("type, assembly") won't expand the assembly closure, the linker won't need to process such expressions.

When linking with trimming (LinkMode!=None), Type.GetType needs to be processed (assuming the target environment supports reflection) because the reflection target could be an assembly that's not statically referenced. But with the current LinkMode=None settings, as far as I understand, it shouldn't really make a difference in terms of what is kept.

If the target doesn't support reflection, then in theory LinkMode=None would only need to keep the static reference closure instead of everything passed to the linker, but the current marking behavior still means that the entire reference closure would be processed by MarkStep, which might still be prohibitively expensive.

[rolfbjarne]

This is true, but it can only expand the closure up to the set of assemblies passed into it by the SDK.

Ah ok, I thought it could expand it even further. That removes the concern I had about apps only working with the linker enabled.

If the target doesn't support reflection, then in theory LinkMode=None would only need to keep the static reference closure instead of everything passed to the linker, but the current marking behavior still means that the entire reference closure would be processed by MarkStep, which might still be prohibitively expensive.

I'm not quite sure I understand what you're saying here, but I believe you're saying that we have basically two options (with LinkMode=None):

1. Output every assembly passed to the linker.
2. Prune assemblies that aren't referenced by the main executable, either directly or indirectly (for any definition of "to be referenced by").

In legacy Xamarin code we used option 2, and where "to be referenced by" meant either:

a. An assembly reference.
b. Show up in a linker description file passed to the linker.

In .NET this has changed so that "to be referenced by" also means if there's a Type.GetType("type, assembly") somewhere, but that's prohibitively expensive (which is what this issue is about). So we're back to either option 1, or option 2 with a different definition of "to be referenced by" (at the very least one that doesn't require marking all the assemblies).

Is there any particular reason we can't go with what we do in legacy Xamarin?

[MichalStrehovsky]

Is any of the illinker logic required in this special mode?

It kind of feels like all of the logic we are talking about (computing stuff to pass to native linker) should actually be done by the AOT compiler (that's the one producing outputs for the native linker), but nobody wants to write that logic in C and ILLinker was a convenient place to put it in - "we have a C# app looking at all the assemblies, so we can make it do other stuff too".

I kind of like Sven's option 3 - if we're not trimming, illinker should not run and if Xamarin has steps it still needs to run, maybe a small C#-based host that lets the custom steps execute would be a good way out without having to twist illinker arms to suppress behaviors its designed to do.

[sbomer]

I believe you're saying that we have basically two options (with LinkMode=None):

Yup, thanks for laying it out so clearly. More specifically, option 2. (Prune assemblies that aren't referenced...) is only correct if we do handle Type.GetType, otherwise trimming might remove an assembly needed by the app, breaking the app compared to non-trimmed publish.

I was also trying to point out that even if we are ok with ignoring Type.GetType, the current semantics of copy still mark the whole assembly. So your option 2 breaks down into:

2.1 Prune assemblies that aren't referenced directly, keeping copy semantics (marking referenced assemblies)
2.2 Prune assemblies that aren't referenced directly, without marking referenced assemblies

2.1 could significantly improve perf (especially for small apps) compared to marking the whole universe like we do today, but still wouldn't get us to the legacy behavior. (Maybe worth measuring if we're ok with the correctness issue?) My point is that whether or not we prune references, we may need some way to avoid marking the kept assemblies.

Is there any particular reason we can't go with what we do in legacy Xamarin?

I see two: 1. the correctness concern around not seeing Type.GetType, and (whether or not we respect Type.GetType) 2. the fact that the API doesn't allow skipping MarkStep. The ideas I gave in #2089 (comment) were to address the second piece (skipping MarkStep).

[spouliot]

Trying this on RC1 but I had a few (mostly fixed) issues.

/usr/local/share/dotnet/sdk/6.0.100-rc.1.21452.64/Sdks/Microsoft.NET.Sdk.Razor/targets/Microsoft.NET.Sdk.Razor.StaticWebAssets.targets(795,7): error : Manifest file at 'obj/Debug/net6.0/staticwebassets.build.json' not found. [/Users/poupou/git/bugs/BlazorWeather/BlazorWeather/BlazorWeather.csproj]

@danroth27 any clue ?

[rolfbjarne]

@sbomer @marek-safar are there been any updates on this from the your side? IIRC last we talked you were going to look at it and see if you could come up with something.

[vitek-karas]

I have a "hacky" solution here: https://github.com/vitek-karas/linker/tree/DisableMarkingOfCopyAssemblies
It assumes that the linker runs in "copy" mode for all assemblies (otherwise it's potentially going to break things). Pass --custom-data DisableMarkingOfCopyAssemblies=true on the command line and it will be MUCH faster.

On a Windows console hello world with "copy" action on everything:

• Before: 8.1 seconds
• After: 2.1 seconds

[rolfbjarne]

@vitek-karas would it be possible to get packages with those changes somehow?

[vitek-karas]

Hmm - if the "solution" (as in relying on "copy" on everything and using the command line mentioned above) works for you, I think we could merge it into the product. Which branch do you rely on main (7.0) or release/6.0.2xx ?

[rolfbjarne]

@vitek-karas We're still tracking 6.0.1xx, but we're probably going to switch to 6.0.2xx soon, so I guess that would work

[rolfbjarne]

@vitek-karas would it be possible to get packages with those changes somehow?

I tried this, and the improvement is pretty substantial for a simple hello-world app: it went from building in 19s to 7.5s

I think this is good to go in now :)

[vitek-karas]

Perfect - thanks a lot!

Created #2370 to adds this to 6.0.2xx. If we need it sooner we can try that as well, once it's merged.

[rolfbjarne]

@vitek-karas this fix doesn't seem to be in 6.0.2xx, as far as I can tell, #2370 went into your main branch, which is .NET 7.0.

The 6.0.2xx release is using your release/6.0 branch: https://github.com/dotnet/installer/blob/38a612150c53bbadffce67143067cabeab6e2f9c/eng/Version.Details.xml#L138-L143

[vitek-karas]

#2437 is the port of this to release/6.0.2xx branch.

We will have to change the flow to get the dotnet/linker/release/6.0.2xx branch be the one which is picked by the release/6.0.2xx branch in the SDK/installer repo.

[vitek-karas]

#2437 is now merged into 6.0.2xx and the flow to 6.0.200 SDK has been started.

[rolfbjarne]

@vitek-karas

#2437 is now merged into 6.0.2xx and the flow to 6.0.200 SDK has been started.

The linker commit mentioned here: https://github.com/dotnet/sdk/pull/23303/files (5b33e3a) is a commit from main, not the release/6.0.2xx branch. Is that intentional?

[vitek-karas]

@agocke - did we set up DARC correctly?

[agocke]

I think it was a commit from the 6.0.2xx branch, just with the wrong version number. Either way, the latest commit from SDK looks like it has the right version, and it was just merged a few hours ago: dotnet/sdk#23330