Skip to content

Commit

Permalink
ResolveAssemblyReference CPU optimizations (dotnet#8916)
Browse files Browse the repository at this point in the history
### Context

Low-hanging fruit is showing in RAR performance profiles.

### Changes Made

1. Avoided constructing `AssemblyName` on a hot path as the constructor makes expensive Fusion calls on .NET Framework. The problematic code was introduced in dotnet#8688.

2. Added a metadata bulk-set operation to the internal `IMetadataContainer` interface. Calling `SetMetadata` for more than a couple of metadata is slow if `ImmutableDictionary` is used as its backing storage. RAR is heavy on metadata manipulation and switching to the new operation saves about 10% of RAR run-time when building OrchardCore. 

### Testing

Existing and new unit tests. Measured the perf impact by building OC.

---------

Co-authored-by: Rainer Sigwald <raines@microsoft.com>
  • Loading branch information
2 people authored and YuliiaKovalova committed Jul 18, 2023
1 parent 45645b1 commit de6b795
Show file tree
Hide file tree
Showing 8 changed files with 154 additions and 38 deletions.
43 changes: 43 additions & 0 deletions src/Build.OM.UnitTests/Instance/ProjectItemInstance_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,49 @@ public void AccessorsWithMetadata()
Assert.Equal("v2", item.GetMetadataValue("m2"));
}

/// <summary>
/// Basic ProjectItemInstance with metadata added using ImportMetadata
/// </summary>
[Fact]
public void AccessorsWithImportedMetadata()
{
ProjectItemInstance item = GetItemInstance();

((IMetadataContainer)item).ImportMetadata(new Dictionary<string, string>
{
{ "m1", "v1" },
{ "m2", "v2" },
});

Assert.Equal("m1", item.GetMetadata("m1").Name);
Assert.Equal("m2", item.GetMetadata("m2").Name);
Assert.Equal("v1", item.GetMetadataValue("m1"));
Assert.Equal("v2", item.GetMetadataValue("m2"));
}

/// <summary>
/// ImportMetadata adds and overwrites metadata, does not delete existing metadata
/// </summary>
[Fact]
public void ImportMetadataAddsAndOverwrites()
{
ProjectItemInstance item = GetItemInstance();

item.SetMetadata("m1", "v1");
item.SetMetadata("m2", "v0");

((IMetadataContainer) item).ImportMetadata(new Dictionary<string, string>
{
{ "m2", "v2" },
{ "m3", "v3" },
});

// m1 was not deleted, m2 was overwritten, m3 was added
Assert.Equal("v1", item.GetMetadataValue("m1"));
Assert.Equal("v2", item.GetMetadataValue("m2"));
Assert.Equal("v3", item.GetMetadataValue("m3"));
}

/// <summary>
/// Get metadata not present
/// </summary>
Expand Down
27 changes: 24 additions & 3 deletions src/Build/Instance/ProjectItemInstance.cs
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,8 @@ IDictionary ITaskItem2.CloneCustomMetadataEscaped()

IEnumerable<KeyValuePair<string, string>> IMetadataContainer.EnumerateMetadata() => _taskItem.EnumerateMetadata();

void IMetadataContainer.ImportMetadata(IEnumerable<KeyValuePair<string, string>> metadata) => _taskItem.ImportMetadata(metadata);

#region IMetadataTable Members

/// <summary>
Expand Down Expand Up @@ -1034,6 +1036,19 @@ public IEnumerable<KeyValuePair<string, string>> EnumerateMetadata()
}
}

/// <summary>
/// Sets the given metadata.
/// Equivalent to calling <see cref="SetMetadata(string,string)"/> for each item in <paramref name="metadata"/>.
/// </summary>
/// <param name="metadata">The metadata to set.</param>
public void ImportMetadata(IEnumerable<KeyValuePair<string, string>> metadata)
{
ProjectInstance.VerifyThrowNotImmutable(_isImmutable);

_directMetadata ??= new CopyOnWritePropertyDictionary<ProjectMetadataInstance>();
_directMetadata.ImportProperties(metadata.Select(kvp => new ProjectMetadataInstance(kvp.Key, kvp.Value, allowItemSpecModifiers: true)));
}

/// <summary>
/// Used to return metadata from another AppDomain. Can't use yield return because the
/// generated state machine is not marked as [Serializable], so we need to allocate.
Expand Down Expand Up @@ -1371,9 +1386,7 @@ public void CopyMetadataTo(ITaskItem destinationItem, bool addOriginalItemSpec)
originalItemSpec = destinationItem.GetMetadata("OriginalItemSpec");
}

TaskItem destinationAsTaskItem = destinationItem as TaskItem;

if (destinationAsTaskItem != null && destinationAsTaskItem._directMetadata == null)
if (destinationItem is TaskItem destinationAsTaskItem && destinationAsTaskItem._directMetadata == null)
{
ProjectInstance.VerifyThrowNotImmutable(destinationAsTaskItem._isImmutable);

Expand All @@ -1391,6 +1404,14 @@ public void CopyMetadataTo(ITaskItem destinationItem, bool addOriginalItemSpec)
destinationAsTaskItem._itemDefinitions.AddRange(_itemDefinitions);
}
}
else if (destinationItem is IMetadataContainer destinationItemAsMetadataContainer)
{
// The destination implements IMetadataContainer so we can use the ImportMetadata bulk-set operation.
IEnumerable<ProjectMetadataInstance> metadataEnumerable = MetadataCollection;
destinationItemAsMetadataContainer.ImportMetadata(metadataEnumerable
.Where(metadatum => string.IsNullOrEmpty(destinationItem.GetMetadata(metadatum.Name)))
.Select(metadatum => new KeyValuePair<string, string>(metadatum.Name, GetMetadataEscaped(metadatum.Name))));
}
else
{
// OK, most likely the destination item was a Microsoft.Build.Utilities.TaskItem.
Expand Down
12 changes: 12 additions & 0 deletions src/Framework/IMetadataContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,17 @@ internal interface IMetadataContainer
/// in the binary logger.
/// </summary>
IEnumerable<KeyValuePair<string, string>> EnumerateMetadata();

/// <summary>
/// Sets the given metadata. The operation is equivalent to calling
/// <see cref="ITaskItem.SetMetadata"/> on all metadata, but takes
/// advantage of a faster bulk-set operation where applicable. The
/// implementation may not perform the same parameter validation
/// as SetMetadata.
/// </summary>
/// <param name="metadata">The metadata to set. The keys are assumed
/// to be unique and values are assumed to be escaped.
/// </param>
void ImportMetadata(IEnumerable<KeyValuePair<string, string>> metadata);
}
}
3 changes: 3 additions & 0 deletions src/Framework/TaskItemData.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ public TaskItemData(ITaskItem original)

IEnumerable<KeyValuePair<string, string>> IMetadataContainer.EnumerateMetadata() => Metadata;

void IMetadataContainer.ImportMetadata(IEnumerable<KeyValuePair<string, string>> metadata)
=> throw new NotImplementedException();

public int MetadataCount => Metadata.Count;

public ICollection MetadataNames => (ICollection)Metadata.Keys;
Expand Down
8 changes: 8 additions & 0 deletions src/Shared/TaskParameter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,14 @@ private IEnumerable<KeyValuePair<string, string>> EnumerateMetadataLazy()
yield return unescaped;
}
}

public void ImportMetadata(IEnumerable<KeyValuePair<string, string>> metadata)
{
foreach (KeyValuePair<string, string> kvp in metadata)
{
SetMetadata(kvp.Key, kvp.Value);
}
}
}
}
}
74 changes: 39 additions & 35 deletions src/Tasks/AssemblyDependency/ReferenceTable.cs
Original file line number Diff line number Diff line change
Expand Up @@ -852,8 +852,7 @@ private static AssemblyNameExtension GetAssemblyNameFromItemMetadata(ITaskItem i
name = item.GetMetadata(FileUtilities.ItemSpecModifiers.Filename);
}

AssemblyName assemblyName = new AssemblyName($"{name}, Version={version}, Culture=neutral, PublicKeyToken={publicKeyToken}");
return new AssemblyNameExtension(assemblyName);
return new AssemblyNameExtension($"{name}, Version={version}, Culture=neutral, PublicKeyToken={publicKeyToken}");
}

/// <summary>
Expand Down Expand Up @@ -2677,36 +2676,9 @@ private ITaskItem SetItemMetadata(List<ITaskItem> relatedItems, List<ITaskItem>
// Set up the main item.
TaskItem referenceItem = new TaskItem();
referenceItem.ItemSpec = reference.FullPath;
referenceItem.SetMetadata(ItemMetadataNames.resolvedFrom, reference.ResolvedSearchPath);

// Set the CopyLocal metadata.
referenceItem.SetMetadata(ItemMetadataNames.copyLocal, reference.IsCopyLocal ? "true" : "false");

// Set the Redist name metadata.
if (!String.IsNullOrEmpty(reference.RedistName))
{
referenceItem.SetMetadata(ItemMetadataNames.redist, reference.RedistName);
}

if (Reference.IsFrameworkFile(reference.FullPath, _frameworkPaths) || (_installedAssemblies?.FrameworkAssemblyEntryInRedist(assemblyName) == true))
{
if (!IsAssemblyRemovedFromDotNetFramework(assemblyName, reference.FullPath, _frameworkPaths, _installedAssemblies))
{
referenceItem.SetMetadata(ItemMetadataNames.frameworkFile, "true");
}
}

if (!String.IsNullOrEmpty(reference.ImageRuntime))
{
referenceItem.SetMetadata(ItemMetadataNames.imageRuntime, reference.ImageRuntime);
}

// The redist root is "null" when there was no IsRedistRoot flag in the Redist XML
// (or there was no redist XML at all for this item).
if (reference.IsRedistRoot != null)
{
referenceItem.SetMetadata(ItemMetadataNames.isRedistRoot, (bool)reference.IsRedistRoot ? "true" : "false");
}
IMetadataContainer referenceItemAsMetadataContainer = referenceItem;
referenceItemAsMetadataContainer.ImportMetadata(EnumerateCommonMetadata());

// If there was a primary source item, then forward metadata from it.
// It's important that the metadata from the primary source item
Expand Down Expand Up @@ -2880,13 +2852,45 @@ private ITaskItem SetItemMetadata(List<ITaskItem> relatedItems, List<ITaskItem>
// nonForwardableMetadata should be null here if relatedFileExtensions, satellites, serializationAssemblyFiles, and scatterFiles were all empty.
if (nonForwardableMetadata != null)
{
foreach (KeyValuePair<string, string> kvp in nonForwardableMetadata)
{
referenceItem.SetMetadata(kvp.Key, kvp.Value);
}
referenceItemAsMetadataContainer.ImportMetadata(nonForwardableMetadata);
}

return referenceItem;

// Enumerate common metadata with an iterator to allow using a more efficient bulk-set operation.
IEnumerable<KeyValuePair<string, string>> EnumerateCommonMetadata()
{
yield return new KeyValuePair<string, string>(ItemMetadataNames.resolvedFrom, reference.ResolvedSearchPath);

// Set the CopyLocal metadata.
yield return new KeyValuePair<string, string>(ItemMetadataNames.copyLocal, reference.IsCopyLocal ? "true" : "false");

// Set the Redist name metadata.
if (!string.IsNullOrEmpty(reference.RedistName))
{
yield return new KeyValuePair<string, string>(ItemMetadataNames.redist, reference.RedistName);
}

if (Reference.IsFrameworkFile(reference.FullPath, _frameworkPaths) || (_installedAssemblies?.FrameworkAssemblyEntryInRedist(assemblyName) == true))
{
if (!IsAssemblyRemovedFromDotNetFramework(assemblyName, reference.FullPath, _frameworkPaths, _installedAssemblies))
{
yield return new KeyValuePair<string, string>(ItemMetadataNames.frameworkFile, "true");
}
}

if (!string.IsNullOrEmpty(reference.ImageRuntime))
{
yield return new KeyValuePair<string, string>(ItemMetadataNames.imageRuntime, reference.ImageRuntime);
}

// The redist root is "null" when there was no IsRedistRoot flag in the Redist XML
// (or there was no redist XML at all for this item).
if (reference.IsRedistRoot != null)
{
yield return new KeyValuePair<string, string>(ItemMetadataNames.isRedistRoot, (bool)reference.IsRedistRoot ? "true" : "false");
}
}
}

/// <summary>
Expand Down
19 changes: 19 additions & 0 deletions src/Utilities.UnitTests/TaskItem_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,25 @@ public void SetNullMetadataValue()
item.GetMetadata("m").ShouldBe(string.Empty);
}

[Fact]
public void ImplementsIMetadataContainer()
{
Dictionary<string, string> metadata = new()
{
{ "a", "a1" },
{ "b", "b1" },
};

TaskItem item = new TaskItem("foo");
IMetadataContainer metadataContainer = (IMetadataContainer)item;

metadataContainer.ImportMetadata(metadata);

var actualMetadata = metadataContainer.EnumerateMetadata().OrderBy(metadata => metadata.Key).ToList();
var expectedMetadata = metadata.OrderBy(metadata => metadata.Value).ToList();
Assert.True(actualMetadata.SequenceEqual(expectedMetadata));
}

#if FEATURE_APPDOMAIN
/// <summary>
/// Test that task items can be successfully constructed based on a task item from another appdomain.
Expand Down
6 changes: 6 additions & 0 deletions src/Utilities/TaskItem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,12 @@ IEnumerable<KeyValuePair<string, string>> IMetadataContainer.EnumerateMetadata()
return EnumerateMetadataLazy();
}

void IMetadataContainer.ImportMetadata(IEnumerable<KeyValuePair<string, string>> metadata)
{
_metadata ??= new CopyOnWriteDictionary<string>(MSBuildNameIgnoreCaseComparer.Default);
_metadata.SetItems(metadata.Select(kvp => new KeyValuePair<string, string>(kvp.Key, kvp.Value ?? string.Empty)));
}

private IEnumerable<KeyValuePair<string, string>> EnumerateMetadataEager()
{
if (_metadata == null)
Expand Down

0 comments on commit de6b795

Please sign in to comment.