-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
ResolvePackageFileConflicts performance enhancements #1805
Conversation
…semblyVersion. Allow for packages to override other packages by default.
<ItemGroup Condition="'$(DisableDefaultPackageConflictOverrides)' != 'true'"> | ||
<PackageConflictOverrides Include="Microsoft.NETCore.App"> | ||
<OverridenPackages> | ||
Microsoft.CSharp|4.4.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using a <=
check. So if Microsoft.CSharp 4.1.0
package is encountered, it still loses to Microsoft.NETCore.App
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -44,6 +55,7 @@ protected override void ExecuteCore() | |||
{ | |||
var log = new MSBuildLog(Log); | |||
var packageRanks = new PackageRank(PreferredPackages); | |||
var packageOverrides = new PackageOverrideResolver<ConflictItem>(PackageOverrides); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW - I've updated the original post with the perf numbers I'm seeing on my machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'm satisfied. We can tune the rest if it ever it shows up and this is clearly a nice win already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
System.Diagnostics.Process|4.3.0; | ||
System.Diagnostics.StackTrace|4.3.0; | ||
System.Diagnostics.TextWriterTraceListener|4.3.0; | ||
System.Diagnostics.Tools|4.3.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should ship any updates to this that way, but we need this to work on already shipped versions of these packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
int separatorIndex = trimmedOverridenPackagesAndVersion.IndexOf('|'); | ||
if (separatorIndex != -1) | ||
{ | ||
if (Version.TryParse(trimmedOverridenPackagesAndVersion.Substring(separatorIndex + 1), out Version version)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm missing something but don't we also need the assembly version and not just the package version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
<ItemGroup Condition="'$(SkipDefaultPackageConflictOverrides)' != 'true'"> | ||
<PackageConflictOverrides Include="Microsoft.NETCore.App"> | ||
<OverridenPackages> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this list twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. This is not needed.
I added unit tests. This change is ready for review. |
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - this change is unrelated to the package override work, but I saw roughly a 200ms gain in OrchardCore by making this change.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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