Speed up package asset resolution #1857

nguerrera · 2018-01-16T18:57:45Z

Raise only package assets that are used to build

The new task (ResolvePackageAssets) unlike the old ResolvePackageDependencies, only raises items that that will actually be consumed by the build. The new
items are also much easier to consume in targets and do not require "joins" to resolve paths, querying via WithMetadataValue, etc.

I'm seeing about 10% speed gain in single MVC project, and 15% in OrchardCore on incremental dotnet build (with restore). The gains when the binary log is enabled are more pronounced: about 25% with 4X improvement in working setmand 2X improvement in .binlog size + time to open in structured log viewer! Not to mention, that the log is much easier to interpret without all of the extra items.

There is a breaking change here:

If you use the old items from RunResolvePackageDependencies, then you need to depend-on/invoke that target. The code has not been deleted, it just won't be invoked if you don't use it. This is not a common sceanario.
There are also intermediate _Xxx ("private") items that have disappeared.
Duplicate/unnecessary metadata has been trimmed from items (e.g. NuGetPackageId vs PackageName).

cc @dsplaisted @livarcocc @mikeharder

mikeharder · 2018-01-18T02:11:14Z

I'm seeing approximately the same perf improvements:

Solution	Operation	7947	7947-ResolvePackageAssets	Improvement
WebSmall	BuildIncrementalSourceChangedLeaf	2.8	2.5	13%
WebLarge	BuildIncrementalSourceChangedLeaf	15.2	14.7	3%
OrchardCore	BuildIncrementalNoChange	22.5	18.9	16%

I suspect the improvement for WebLarge is relatively small because most of the projects are class libraries with no package references. I will add package references to the WebLarge class libraries to make it closer to a real-world solution (like OrchardCore).

nguerrera · 2018-01-19T18:46:37Z

Ready for review. I am still looking into adding some end-to-end tests for analyzers and content files because there don't seem to be any and they are impacted by this.

livarcocc · 2018-01-20T00:36:30Z

src/Tasks/Microsoft.NET.Build.Tasks/FindItemsFromPackages.cs

@@ -13,7 +13,7 @@ namespace Microsoft.NET.Build.Tasks
    /// set of Packages.
    /// </summary>
    /// <remarks>
-    /// Both Items and Packages are expected to have 'PackageName' and 'PackageVersion' 
+    /// Both Items and Packages are expected to have ('PackageName' and 'PackageVersion)' or ('NuGetPackageId' and 'NuGetPackageVersion')


What is the difference between PackageName and NuGetPackageId?

Different conventions were used in different places. Semantically, they are the same.

livarcocc · 2018-01-20T00:37:49Z

src/Tasks/Microsoft.NET.Build.Tasks/ProduceContentAssets.cs

@@ -251,18 +217,17 @@ private void ProduceContentAsset(ITaskItem contentFile)
                }
            }

-            if (contentFile.GetBooleanMetadata("copyToOutput") == true)
+            if (contentFile.GetBooleanMetadata("CopyToOutput") == true)


Don't you have a MetadataKey for this?

livarcocc · 2018-01-20T00:38:08Z

src/Tasks/Microsoft.NET.Build.Tasks/ProduceContentAssets.cs

            {
-                string outputPath = contentFile.GetMetadata("outputPath");
+                string outputPath = contentFile.GetMetadata("OutputPath");


livarcocc · 2018-01-20T00:38:21Z

src/Tasks/Microsoft.NET.Build.Tasks/ProduceContentAssets.cs

@@ -273,13 +238,12 @@ private void ProduceContentAsset(ITaskItem contentFile)
            }

            // TODO if build action is none do we even need to write the processed file above?
-            string buildAction = contentFile.GetMetadata("buildAction");
+            string buildAction = contentFile.GetMetadata("BuildAction");


nguerrera · 2018-01-20T00:43:44Z

Adding back WIP because I discovered a regression and don't want this to get merged.

dasMulli · 2018-01-21T19:49:10Z

on my system, https://github.com/dasMulli/cli-incremental-perf-testbed shows ~23% improvement in incremental builds - 24s down to 18.5s. (20 projects referencing Microsoft.AspNetCore.All and each other in a chain)

nguerrera · 2018-01-22T19:02:03Z

@dasMulli, cool. Just curious, does that test use the binary logger?

dsplaisted

