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.

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

Razor Static Web Assets feature is regressing build perf for large solutions #12933

Closed
dsplaisted opened this issue Aug 6, 2019 · 6 comments
Closed
Assignees
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed

Comments

@dsplaisted
Copy link
Member

We are seeing perf regressions for large solutions that target .NET Core 3.0 instead of .NET Core 2.0. We believe that the Razor static web assets feature is contributing to this. The implementation for the static web assets feature calls the GetCurrentProjectStaticWebAssets target in dependent projects with _StaticWebAssetsSkipDependencies=true as a global property:

https://github.com/aspnet/AspNetCore-Tooling/blob/2e70e0cbd7d8b952313d1114cd750befca2b1453/src/Razor/src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.StaticWebAssets.targets#L255-L264

Setting the new global property causes all of the transitively referenced projects to be re-evaluated, which can be expensive. You can see where we had a similar issue here: dotnet/msbuild#1276

@pranavkm @rynowak @mkArtakMSFT @rainersigwald

@mkArtakMSFT mkArtakMSFT added the area-blazor Includes: Blazor, Razor Components label Aug 7, 2019
@mkArtakMSFT mkArtakMSFT added the bug This issue describes a behavior which is not expected - a bug. label Aug 7, 2019
@mkArtakMSFT mkArtakMSFT added this to the 3.0.0-preview9 milestone Aug 7, 2019
@rainersigwald
Copy link
Member

@dsplaisted do you have a binlog of a build that suffers from this? I'd like to take a deeper look.

@dsplaisted
Copy link
Member Author

@rainersigwald I hit it when building the WebLarge30 performance test solution: https://github.com/dotnet/BuildPerformanceTestAssets/tree/master/PerformanceTestProjects/WebLarge30

However, that's quite a big solution (129 projects). You should be able to repro it with a vanilla 3.0 MVC project referencing a class library.

@javiercn
Copy link
Member

javiercn commented Aug 9, 2019

I'm going to be looking at this on Monday. The relevant piece of code is https://github.com/aspnet/AspNetCore-Tooling/blob/master/src/Razor/src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.StaticWebAssets.targets

We are using that property precisely to try and prevent executing on the dependencies :(. I think we need to do a small change in this feature to avoid passing the property. Would that fix it?

I investigated a bit and we'll likely can fix this by capturing the information the first time we compute it and having an additional target that reads it back when needed.

@mkArtakMSFT mkArtakMSFT added investigate and removed bug This issue describes a behavior which is not expected - a bug. labels Aug 9, 2019
@javiercn
Copy link
Member

dotnet/razor#958

@javiercn javiercn added bug This issue describes a behavior which is not expected - a bug. Working and removed investigate labels Aug 12, 2019
@javiercn
Copy link
Member

I have a PR out for this, gets it down from 11s to 3s and removes the use of the custom property.

@javiercn javiercn added Done This issue has been fixed and removed Working labels Aug 14, 2019
@javiercn
Copy link
Member

Fixed in dotnet/razor#958

@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2019
JunTaoLuo pushed a commit that referenced this issue May 7, 2020
…olving static web assets from referenced projects

* Removes the need for a custom property when resolving static web assets from referenced projects
* Rolls in several improvements suggested by the MSBuild folks to improve performance.
JunTaoLuo pushed a commit that referenced this issue May 7, 2020
* Give "builder" a less discoverable name

Fixes #12991

* Fixup tests

* [Blazor][Fixes #12933] Remove the need for a custom property when resolving static web assets from referenced projects

* Removes the need for a custom property when resolving static web assets from referenced projects
* Rolls in several improvements suggested by the MSBuild folks to improve performance.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

4 participants