-
Notifications
You must be signed in to change notification settings - Fork 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
Changes from 1 commit
dd07581
8e818fd
ed7cf09
2b05809
28e3268
f6a3f88
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line 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. | ||
// However, the only scenarios we need to support this property for in conflict resolution is stable versions | ||
// of System packages. PackageVersion will be null if System.Version can't parse the version (i.e. if is pre-release) | ||
Version PackageVersion { get; } | ||
} | ||
|
||
// Wraps an ITask item and adds lazy evaluated properties used by Conflict resolution. | ||
|
@@ -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 commentThe 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. |
||
} | ||
return _fileName; | ||
} | ||
|
@@ -165,7 +170,31 @@ public string PackageId | |
} | ||
private set { _packageId = value; } | ||
} | ||
|
||
|
||
private bool _hasPackageVersion; | ||
private Version _packageVersion; | ||
public Version PackageVersion | ||
{ | ||
get | ||
{ | ||
if (!_hasPackageVersion) | ||
{ | ||
_packageVersion = null; | ||
|
||
var packageVersionString = OriginalItem?.GetMetadata(nameof(MetadataNames.NuGetPackageVersion)) ?? String.Empty; | ||
|
||
if (packageVersionString.Length != 0) | ||
{ | ||
Version.TryParse(packageVersionString, out _packageVersion); | ||
} | ||
|
||
// PackageVersion may be null but don't try to recalculate it | ||
_hasPackageVersion = true; | ||
} | ||
|
||
return _packageVersion; | ||
} | ||
} | ||
|
||
private string _sourcePath; | ||
public string SourcePath | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,17 @@ public class ResolvePackageFileConflicts : TaskBase | |
/// </summary> | ||
public string[] PreferredPackages { get; set; } | ||
|
||
/// <summary> | ||
/// A collection of items that contain information of which packages get overriden | ||
/// by which packages before doing any other conflict resolution. | ||
/// </summary> | ||
/// <remarks> | ||
/// This is an optimizaiton so AssemblyVersions, FileVersions, etc. don't need to be read | ||
/// in the default cases where platform packages (Microsoft.NETCore.App) should override specific packages | ||
/// (System.Console v4.3.0). | ||
/// </remarks> | ||
public ITaskItem[] PackageOverrides { get; set; } | ||
|
||
[Output] | ||
public ITaskItem[] ReferencesWithoutConflicts { get; set; } | ||
|
||
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. My original mistake was doing the data creation in the I did some profiling of my current approach, and this creation takes There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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. |
||
|
||
// Treat assemblies from FrameworkList.xml as platform assemblies that also get considered at compile time | ||
IEnumerable<ConflictItem> compilePlatformItems = null; | ||
|
@@ -60,7 +72,7 @@ protected override void ExecuteCore() | |
// resolve conflicts at compile time | ||
var referenceItems = GetConflictTaskItems(References, ConflictItemType.Reference).ToArray(); | ||
|
||
var compileConflictScope = new ConflictResolver<ConflictItem>(packageRanks, log); | ||
var compileConflictScope = new ConflictResolver<ConflictItem>(packageRanks, packageOverrides, log); | ||
|
||
compileConflictScope.ResolveConflicts(referenceItems, | ||
ci => ItemUtilities.GetReferenceFileName(ci.OriginalItem), | ||
|
@@ -74,7 +86,7 @@ protected override void ExecuteCore() | |
} | ||
|
||
// resolve conflicts that class in output | ||
var runtimeConflictScope = new ConflictResolver<ConflictItem>(packageRanks, log); | ||
var runtimeConflictScope = new ConflictResolver<ConflictItem>(packageRanks, packageOverrides, log); | ||
|
||
runtimeConflictScope.ResolveConflicts(referenceItems, | ||
ci => ItemUtilities.GetReferenceTargetPath(ci.OriginalItem), | ||
|
@@ -95,7 +107,7 @@ protected override void ExecuteCore() | |
|
||
// resolve conflicts with platform (eg: shared framework) items | ||
// we only commit the platform items since its not a conflict if other items share the same filename. | ||
var platformConflictScope = new ConflictResolver<ConflictItem>(packageRanks, log); | ||
var platformConflictScope = new ConflictResolver<ConflictItem>(packageRanks, packageOverrides, log); | ||
var platformItems = PlatformManifests?.SelectMany(pm => PlatformManifestReader.LoadConflictItems(pm.ItemSpec, log)) ?? Enumerable.Empty<ConflictItem>(); | ||
|
||
if (compilePlatformItems != null) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,233 @@ | ||
<!-- | ||
*********************************************************************************************** | ||
Microsoft.NET.DefaultPackageConflictOverrides.targets | ||
|
||
WARNING: DO NOT MODIFY this file unless you are knowledgeable about MSBuild and have | ||
created a backup copy. Incorrect changes to this file will make it | ||
impossible to load or build your projects from the command-line or the IDE. | ||
|
||
Copyright (c) .NET Foundation. All rights reserved. | ||
*********************************************************************************************** | ||
--> | ||
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
|
||
<PropertyGroup> | ||
<MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects> | ||
</PropertyGroup> | ||
|
||
<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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. We are using a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Concretely, you mean we might ship a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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. |
||
Microsoft.Win32.Primitives|4.3.0; | ||
Microsoft.Win32.Registry|4.4.0; | ||
runtime.debian.8-x64.runtime.native.System.Security.Cryptography.OpenSsl|4.3.0; | ||
runtime.fedora.23-x64.runtime.native.System.Security.Cryptography.OpenSsl|4.3.0; | ||
runtime.fedora.24-x64.runtime.native.System.Security.Cryptography.OpenSsl|4.3.0; | ||
runtime.opensuse.13.2-x64.runtime.native.System.Security.Cryptography.OpenSsl|4.3.0; | ||
runtime.opensuse.42.1-x64.runtime.native.System.Security.Cryptography.OpenSsl|4.3.0; | ||
runtime.osx.10.10-x64.runtime.native.System.Security.Cryptography.Apple|4.3.0; | ||
runtime.osx.10.10-x64.runtime.native.System.Security.Cryptography.OpenSsl|4.3.0; | ||
runtime.rhel.7-x64.runtime.native.System.Security.Cryptography.OpenSsl|4.3.0; | ||
runtime.ubuntu.14.04-x64.runtime.native.System.Security.Cryptography.OpenSsl|4.3.0; | ||
runtime.ubuntu.16.04-x64.runtime.native.System.Security.Cryptography.OpenSsl|4.3.0; | ||
runtime.ubuntu.16.10-x64.runtime.native.System.Security.Cryptography.OpenSsl|4.3.0; | ||
System.AppContext|4.3.0; | ||
System.Collections|4.3.0; | ||
System.Collections.Concurrent|4.3.0; | ||
System.Collections.Immutable|1.4.0; | ||
System.Collections.NonGeneric|4.3.0; | ||
System.Collections.Specialized|4.3.0; | ||
System.ComponentModel|4.3.0; | ||
System.ComponentModel.EventBasedAsync|4.3.0; | ||
System.ComponentModel.Primitives|4.3.0; | ||
System.ComponentModel.TypeConverter|4.3.0; | ||
System.Console|4.3.0; | ||
System.Data.Common|4.3.0; | ||
System.Diagnostics.Contracts|4.3.0; | ||
System.Diagnostics.Debug|4.3.0; | ||
System.Diagnostics.FileVersionInfo|4.3.0; | ||
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 commentThe 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 commentThe 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 commentThe 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 |
||
System.Diagnostics.TraceSource|4.3.0; | ||
System.Diagnostics.Tracing|4.3.0; | ||
System.Dynamic.Runtime|4.3.0; | ||
System.Globalization|4.3.0; | ||
System.Globalization.Extensions|4.3.0; | ||
System.IO|4.3.0; | ||
System.IO.Compression|4.3.0; | ||
System.IO.Compression.ZipFile|4.3.0; | ||
System.IO.FileSystem|4.3.0; | ||
System.IO.FileSystem.AccessControl|4.4.0; | ||
System.IO.FileSystem.DriveInfo|4.3.0; | ||
System.IO.FileSystem.Primitives|4.3.0; | ||
System.IO.FileSystem.Watcher|4.3.0; | ||
System.IO.IsolatedStorage|4.3.0; | ||
System.IO.MemoryMappedFiles|4.3.0; | ||
System.IO.Pipes|4.3.0; | ||
System.IO.UnmanagedMemoryStream|4.3.0; | ||
System.Linq|4.3.0; | ||
System.Linq.Expressions|4.3.0; | ||
System.Linq.Queryable|4.3.0; | ||
System.Net.Http|4.3.0; | ||
System.Net.NameResolution|4.3.0; | ||
System.Net.Primitives|4.3.0; | ||
System.Net.Requests|4.3.0; | ||
System.Net.Security|4.3.0; | ||
System.Net.Sockets|4.3.0; | ||
System.Net.WebHeaderCollection|4.3.0; | ||
System.ObjectModel|4.3.0; | ||
System.Private.DataContractSerialization|4.3.0; | ||
System.Reflection|4.3.0; | ||
System.Reflection.Emit|4.3.0; | ||
System.Reflection.Emit.ILGeneration|4.3.0; | ||
System.Reflection.Emit.Lightweight|4.3.0; | ||
System.Reflection.Extensions|4.3.0; | ||
System.Reflection.Metadata|1.5.0; | ||
System.Reflection.Primitives|4.3.0; | ||
System.Reflection.TypeExtensions|4.3.0; | ||
System.Resources.ResourceManager|4.3.0; | ||
System.Runtime|4.3.0; | ||
System.Runtime.Extensions|4.3.0; | ||
System.Runtime.Handles|4.3.0; | ||
System.Runtime.InteropServices|4.3.0; | ||
System.Runtime.InteropServices.RuntimeInformation|4.3.0; | ||
System.Runtime.Loader|4.3.0; | ||
System.Runtime.Numerics|4.3.0; | ||
System.Runtime.Serialization.Formatters|4.3.0; | ||
System.Runtime.Serialization.Json|4.3.0; | ||
System.Runtime.Serialization.Primitives|4.3.0; | ||
System.Security.AccessControl|4.4.0; | ||
System.Security.Claims|4.3.0; | ||
System.Security.Cryptography.Algorithms|4.3.0; | ||
System.Security.Cryptography.Cng|4.4.0; | ||
System.Security.Cryptography.Csp|4.3.0; | ||
System.Security.Cryptography.Encoding|4.3.0; | ||
System.Security.Cryptography.OpenSsl|4.4.0; | ||
System.Security.Cryptography.Primitives|4.3.0; | ||
System.Security.Cryptography.X509Certificates|4.3.0; | ||
System.Security.Cryptography.Xml|4.4.0; | ||
System.Security.Principal|4.3.0; | ||
System.Security.Principal.Windows|4.4.0; | ||
System.Text.Encoding|4.3.0; | ||
System.Text.Encoding.Extensions|4.3.0; | ||
System.Text.RegularExpressions|4.3.0; | ||
System.Threading|4.3.0; | ||
System.Threading.Overlapped|4.3.0; | ||
System.Threading.Tasks|4.3.0; | ||
System.Threading.Tasks.Extensions|4.3.0; | ||
System.Threading.Tasks.Parallel|4.3.0; | ||
System.Threading.Thread|4.3.0; | ||
System.Threading.ThreadPool|4.3.0; | ||
System.Threading.Timer|4.3.0; | ||
System.ValueTuple|4.3.0; | ||
System.Xml.ReaderWriter|4.3.0; | ||
System.Xml.XDocument|4.3.0; | ||
System.Xml.XmlDocument|4.3.0; | ||
System.Xml.XmlSerializer|4.3.0; | ||
System.Xml.XPath|4.3.0; | ||
System.Xml.XPath.XDocument|4.3.0; | ||
</OverridenPackages> | ||
</PackageConflictOverrides> | ||
<PackageConflictOverrides Include="NETStandard.Library"> | ||
<OverridenPackages> | ||
Microsoft.Win32.Primitives|4.3.0; | ||
System.AppContext|4.3.0; | ||
System.Collections|4.3.0; | ||
System.Collections.Concurrent|4.3.0; | ||
System.Collections.Immutable|1.4.0; | ||
System.Collections.NonGeneric|4.3.0; | ||
System.Collections.Specialized|4.3.0; | ||
System.ComponentModel|4.3.0; | ||
System.ComponentModel.EventBasedAsync|4.3.0; | ||
System.ComponentModel.Primitives|4.3.0; | ||
System.ComponentModel.TypeConverter|4.3.0; | ||
System.Console|4.3.0; | ||
System.Data.Common|4.3.0; | ||
System.Diagnostics.Contracts|4.3.0; | ||
System.Diagnostics.Debug|4.3.0; | ||
System.Diagnostics.FileVersionInfo|4.3.0; | ||
System.Diagnostics.Process|4.3.0; | ||
System.Diagnostics.StackTrace|4.3.0; | ||
System.Diagnostics.TextWriterTraceListener|4.3.0; | ||
System.Diagnostics.Tools|4.3.0; | ||
System.Diagnostics.TraceSource|4.3.0; | ||
System.Diagnostics.Tracing|4.3.0; | ||
System.Dynamic.Runtime|4.3.0; | ||
System.Globalization|4.3.0; | ||
System.Globalization.Extensions|4.3.0; | ||
System.IO|4.3.0; | ||
System.IO.Compression|4.3.0; | ||
System.IO.Compression.ZipFile|4.3.0; | ||
System.IO.FileSystem|4.3.0; | ||
System.IO.FileSystem.DriveInfo|4.3.0; | ||
System.IO.FileSystem.Primitives|4.3.0; | ||
System.IO.FileSystem.Watcher|4.3.0; | ||
System.IO.IsolatedStorage|4.3.0; | ||
System.IO.MemoryMappedFiles|4.3.0; | ||
System.IO.Pipes|4.3.0; | ||
System.IO.UnmanagedMemoryStream|4.3.0; | ||
System.Linq|4.3.0; | ||
System.Linq.Expressions|4.3.0; | ||
System.Linq.Queryable|4.3.0; | ||
System.Net.Http|4.3.0; | ||
System.Net.NameResolution|4.3.0; | ||
System.Net.Primitives|4.3.0; | ||
System.Net.Requests|4.3.0; | ||
System.Net.Security|4.3.0; | ||
System.Net.Sockets|4.3.0; | ||
System.Net.WebHeaderCollection|4.3.0; | ||
System.ObjectModel|4.3.0; | ||
System.Private.DataContractSerialization|4.3.0; | ||
System.Reflection|4.3.0; | ||
System.Reflection.Emit|4.3.0; | ||
System.Reflection.Emit.ILGeneration|4.3.0; | ||
System.Reflection.Emit.Lightweight|4.3.0; | ||
System.Reflection.Extensions|4.3.0; | ||
System.Reflection.Primitives|4.3.0; | ||
System.Reflection.TypeExtensions|4.3.0; | ||
System.Resources.ResourceManager|4.3.0; | ||
System.Runtime|4.3.0; | ||
System.Runtime.Extensions|4.3.0; | ||
System.Runtime.Handles|4.3.0; | ||
System.Runtime.InteropServices|4.3.0; | ||
System.Runtime.InteropServices.RuntimeInformation|4.3.0; | ||
System.Runtime.Loader|4.3.0; | ||
System.Runtime.Numerics|4.3.0; | ||
System.Runtime.Serialization.Formatters|4.3.0; | ||
System.Runtime.Serialization.Json|4.3.0; | ||
System.Runtime.Serialization.Primitives|4.3.0; | ||
System.Security.AccessControl|4.4.0; | ||
System.Security.Claims|4.3.0; | ||
System.Security.Cryptography.Algorithms|4.3.0; | ||
System.Security.Cryptography.Csp|4.3.0; | ||
System.Security.Cryptography.Encoding|4.3.0; | ||
System.Security.Cryptography.Primitives|4.3.0; | ||
System.Security.Cryptography.X509Certificates|4.3.0; | ||
System.Security.Cryptography.Xml|4.4.0; | ||
System.Security.Principal|4.3.0; | ||
System.Security.Principal.Windows|4.4.0; | ||
System.Text.Encoding|4.3.0; | ||
System.Text.Encoding.Extensions|4.3.0; | ||
System.Text.RegularExpressions|4.3.0; | ||
System.Threading|4.3.0; | ||
System.Threading.Overlapped|4.3.0; | ||
System.Threading.Tasks|4.3.0; | ||
System.Threading.Tasks.Extensions|4.3.0; | ||
System.Threading.Tasks.Parallel|4.3.0; | ||
System.Threading.Thread|4.3.0; | ||
System.Threading.ThreadPool|4.3.0; | ||
System.Threading.Timer|4.3.0; | ||
System.ValueTuple|4.3.0; | ||
System.Xml.ReaderWriter|4.3.0; | ||
System.Xml.XDocument|4.3.0; | ||
System.Xml.XmlDocument|4.3.0; | ||
System.Xml.XmlSerializer|4.3.0; | ||
System.Xml.XPath|4.3.0; | ||
System.Xml.XPath.XDocument|4.3.0; | ||
</OverridenPackages> | ||
</PackageConflictOverrides> | ||
</ItemGroup> | ||
</Project> |
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/