I really really like these changes. It makes the asset resolution much easier to understand, it makes debugging with build logs much easier (since there's not so much junk), and on top of that it improves perf!

I reviewed this commit-by-commit, so some of my comments were addressed in later commits.

💯💯💯💯💯

dsplaisted · 2018-01-23T18:40:38Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

+            ContentFiles = RaisePackageAssets(
+                runtimeTarget,
+                p => p.ContentFiles,
+                filter: asset => !string.IsNullOrEmpty(asset.PPOutputPath),


Is this filter correct? Can't there be NuGet content items where the build action is None which are copied to the output directory, or which are EmbeddedResources and so shouldn't be preprocessed? If this filter is correct, then the check inside the setup delegate is redundant.

Oops. I forgot to loop back to this. The thing is that we only process the PPOutputPath files in the common case because nuget writes content items directly to generated props otherwise. There's a bool to override it though. I decided to revert the optimization, but now I'm thinking about adding it back. I need to add a content file test still.

dsplaisted · 2018-01-23T18:42:44Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

+                setup: (asset, item) =>
+                {
+                    string locale = asset.Properties["locale"];
+                    item.SetMetadata(MetadataKeys.Culture, "locale");


This seems wrong. Did you mean to set the metadata to the locale variable instead of the string literal "locale"? Is this consumed anywhere, and if not so should it even be added?

dsplaisted · 2018-01-23T18:44:20Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

+                setup: (asset, item) =>
+                {
+                    string directory = Path.GetDirectoryName(asset.Path);
+                    item.SetMetadata("DestinationSubDirectory", directory + Path.DirectorySeparatorChar);


Just to confirm, is asset.Path relative to the right directory here?

IE, it probably shouldn't start with "runtimes".

By design. It should start with runtimes.

This matches

sdk/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.ConflictResolution.targets

Line 48 in b0433de

DestinationSubPath="%(Path)" />

If you dotnet publish a portable app, the RID specific assets will go to a runtimes/ folder.

dsplaisted · 2018-01-23T18:59:54Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

+        {
+            var set = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+
+            foreach (var group in lockFile.ProjectFileDependencyGroups)


I would really like a definitive reference for the assets file format. It would make writing and understanding code like this a lot easier...

dsplaisted · 2018-01-23T19:08:56Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

+
+        private static string GetPackageNameFromDependency(string dependency)
+        {
+            int indexOfWhiteSpace = IndexOfWhiteSpace(dependency);


A comment about why we need to look for whitespace would be helpful here (ie the dependency value is of the format Microsoft.Build >= 15.1.0-dev).

dsplaisted · 2018-01-23T23:49:01Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.PackageDependencyResolution.targets

-          DependsOnTargets="_ComputeLockFileReferences;_ComputeLockFileFrameworks">
-
-    <ItemGroup>
+    <ItemGroup Condition="'$(MarkPackageReferencesAsExternallyResolved)' == 'true'">
      <!--
        Update Reference items with NuGetPackageId metadata to set ExternallyResolved appropriately.
        NetStandard.Library adds its assets in targets this way and not in the standard way that
        would get ExternallyResolved set in ResolvedCompileFileDefinitions below.


This comment is now out of date, the "ResolvedCompileFileDefinitions below" it refers to has been removed.

dsplaisted · 2018-01-24T00:02:25Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

@@ -36,6 +36,12 @@ public class ResolvePackageAssets : TaskBase

        public bool MarkPackageReferencesAsExternallyResolved { get; set; }

+        /// <summary>
+        /// Check that there is at least one package dependency in the RID graph that is not in the 


This looks like an incomplete comment. "Not in the" ...what?

dsplaisted · 2018-01-24T00:04:39Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

+        {
+            if (EnsureRuntimePackageDependencies && !string.IsNullOrEmpty(RuntimeIdentifier))
+            {
+                if (compileTimeTarget.Libraries.Count >= runtimeTarget.Libraries.Count)


I know you discussed this logic with me, but it's probably a good idea to get feedback from the CoreFx and NuGet folks to make sure it will work correctly.

cc @ericstj We discussed this approach.

dsplaisted · 2018-01-24T00:07:41Z

src/Tasks/Microsoft.NET.Build.Tasks/LockFileExtensions.cs

+
+        public static HashSet<string> GetProjectFileDependencySet(this LockFile lockFile)
+        {
+            string GetPackageNameFromDependency(string dependency)


I'd suggest a comment here explaining what the format of dependency is and thus why we're looking for the whitespace.

dsplaisted · 2018-01-24T00:09:20Z

src/Tasks/Microsoft.NET.Build.Tasks/LockFileExtensions.cs

+
+
+        // A package is a TransitiveProjectReference if it is a project, is not directly referenced,
+        // and does not contain a placeholder compile time assembly


What would cause there to be a project in the assets file that meets the other conditions but has a placeholder file?

I couldn't repro it. I talked to @emgarten and it has something to do with assets flags on project refs, but none that I tried caused it. This is just moved from elsewhere.

Example: `Returns="_ActiveTFMFileDependencies"` This returns the string `"_ActiveTFMFileDependencies"` not `@(_ActiveTFMFileDependencies)". Clearly these targets aren't actually run for their return values. The useless values are cluttering the binlog and confusing me.

The new task (ResolvePackageAssets) unlike the old ResolvePackageDependencies, only raises items that that will actually be consumed by the build. The new items are also much easier to consume in targets and do not require "joins" to resolve paths, querying via WithMetadataValue, etc.

Previously, Microsoft.NETCore.App package would do this in a custom target with the PackageDependency items that we no longer raise by default. Also, use repo-toolset to generate designer code for resx during build. There was a project system bug preventing it from getting generated in VS.

This fixes a bug introduced by dotnet/sdk#1857, which removes some metadata from this ItemGroup.

…4-7990bca4acf6 [main] Update dependencies from dotnet/roslyn

nguerrera force-pushed the fast-package-resolve branch from f40056b to 40ba317 Compare January 16, 2018 19:11

nguerrera mentioned this pull request Jan 16, 2018

Defend against forthcoming breaking change in SDK dotnet/cli#8409

Merged

nguerrera force-pushed the fast-package-resolve branch from 35af8b0 to 9467997 Compare January 19, 2018 01:45

nguerrera changed the title ~~[WIP] Speed up package asset resolution~~ Speed up package asset resolution Jan 19, 2018

nguerrera requested review from dsplaisted, livarcocc and a team January 19, 2018 18:45

livarcocc reviewed Jan 20, 2018

View reviewed changes

nguerrera changed the title ~~Speed up package asset resolution~~ WIP Speed up package asset resolution Jan 20, 2018

dsplaisted approved these changes Jan 24, 2018

View reviewed changes

This was referenced Jan 25, 2018

Incorrect publish output for netcoreapp1.1 on linux-x64 targets #1464

Closed

Issues with preprocessed nuget contentFile handling #1898

Open

nguerrera added 7 commits January 25, 2018 13:31

Remove unused generalization of dependency diagnostics

8076b8a

Share helper code between old and new package-resolution tasks

389d190

Add end-to-end analyzer resolution test

af9be11

Add preprocessed content file end-to-end test

430e180

nguerrera force-pushed the fast-package-resolve branch from 4eae2d7 to 430e180 Compare January 25, 2018 22:12

nguerrera changed the title ~~WIP Speed up package asset resolution~~ Speed up package asset resolution Jan 25, 2018

nguerrera merged commit f5db1af into dotnet:master Jan 25, 2018

nguerrera deleted the fast-package-resolve branch January 25, 2018 23:02

mikeharder mentioned this pull request Jan 31, 2018

_ComputeLockFileReferences/RunResolvePackageDependencies should be faster when projects are up-to-date #1116

Closed

nguerrera mentioned this pull request Jan 31, 2018

Insert latest dotnet/sdk dotnet/cli#8516

Merged

sbomer mentioned this pull request Mar 21, 2018

Fatal error in IL Linker with latest SDK dotnet/linker#286

Closed

This was referenced Mar 30, 2018

[Tracking] Overhead of SDK build targets is too high for a normal build #1534

Closed

Loading NuGet/JSON.NET binaries adds ~130ms to a build #1483

Closed

sbomer added a commit to sbomer/linker that referenced this pull request Jun 6, 2018

Don't use ResolvedPath metadata on ReferencePath

ab9d7b7

This fixes a bug introduced by dotnet/sdk#1857, which removes some metadata from this ItemGroup.

sbomer mentioned this pull request Jun 6, 2018

Don't use ResolvedPath metadata on ReferencePath dotnet/linker#324

Closed

nguerrera mentioned this pull request Jan 30, 2020

Remove EnsureNETCoreAppRuntime target from future Microsoft.NETCore.App releases dotnet/runtime#3011

Closed

JL03-Yue pushed a commit that referenced this pull request Mar 19, 2024

Merge pull request #1857 from dotnet/darc-main-70ce9e73-5dbb-472e-b02…

d02638c

…4-7990bca4acf6 [main] Update dependencies from dotnet/roslyn

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up package asset resolution #1857

Speed up package asset resolution #1857

nguerrera commented Jan 16, 2018 •

edited

Loading

mikeharder commented Jan 18, 2018

nguerrera commented Jan 19, 2018

livarcocc Jan 20, 2018

nguerrera Jan 20, 2018

livarcocc Jan 20, 2018

livarcocc Jan 20, 2018

livarcocc Jan 20, 2018

nguerrera commented Jan 20, 2018

dasMulli commented Jan 21, 2018

nguerrera commented Jan 22, 2018

dsplaisted left a comment

dsplaisted Jan 23, 2018

nguerrera Jan 24, 2018

nguerrera Jan 25, 2018

dsplaisted Jan 23, 2018

nguerrera Jan 25, 2018

dsplaisted Jan 23, 2018

nguerrera Jan 25, 2018

dsplaisted Jan 23, 2018

dsplaisted Jan 23, 2018

nguerrera Jan 25, 2018

dsplaisted Jan 23, 2018

dsplaisted Jan 24, 2018

nguerrera Jan 25, 2018

dsplaisted Jan 24, 2018

nguerrera Jan 24, 2018

dsplaisted Jan 24, 2018

dsplaisted Jan 24, 2018

nguerrera Jan 24, 2018



		// A package is a TransitiveProjectReference if it is a project, is not directly referenced,
		// and does not contain a placeholder compile time assembly

Speed up package asset resolution #1857

Speed up package asset resolution #1857

Conversation

nguerrera commented Jan 16, 2018 • edited Loading

mikeharder commented Jan 18, 2018

nguerrera commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera commented Jan 20, 2018

dasMulli commented Jan 21, 2018

nguerrera commented Jan 22, 2018

dsplaisted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera commented Jan 16, 2018 •

edited

Loading