-
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 all commits
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 |
---|---|---|
@@ -0,0 +1,64 @@ | ||
// Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using Microsoft.Build.Framework; | ||
|
||
namespace Microsoft.NET.Build.Tasks.ConflictResolution | ||
{ | ||
/// <summary> | ||
/// A PackageOverride contains information about a package that overrides | ||
/// a set of packages up to a certain version. | ||
/// </summary> | ||
/// <remarks> | ||
/// For example, Microsoft.NETCore.App overrides System.Console up to version 4.3.0, | ||
/// System.IO up to version version 4.3.0, etc. | ||
/// </remarks> | ||
internal class PackageOverride | ||
{ | ||
public string PackageName { get; } | ||
public Dictionary<string, Version> OverridenPackages { get; } | ||
|
||
private PackageOverride(string packageName, IEnumerable<Tuple<string, Version>> overridenPackages) | ||
{ | ||
PackageName = packageName; | ||
|
||
OverridenPackages = new Dictionary<string, Version>(StringComparer.OrdinalIgnoreCase); | ||
foreach (Tuple<string, Version> package in overridenPackages) | ||
{ | ||
OverridenPackages[package.Item1] = package.Item2; | ||
} | ||
} | ||
|
||
public static PackageOverride Create(ITaskItem packageOverrideItem) | ||
{ | ||
string packageName = packageOverrideItem.ItemSpec; | ||
string overridenPackagesString = packageOverrideItem.GetMetadata(MetadataKeys.OverridenPackages); | ||
|
||
return new PackageOverride(packageName, CreateOverridenPackages(overridenPackagesString)); | ||
} | ||
|
||
private static IEnumerable<Tuple<string, Version>> CreateOverridenPackages(string overridenPackagesString) | ||
{ | ||
if (!string.IsNullOrEmpty(overridenPackagesString)) | ||
{ | ||
overridenPackagesString = overridenPackagesString.Trim(); | ||
string[] overridenPackagesAndVersions = overridenPackagesString.Split(new char[] { ';' }, StringSplitOptions.RemoveEmptyEntries); | ||
foreach (string overridenPackagesAndVersion in overridenPackagesAndVersions) | ||
{ | ||
string trimmedOverridenPackagesAndVersion = overridenPackagesAndVersion.Trim(); | ||
int separatorIndex = trimmedOverridenPackagesAndVersion.IndexOf('|'); | ||
if (separatorIndex != -1) | ||
{ | ||
if (Version.TryParse(trimmedOverridenPackagesAndVersion.Substring(separatorIndex + 1), out Version version)) | ||
{ | ||
yield return Tuple.Create(trimmedOverridenPackagesAndVersion.Substring(0, separatorIndex), version); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
// Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using Microsoft.Build.Framework; | ||
|
||
namespace Microsoft.NET.Build.Tasks.ConflictResolution | ||
{ | ||
/// <summary> | ||
/// Resolves conflicts between items by allowing specific packages to override | ||
/// all items coming from a set of packages up to a certain version of each package. | ||
/// </summary> | ||
internal class PackageOverrideResolver<TConflictItem> where TConflictItem : class, IConflictItem | ||
{ | ||
private ITaskItem[] _packageOverrideItems; | ||
private Lazy<Dictionary<string, PackageOverride>> _packageOverrides; | ||
|
||
public PackageOverrideResolver(ITaskItem[] packageOverrideItems) | ||
{ | ||
_packageOverrideItems = packageOverrideItems; | ||
_packageOverrides = new Lazy<Dictionary<string, PackageOverride>>(() => BuildPackageOverrides()); | ||
} | ||
|
||
public Dictionary<string, PackageOverride> PackageOverrides => _packageOverrides.Value; | ||
|
||
private Dictionary<string, PackageOverride> BuildPackageOverrides() | ||
{ | ||
Dictionary<string, PackageOverride> result; | ||
|
||
if (_packageOverrideItems?.Length > 0) | ||
{ | ||
result = new Dictionary<string, PackageOverride>(_packageOverrideItems.Length, StringComparer.OrdinalIgnoreCase); | ||
|
||
foreach (ITaskItem packageOverrideItem in _packageOverrideItems) | ||
{ | ||
PackageOverride packageOverride = PackageOverride.Create(packageOverrideItem); | ||
|
||
if (result.TryGetValue(packageOverride.PackageName, out PackageOverride existing)) | ||
{ | ||
MergePackageOverrides(packageOverride, existing); | ||
} | ||
else | ||
{ | ||
result[packageOverride.PackageName] = packageOverride; | ||
} | ||
} | ||
} | ||
else | ||
{ | ||
result = null; | ||
} | ||
|
||
return result; | ||
} | ||
|
||
/// <summary> | ||
/// Merges newPackageOverride into existingPackageOverride by adding all the new overriden packages | ||
/// and taking the highest version when they both contain the same overriden package. | ||
/// </summary> | ||
private static void MergePackageOverrides(PackageOverride newPackageOverride, PackageOverride existingPackageOverride) | ||
{ | ||
foreach (KeyValuePair<string, Version> newOverride in newPackageOverride.OverridenPackages) | ||
{ | ||
if (existingPackageOverride.OverridenPackages.TryGetValue(newOverride.Key, out Version existingOverrideVersion)) | ||
{ | ||
if (existingOverrideVersion < newOverride.Value) | ||
{ | ||
existingPackageOverride.OverridenPackages[newOverride.Key] = newOverride.Value; | ||
} | ||
} | ||
else | ||
{ | ||
existingPackageOverride.OverridenPackages[newOverride.Key] = newOverride.Value; | ||
} | ||
} | ||
} | ||
|
||
public TConflictItem Resolve(TConflictItem item1, TConflictItem item2) | ||
{ | ||
if (PackageOverrides != null) | ||
{ | ||
PackageOverride packageOverride; | ||
Version version; | ||
if (item1.PackageId != null | ||
&& PackageOverrides.TryGetValue(item1.PackageId, out packageOverride) | ||
&& packageOverride.OverridenPackages.TryGetValue(item2.PackageId, out version) | ||
&& item2.PackageVersion != null | ||
&& item2.PackageVersion <= version) | ||
{ | ||
return item1; | ||
} | ||
else if (item2.PackageId != null | ||
&& PackageOverrides.TryGetValue(item2.PackageId, out packageOverride) | ||
&& packageOverride.OverridenPackages.TryGetValue(item1.PackageId, out version) | ||
&& item1.PackageVersion != null | ||
&& item1.PackageVersion <= version) | ||
{ | ||
return item2; | ||
} | ||
} | ||
|
||
return null; | ||
} | ||
} | ||
} |
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) | ||
|
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.