ResolvePackageFileConflicts performance enhancements #1805

eerhardt · 2017-12-08T05:04:44Z

Speed up ResolvePackageFileConflicts by avoiding reading files for their AssemblyVersion.
Allow for packages to override other packages by default.

Here are my timings before and after my changes on my win-x64 machine. Each scenario was fully built, and then a single "warm up" command, followed by 3 captured executions of

C:\temp\dotnet\dotnet\dotnet.exe C:\temp\dotnet\dotnet\sdk\2.2.0-preview1-007739\MSBuild.dll /v:m /m /clp:PerformanceSummary

Scenario 1 https://github.com/OrchardCMS/OrchardCore

Without changes	With changes
2667 ms ResolvePackageFileConflicts 129 calls	609 ms ResolvePackageFileConflicts 129 calls
2630 ms ResolvePackageFileConflicts 129 calls	585 ms ResolvePackageFileConflicts 129 calls
2418 ms ResolvePackageFileConflicts 129 calls	598 ms ResolvePackageFileConflicts 129 calls

Scenario 2 https://github.com/mikeharder/dotnet-cli-perf/tree/8d7493b26fd3a1b3d1ba3fb85fc7e60b0c19618e/scenarios/classlib/core

Without changes	With changes
45 ms ResolvePackageFileConflicts 2 calls	29 ms ResolvePackageFileConflicts 2 calls
46 ms ResolvePackageFileConflicts 2 calls	29 ms ResolvePackageFileConflicts 2 calls
45 ms ResolvePackageFileConflicts 2 calls	31 ms ResolvePackageFileConflicts 2 calls

Scenario 3 https://github.com/mikeharder/dotnet-cli-perf/tree/8d7493b26fd3a1b3d1ba3fb85fc7e60b0c19618e/scenarios/web/core

Without changes	With changes
78 ms ResolvePackageFileConflicts 1 calls	21 ms ResolvePackageFileConflicts 1 calls
74 ms ResolvePackageFileConflicts 1 calls	21 ms ResolvePackageFileConflicts 1 calls
56 ms ResolvePackageFileConflicts 1 calls	21 ms ResolvePackageFileConflicts 1 calls

…semblyVersion. Allow for packages to override other packages by default.

eerhardt · 2017-12-08T05:08:20Z

src/Tasks/Common/build/Microsoft.NET.DefaultPackageConflictOverrides.targets

+  <ItemGroup Condition="'$(DisableDefaultPackageConflictOverrides)' != 'true'">
+    <PackageConflictOverrides Include="Microsoft.NETCore.App">
+      <OverridenPackages>
+        Microsoft.CSharp|4.4.0;


@weshaggard - is there a list I can compare these packages to for both NETCore.App and NETStandard? The tricky thing is which OOB packages need versions > 4.3.0.

Do what just the latest versions? It would seem to make this a useful cache you would need all versions.

As for the list of them I think the best you can do is to provide the latest version of everything that lives in Microsoft.NETCore.App and lookup their latest version on nuget.org.

We are using a <= check. So if Microsoft.CSharp 4.1.0 package is encountered, it still loses to Microsoft.NETCore.App.

We need to be a little careful about that type of logic in servicing type of scenarios. During servicing we will have a 4.x.x that we might actually need to override what is in Microsoft.NETCore.App.

Concretely, you mean we might ship a 4.3.1 Microsoft.CSharp that should override the 4.4.0 version inside Microsoft.NETCore.App 2.0.0?

I guess thinking about it a little more I think it is pretty unlikely to ship something that overrides the inbox Microsoft.NETCore.App from anything other then the release/2.0.0 branch (i.e. 4.4.x). So we need to at least ensure that these versions are somehow tied to a version of Microsoft.NETCore.App as well. As in we may very well want the 4.4.1 (if and when it ships) version of Microsoft.Csharp to win a conflict over the 2.0.0 Microsoft.NETCore.App but not for the 2.0.x and greater version that contains it.

I'm not sure how this affects, if at all, Microsoft.NETCore.App < 2.0.0, but we need to be sure that this doesn't start to throw out packages there.

My thinking is that these versions in the SDK won't change going forward. They are the "baseline".

We can do the following going forward in future versions of Microsoft.NETCore.App and/or NETStandard.Library:

Provide higher version numbers to the existing packages, and add new packages. see MergePackageOverrides

Set DisableDefaultPackageConflictOverrides=true and provide a whole new list of these packages and versions. Or set that boolean to turn this functionality off all together.

Choose to not update these packages/versions in Microsoft.NETCore.App or NETStandard.Library. This won't cause any correctness issues, since this new check is only a performance optimization. If a higher Microsoft.CSharp package (say 4.4.1) conflicts with a future version of Microsoft.NETCore.App, then the existing AssemblyVersion and FileVersion checks will kick in and do the right thing. They will just happen to be a bit slower than these NuGet version checks.

I'm choosing to go with the last option for now, until we have data that proves we need to add those future versions to Microsoft.NETCore.App and/or NETStandard.Library. >90% of the conflicts are caused by packages that we aren't shipping new versions for (System.Runtime/Console/Collections/IO/etc). So even if we don't keep the OOB packages up-to-date, we still get a big perf win here.

That makes sense to me.

I'm wondering if there can/should be a test to validate that all of these overrides behave the same as the fallback assemblyversion/fileversion checks. I'm imagining a test that generates a set of package references from the overrides and then we build with and it without DisableDefaultPackageConflictOverrides and check that we get the same @(Reference) and @(ReferenceCopyLocalPaths) both ways. You can get the overrides and the reference lists with GetValuesCommand.

I'm not sure how this affects, if at all, Microsoft.NETCore.App < 2.0.

I would think that none of the assets out of these packages would ever conflict with Microsoft.NETCore.App or NETStandard.Library < 2.0.0 and so there would be no impact there. Right?

Since < 2.0 were packages references nuget should unify the packages and so there shouldn't be a conflict but I not sure if there is any logic to always compare to what is part of Microsoft.NETCore.App so thought I would ask the question just to be sure others think about it as well.

nguerrera · 2017-12-08T06:59:14Z

src/Tasks/Common/ConflictResolution/ResolvePackageFileConflicts.cs

@@ -44,6 +55,7 @@ protected override void ExecuteCore()
        {
            var log = new MSBuildLog(Log);
            var packageRanks = new PackageRank(PreferredPackages);
+            var packageOverrides = new PackageOverrideResolver<ConflictItem>(PackageOverrides);


How much perf did you give back by creating this every time the task runs? I thought the version with a hard coded structure created every time was significantly slower than a static list.

The parsing logic looks very heavy on allocations: split, trim, tuple, yield.

My original mistake was doing the data creation in the ConflictResolver constructor, which is called 3x per invocation of the task. So that's why it came up when I profiled at that time (it was 3x what it needed to be).

I did some profiling of my current approach, and this creation takes <10% of the time of the new task. The real time left in the task (according to PerfView) is getting Item Metadata multiple times for every ConflictItem. This accounts for ~60% of the time of the new task according to the profiles I've looked at.

BTW - I've updated the original post with the perf numbers I'm seeing on my machine.

OK, I'm satisfied. We can tune the rest if it ever it shows up and this is clearly a nice win already.

You made my realize that I should be lazy-loading this data for the case where there are no conflicts. No reason to build the dictionary up, until we need it. I've pushed a change for that.

weshaggard · 2017-12-08T16:54:06Z

src/Tasks/Common/build/Microsoft.NET.DefaultPackageConflictOverrides.targets

+        System.Diagnostics.Process|4.3.0;
+        System.Diagnostics.StackTrace|4.3.0;
+        System.Diagnostics.TextWriterTraceListener|4.3.0;
+        System.Diagnostics.Tools|4.3.0;


Wouldn't a better place for this list be in Microsoft.NETCore.App itself? In fact we already provide the assembly list we should just be able to provide extra metadata for the version.

I think we should ship any updates to this that way, but we need this to work on already shipped versions of these packages.

Yes, it is definitely a better place, but we have already shipped Microsoft.NETCore.App 2.0.0 and NETStandard.Library 2.0.0. Thus we can't add this data to those packages.

This change allows data to come from those packages in the future, and that new data will either augment this (see MergePackageOverrides), or the future packages can turn the default data off all together by setting DisableDefaultPackageConflictOverrides and just provide its own data.

weshaggard · 2017-12-08T16:56:36Z

src/Tasks/Common/src/ConflictResolution/PackageOverride.cs

+                    int separatorIndex = trimmedOverridenPackagesAndVersion.IndexOf('|');
+                    if (separatorIndex != -1)
+                    {
+                        if (Version.TryParse(trimmedOverridenPackagesAndVersion.Substring(separatorIndex + 1), out Version version))


Perhaps I'm missing something but don't we also need the assembly version and not just the package version?

We are already doing assembly version comparisons (which is the thing causing the performance problems - getting the assembly version).

This new approach is providing the NuGet versions, and a list of packages and versions that Microsoft.NETCore.App and NETStandard.Library override by default. That means we can read these NuGet versions and resolve conflicts before needing to read the AssemblyVersions.

weshaggard · 2017-12-08T16:57:48Z

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

+
+    <ItemGroup Condition="'$(SkipDefaultPackageConflictOverrides)' != 'true'">
+      <PackageConflictOverrides Include="Microsoft.NETCore.App">
+        <OverridenPackages>


Why do we need this list twice?

Oops. This is not needed.

…cts, we don't need to build it.

eerhardt · 2017-12-09T23:32:27Z

I added unit tests. This change is ready for review.

eerhardt · 2017-12-09T23:34:06Z

src/Tasks/Common/ConflictResolution/ConflictItem.cs

@@ -103,7 +108,7 @@ public string FileName
            {
                if (_fileName == null)
                {
-                    _fileName = OriginalItem == null ? String.Empty : OriginalItem.GetMetadata(MetadataNames.FileName) + OriginalItem.GetMetadata(MetadataNames.Extension);
+                    _fileName = OriginalItem == null ? String.Empty : Path.GetFileName(OriginalItem.ItemSpec);


FYI - this change is unrelated to the package override work, but I saw roughly a 200ms gain in OrchardCore by making this change.

nguerrera · 2017-12-12T00:32:37Z

src/Tasks/Common/ConflictResolution/ConflictItem.cs

@@ -24,6 +24,11 @@ internal interface IConflictItem
        Version FileVersion { get; }
        string PackageId { get; }
        string DisplayName { get; }
+
+        // NOTE: Technically this should be NuGetVersion because System.Version doesn't work with symver.


typo: s/symver/semver/

- fix typo - Add functional test to ensure the conflict resolution optimizations results in the same References. - Adding that test found 3 more packages to add to the default list of package overrides.

nguerrera

Speed up ResolvePackageFileConflicts by avoiding reading files for As…

dd07581

…semblyVersion. Allow for packages to override other packages by default.

eerhardt requested review from dsplaisted, nguerrera and ericstj December 8, 2017 05:04

eerhardt commented Dec 8, 2017

View reviewed changes

nguerrera reviewed Dec 8, 2017

View reviewed changes

weshaggard reviewed Dec 8, 2017

View reviewed changes

eerhardt added 4 commits December 8, 2017 14:28

Move new ConflictResolution files to the new directory.

8e818fd

Lazy load the package override dictionary so when there are no confli…

ed7cf09

…cts, we don't need to build it.

Remove unnecessary copy of PackageConflictOverrides.

2b05809

Add unit tests for PackageOverrideResolver.

28e3268

eerhardt commented Dec 9, 2017

View reviewed changes

nguerrera reviewed Dec 12, 2017

View reviewed changes

nguerrera approved these changes Dec 12, 2017

View reviewed changes

PR Feedback

f6a3f88

- fix typo - Add functional test to ensure the conflict resolution optimizations results in the same References. - Adding that test found 3 more packages to add to the default list of package overrides.

nguerrera approved these changes Dec 14, 2017

View reviewed changes

eerhardt merged commit 0825b2f into dotnet:master Dec 14, 2017

eerhardt deleted the FixConflictResolutionPerf branch December 14, 2017 00:32

nguerrera mentioned this pull request Jan 11, 2018

Fix spelling of 'overridden' #1852

Merged

nguerrera mentioned this pull request Mar 30, 2018

ConflictResolver.ResolveConflict is ~3.3% of all allocations in solution-wide design-time build #1604

Closed

eerhardt mentioned this pull request Jan 30, 2020

Consider generating PackageConflictOverrides in Microsoft.NETCore.App dotnet/runtime#3010

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ResolvePackageFileConflicts performance enhancements #1805

ResolvePackageFileConflicts performance enhancements #1805

eerhardt commented Dec 8, 2017 •

edited

Loading

eerhardt Dec 8, 2017

weshaggard Dec 8, 2017

eerhardt Dec 8, 2017

weshaggard Dec 11, 2017

eerhardt Dec 11, 2017

weshaggard Dec 11, 2017

eerhardt Dec 11, 2017

nguerrera Dec 12, 2017

nguerrera Dec 12, 2017

weshaggard Dec 12, 2017

nguerrera Dec 8, 2017

eerhardt Dec 8, 2017

eerhardt Dec 8, 2017

nguerrera Dec 8, 2017

eerhardt Dec 8, 2017

weshaggard Dec 8, 2017

nguerrera Dec 8, 2017

eerhardt Dec 8, 2017

weshaggard Dec 8, 2017

eerhardt Dec 8, 2017

weshaggard Dec 8, 2017

eerhardt Dec 8, 2017

eerhardt commented Dec 9, 2017

eerhardt Dec 9, 2017

nguerrera Dec 12, 2017

nguerrera left a comment

ResolvePackageFileConflicts performance enhancements #1805

ResolvePackageFileConflicts performance enhancements #1805

Conversation

eerhardt commented Dec 8, 2017 • edited Loading

Scenario 1 https://github.com/OrchardCMS/OrchardCore

Scenario 2 https://github.com/mikeharder/dotnet-cli-perf/tree/8d7493b26fd3a1b3d1ba3fb85fc7e60b0c19618e/scenarios/classlib/core

Scenario 3 https://github.com/mikeharder/dotnet-cli-perf/tree/8d7493b26fd3a1b3d1ba3fb85fc7e60b0c19618e/scenarios/web/core

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Dec 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera left a comment

Choose a reason for hiding this comment

eerhardt commented Dec 8, 2017 •

edited

Loading