-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
ResolveAssemblyReference is slow on .NET Core with many references #2015
Comments
One thing of note: MSBuild only calls that |
To add a little more detail we really have a project that's taking this long to build (referring to the 95sec spent in You may want to check out https://github.com/aspnet/Mvc/tree/dev/test/Microsoft.AspNetCore.Mvc.FunctionalTests in general as example of a really complicated dotnet core package that's updated to the bleeding edge. |
AssemblyName.Clone is not available in early versions of .NET Core. Until we can upgrade MSBuild to use .NET Core 2.0, we can't call the Clone method directly. This change will attempt to call .Clone using reflection and if not available fallback to the previous .NET Core behavior. Potential fix for dotnet#2015 (when running on .NET Core 2.0).
AssemblyName.Clone is not available in early versions of .NET Core. Until we can upgrade MSBuild to use .NET Core 2.0, we can't call the Clone method directly. This change will attempt to call .Clone using reflection and if not available fallback to the previous .NET Core behavior. Potential fix for dotnet#2015 (when running on .NET Core 2.0).
AssemblyName.Clone is not available in early versions of .NET Core. Until we can upgrade MSBuild to use .NET Core 2.0, we can't call the Clone method directly. This change will attempt to call .Clone using reflection and if not available fallback to the previous .NET Core behavior. Potential fix for dotnet#2015 (when running on .NET Core 2.0).
AssemblyName.Clone is not available in early versions of .NET Core. Until we can upgrade MSBuild to use .NET Core 2.0, we can't call the Clone method directly. This change will attempt to call .Clone using reflection and if not available fallback to the previous .NET Core behavior. Potential fix for dotnet#2015 (when running on .NET Core 2.0).
AssemblyName.Clone is not available in early versions of .NET Core. Until we can upgrade MSBuild to use .NET Core 2.0, we can't call the Clone method directly. This change will attempt to call .Clone using reflection and if not available fallback to the previous .NET Core behavior. Potential fix for dotnet#2015 (when running on .NET Core 2.0).
AssemblyName.Clone() is not available in early versions of .NET Core. Until we can upgrade MSBuild to use .NET Core 2.0, we can't call the Clone method directly. This change will attempt to call .Clone using reflection and if not available fallback to the previous .NET Core behavior. Potential fix for #2015 (when running on .NET Core 2.0).
AssemblyName.Clone() is not available in early versions of .NET Core. Until we can upgrade MSBuild to use .NET Core 2.0, we can't call the Clone method directly. This change will attempt to call .Clone using reflection and if not available fallback to the previous .NET Core behavior. Potential fix for #2015 (when running on .NET Core 2.0).
There's an ask from the ASP.NET team to not even run ResolveAssemblyReference in some cases. See dotnet/sdk#1193 |
I think you should look at a couple options:
I think #2 is doable if we look at the right set of inputs/outputs and potentially change AssemblySearchPaths for some project types. |
There is a cache in RAR but we can't use it in Core because it depends on BinarySerialization which isn't accessible to us. I'm fairly hesitant to build any assumptions about nuget immutability into RAR. |
Isn't binary serialization available in |
@DamianEdwards Yes, but we're not targeting that. We'd like to but it's a lot of work and we can't do it for 15.3. |
@rainersigwald can you not light-up on it in the same way you did for the change above? Or is not really factored to enable that easily right now? |
@DamianEdwards I don't think so, because binary serialization requires source attributes, which can't be discovered via reflection. |
What other serialization options do we have? |
I'm going to spend tomorrow on some prototyping and profiling on this. |
@rainersigwald thanks |
@DamianEdwards what's the best scenario to hammer on? A
|
@rainersigwald try 1st build
2nd build
3rd build
|
Or use this project if you want a really fun one 😆 https://github.com/aspnet/Mvc/tree/dev/test/Microsoft.AspNetCore.Mvc.FunctionalTests |
Here's what I've figured out so far:
There are a couple of options to shorten this:
I spoke to a source who was involved with RAR in the past (you can't escape if you're still in DevDiv, but I can keep your name out of the press!) who thought that the best option might be to teach the NuGet-resolution targets to emit directly to the items that are RAR outputs (like RAR does basically three things, none of which are super interesting for NuGet references:
We can't eliminate RAR altogether, because when targeting a full framework TFM you can use references that don't come from NuGet (like I prototyped this with an injected task: <Target Name="AdjustRAR" BeforeTargets="ResolveAssemblyReferences" Condition="'$(AdjustRAR)' != 'false'">
<ItemGroup>
<ReferencePath Include="@(Reference)" Condition="'%(NuGetSourceType)' == 'Package'" />
<Reference Remove="@(Reference)" Condition="'%(NuGetSourceType)' == 'Package'" />
</ItemGroup>
</Target> For an MVC template project, that completely eliminates RAR:
Ok, so great. What are the risks here? We need to sync with the SDK and NuGet teams about:
|
@rainersigwald great info. That number of assemblies is much higher than expected (by about 67%). Could you share the list? |
Also, you say:
Could we eliminate it in the cases the project is not targeting a full framework TFM then? |
I think if NuGet implemented NuGet/Home#7617, some global targets provided by them could hook into that and do that per-nuget immutability caching/optimization at restore time, ideally only once, without having MSBuild know anything about how that's done. For example, they could emit additional It would also allow package authors to extend the same mechanism, as explained in that issue. |
@kzu I don't think I'm seeing the connection between that comment and RAR performance; can you elaborate? |
@rainersigwald basically, do what was suggested only once and at restore time:
;). That part could leverage the same extension point for |
I see. I don't think that's a viable option here because RAR needs to see all references in order to do version unification and generate binding redirects, and the inputs to that could change outside of Restore (via |
#3868 has a good perf improvement in RAR cache serialization, but we're not sure the Bond serialization is the best approach. We should consider reviving it at some point in the future. |
What are the downsides of it? Have you explored other alternatives? |
@ladipro, heads up on this one. We should link it to our RAR user story. |
I just collected some data from a dotnet/runtime libraries build with ~1400 evaluations (outer + inner builds). I crossed out the tasks that are dotnet/runtime specific (a fix for ValidatePackage no-op build times is tracked via dotnet/sdk#23517). Assuming RAR only runs in inner-builds, this should represent the time it takes RAR to serve ~1100 inner builds. I was looking into speeding up incremental builds in dotnet/runtime libs and I believe the highest impact contributors on the msbuild side are:
23% are dotnet/runtime specific and must be fixed by ourselves. Just wanted to share this data in case it helps. |
ResolveAssemblyReferences
runs unconditionally even on incremental builds (because the user may have installed a targeting pack or, for full-framework scenarios, GACed an assembly). This makes it performance-sensitive.With the advent of .NET Core micro-assemblies, it has become common to pass many, many references to RAR. This exacerbates performance problems.
@rynowak was kind enough to run a profiler to see this:
The text was updated successfully, but these errors were encountered: