From 8fb627e7fcee0531050fe1c68d3726a2dc35b887 Mon Sep 17 00:00:00 2001 From: Forgind Date: Sat, 6 Feb 2021 13:07:01 -0800 Subject: [PATCH] Optimize RAR's GetReferenceItems method (#5929) * Iterate over dictionary once * Avoid unnecessary initializations * Set copy local more efficiently * referenceItem should have no metadata initially * Update larger metadata dictionary * Set fusionName at end * Slightly simplify referenceItem creation * Fix typos * Addition to setting fusionName at end * Shift other types of removed metadata later so they only have to be removed once nonForwardableMetadata?.Remove(ItemMetadataNames.*) is used to prevent the correct metadata value from being overwritten when metadata are re-added from nonForwardableMetadata. * Use ternary operators * Set items as a group --- .../Immutable/ImmutableDictionary.cs | 11 + src/Shared/CopyOnWriteDictionary.cs | 12 + .../AssemblyDependency/ReferenceTable.cs | 209 +++++++++--------- src/Utilities/TaskItem.cs | 23 +- 4 files changed, 145 insertions(+), 110 deletions(-) diff --git a/src/MSBuildTaskHost/Immutable/ImmutableDictionary.cs b/src/MSBuildTaskHost/Immutable/ImmutableDictionary.cs index c540a86288e..6ed1e6755d5 100644 --- a/src/MSBuildTaskHost/Immutable/ImmutableDictionary.cs +++ b/src/MSBuildTaskHost/Immutable/ImmutableDictionary.cs @@ -138,6 +138,17 @@ private void CheckCopyToArguments(Array array, int arrayIndex) return clone; } + internal ImmutableDictionary SetItems(IEnumerable> items) + { + var clone = new ImmutableDictionary(_backing); + foreach (KeyValuePair item in items) + { + clone._backing[item.Key] = item.Value; + } + + return clone; + } + internal ImmutableDictionary Remove(K key) { if (!ContainsKey(key)) diff --git a/src/Shared/CopyOnWriteDictionary.cs b/src/Shared/CopyOnWriteDictionary.cs index 31d376092ea..7059463de3b 100644 --- a/src/Shared/CopyOnWriteDictionary.cs +++ b/src/Shared/CopyOnWriteDictionary.cs @@ -227,6 +227,18 @@ public void Add(string key, V value) _backing = _backing.SetItem(key, value); } + /// + /// Adds several value to the dictionary. + /// + public void SetItems(IEnumerable> items) + { + _backing = _backing.SetItems(items); + } + + public IEnumerable> Where(Func, bool> predicate) + { + return _backing.Where(predicate); + } /// /// Returns true if the dictionary contains the specified key. /// diff --git a/src/Tasks/AssemblyDependency/ReferenceTable.cs b/src/Tasks/AssemblyDependency/ReferenceTable.cs index 405f8662611..dcc38343d3c 100644 --- a/src/Tasks/AssemblyDependency/ReferenceTable.cs +++ b/src/Tasks/AssemblyDependency/ReferenceTable.cs @@ -2616,14 +2616,6 @@ internal void GetReferenceItems out ITaskItem[] copyLocalFiles ) { - primaryFiles = Array.Empty(); - dependencyFiles = Array.Empty(); - relatedFiles = Array.Empty(); - satelliteFiles = Array.Empty(); - serializationAssemblyFiles = Array.Empty(); - scatterFiles = Array.Empty(); - copyLocalFiles = Array.Empty(); - var primaryItems = new List(); var dependencyItems = new List(); var relatedItems = new List(); @@ -2632,10 +2624,10 @@ internal void GetReferenceItems var scatterItems = new List(); var copyLocalItems = new List(); - foreach (AssemblyNameExtension assemblyName in References.Keys) + foreach (KeyValuePair kvp in References) { - string fusionName = assemblyName.FullName; - Reference reference = GetReference(assemblyName); + AssemblyNameExtension assemblyName = kvp.Key; + Reference reference = kvp.Value; // Conflict victims and badimages are filtered out. if (!reference.IsBadImage) @@ -2664,7 +2656,7 @@ internal void GetReferenceItems if (reference.IsResolved) { - ITaskItem referenceItem = SetItemMetadata(relatedItems, satelliteItems, serializationAssemblyItems, scatterItems, fusionName, reference, assemblyName); + ITaskItem referenceItem = SetItemMetadata(relatedItems, satelliteItems, serializationAssemblyItems, scatterItems, assemblyName.FullName, reference, assemblyName); if (reference.IsPrimary) { @@ -2683,9 +2675,7 @@ internal void GetReferenceItems } } - primaryFiles = new ITaskItem[primaryItems.Count]; - primaryItems.CopyTo(primaryFiles, 0); - + primaryFiles = primaryItems.ToArray(); dependencyFiles = dependencyItems.ToArray(); relatedFiles = relatedItems.ToArray(); satelliteFiles = satelliteItems.ToArray(); @@ -2711,22 +2701,12 @@ internal void GetReferenceItems private ITaskItem SetItemMetadata(List relatedItems, List satelliteItems, List serializationAssemblyItems, List scatterItems, string fusionName, Reference reference, AssemblyNameExtension assemblyName) { // Set up the main item. - ITaskItem referenceItem = new TaskItem(); + TaskItem referenceItem = new TaskItem(); referenceItem.ItemSpec = reference.FullPath; referenceItem.SetMetadata(ItemMetadataNames.resolvedFrom, reference.ResolvedSearchPath); // Set the CopyLocal metadata. - if (reference.IsCopyLocal) - { - referenceItem.SetMetadata(ItemMetadataNames.copyLocal, "true"); - } - else - { - referenceItem.SetMetadata(ItemMetadataNames.copyLocal, "false"); - } - - // Set the FusionName metadata. - referenceItem.SetMetadata(ItemMetadataNames.fusionName, fusionName); + referenceItem.SetMetadata(ItemMetadataNames.copyLocal, reference.IsCopyLocal ? "true" : "false"); // Set the Redist name metadata. if (!String.IsNullOrEmpty(reference.RedistName)) @@ -2747,57 +2727,11 @@ private ITaskItem SetItemMetadata(List relatedItems, List referenceItem.SetMetadata(ItemMetadataNames.imageRuntime, reference.ImageRuntime); } - if (reference.IsWinMDFile) - { - referenceItem.SetMetadata(ItemMetadataNames.winMDFile, "true"); - - // The ImplementationAssembly is only set if the implementation file exits on disk - if (reference.ImplementationAssembly != null) - { - if (VerifyArchitectureOfImplementationDll(reference.ImplementationAssembly, reference.FullPath)) - { - if (string.IsNullOrEmpty(referenceItem.GetMetadata(ItemMetadataNames.winmdImplmentationFile))) - { - referenceItem.SetMetadata(ItemMetadataNames.winmdImplmentationFile, Path.GetFileName(reference.ImplementationAssembly)); - } - - // Add the implementation item as a related file - ITaskItem item = new TaskItem(reference.ImplementationAssembly); - // Clone metadata. - referenceItem.CopyMetadataTo(item); - // Related files don't have a fusion name. - item.SetMetadata(ItemMetadataNames.fusionName, ""); - RemoveNonForwardableMetadata(item); - - // Add the related item. - relatedItems.Add(item); - } - } - - if (reference.IsManagedWinMDFile) - { - referenceItem.SetMetadata(ItemMetadataNames.winMDFileType, "Managed"); - } - else - { - referenceItem.SetMetadata(ItemMetadataNames.winMDFileType, "Native"); - } - } - - // Set the IsRedistRoot metadata - if (reference.IsRedistRoot == true) - { - referenceItem.SetMetadata(ItemMetadataNames.isRedistRoot, "true"); - } - else if (reference.IsRedistRoot == false) - { - referenceItem.SetMetadata(ItemMetadataNames.isRedistRoot, "false"); - } - else + // 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) { - // This happens when the redist root is "null". This means there - // was no IsRedistRoot flag in the Redist XML (or there was no - // redist XML at all for this item). + referenceItem.SetMetadata(ItemMetadataNames.isRedistRoot, (bool)reference.IsRedistRoot ? "true" : "false"); } // If there was a primary source item, then forward metadata from it. @@ -2826,14 +2760,14 @@ private ITaskItem SetItemMetadata(List relatedItems, List } // If the item originally did not have the implementation file metadata then we do not want to get it from the set of primary source items - // since the implementation file is something specific to the source item and not supposed to be propigated. + // since the implementation file is something specific to the source item and not supposed to be propagated. if (!hasImplementationFile) { referenceItem.RemoveMetadata(ItemMetadataNames.winmdImplmentationFile); } // If the item originally did not have the ImageRuntime metadata then we do not want to get it from the set of primary source items - // since the ImageRuntime is something specific to the source item and not supposed to be propigated. + // since the ImageRuntime is something specific to the source item and not supposed to be propagated. if (!hasImageRuntime) { referenceItem.RemoveMetadata(ItemMetadataNames.imageRuntime); @@ -2847,68 +2781,63 @@ private ITaskItem SetItemMetadata(List relatedItems, List } } - if (reference.ReferenceVersion != null) - { - referenceItem.SetMetadata(ItemMetadataNames.version, reference.ReferenceVersion.ToString()); - } - else + referenceItem.SetMetadata(ItemMetadataNames.version, reference.ReferenceVersion == null ? string.Empty : reference.ReferenceVersion.ToString()); + + // Unset fusionName so we don't have to unset it later. + referenceItem.RemoveMetadata(ItemMetadataNames.fusionName); + + List relatedFileExtensions = reference.GetRelatedFileExtensions(); + List satellites = reference.GetSatelliteFiles(); + List serializationAssemblyFiles = reference.GetSerializationAssemblyFiles(); + string[] scatterFiles = reference.GetScatterFiles(); + Dictionary nonForwardableMetadata = null; + if (relatedFileExtensions.Count > 0 || satellites.Count > 0 || serializationAssemblyFiles.Count > 0 || scatterFiles.Length > 0) { - referenceItem.SetMetadata(ItemMetadataNames.version, String.Empty); + // Unset non-forwardable metadata now so we don't have to do it for individual items. + nonForwardableMetadata = RemoveNonForwardableMetadata(referenceItem); } // Now clone all properties onto the related files. - foreach (string relatedFileExtension in reference.GetRelatedFileExtensions()) + foreach (string relatedFileExtension in relatedFileExtensions) { ITaskItem item = new TaskItem(reference.FullPathWithoutExtension + relatedFileExtension); // Clone metadata. referenceItem.CopyMetadataTo(item); - // Related files don't have a fusion name. - item.SetMetadata(ItemMetadataNames.fusionName, ""); - RemoveNonForwardableMetadata(item); // Add the related item. relatedItems.Add(item); } // Set up the satellites. - foreach (string satelliteFile in reference.GetSatelliteFiles()) + foreach (string satelliteFile in satellites) { ITaskItem item = new TaskItem(Path.Combine(reference.DirectoryName, satelliteFile)); // Clone metadata. referenceItem.CopyMetadataTo(item); // Set the destination directory. item.SetMetadata(ItemMetadataNames.destinationSubDirectory, FileUtilities.EnsureTrailingSlash(Path.GetDirectoryName(satelliteFile))); - // Satellite files don't have a fusion name. - item.SetMetadata(ItemMetadataNames.fusionName, ""); - RemoveNonForwardableMetadata(item); // Add the satellite item. satelliteItems.Add(item); } // Set up the serialization assemblies - foreach (string serializationAssemblyFile in reference.GetSerializationAssemblyFiles()) + foreach (string serializationAssemblyFile in serializationAssemblyFiles) { ITaskItem item = new TaskItem(Path.Combine(reference.DirectoryName, serializationAssemblyFile)); // Clone metadata. referenceItem.CopyMetadataTo(item); - // serialization assemblies files don't have a fusion name. - item.SetMetadata(ItemMetadataNames.fusionName, ""); - RemoveNonForwardableMetadata(item); // Add the serialization assembly item. serializationAssemblyItems.Add(item); } // Set up the scatter files. - foreach (string scatterFile in reference.GetScatterFiles()) + foreach (string scatterFile in scatterFiles) { ITaskItem item = new TaskItem(Path.Combine(reference.DirectoryName, scatterFile)); // Clone metadata. referenceItem.CopyMetadataTo(item); - // We don't have a fusion name for scatter files. - item.SetMetadata(ItemMetadataNames.fusionName, ""); - RemoveNonForwardableMetadata(item); // Add the satellite item. scatterItems.Add(item); @@ -2928,6 +2857,61 @@ private ITaskItem SetItemMetadata(List relatedItems, List } } + if (reference.IsWinMDFile) + { + // The ImplementationAssembly is only set if the implementation file exits on disk + if (reference.ImplementationAssembly != null) + { + if (VerifyArchitectureOfImplementationDll(reference.ImplementationAssembly, reference.FullPath)) + { + // Add the implementation item as a related file + ITaskItem item = new TaskItem(reference.ImplementationAssembly); + // Clone metadata. + referenceItem.CopyMetadataTo(item); + + // Add the related item. + relatedItems.Add(item); + + referenceItem.SetMetadata(ItemMetadataNames.winmdImplmentationFile, Path.GetFileName(reference.ImplementationAssembly)); + // This may have been set previously (before it was removed so we could more efficiently set metadata on the various related files). + // This version should take priority, so we remove it from nonForwardableMetadata if it's there to prevent the correct value from + // being overwritten. + nonForwardableMetadata?.Remove(ItemMetadataNames.winmdImplmentationFile); + } + } + + // This may have been set previously (before it was removed so we could more efficiently set metadata on the various related files). + // This version should take priority, so we remove it from nonForwardableMetadata if it's there to prevent the correct value from + // being overwritten. + nonForwardableMetadata?.Remove(ItemMetadataNames.winMDFileType); + if (reference.IsManagedWinMDFile) + { + referenceItem.SetMetadata(ItemMetadataNames.winMDFileType, "Managed"); + } + else + { + referenceItem.SetMetadata(ItemMetadataNames.winMDFileType, "Native"); + } + + // This may have been set previously (before it was removed so we could more efficiently set metadata on the various related files). + // This version should take priority, so we remove it from nonForwardableMetadata if it's there to prevent the correct value from + // being overwritten. + nonForwardableMetadata?.Remove(ItemMetadataNames.winMDFile); + referenceItem.SetMetadata(ItemMetadataNames.winMDFile, "true"); + } + + // Set the FusionName late, so we don't copy it to the derived items, but it's still available on referenceItem. + referenceItem.SetMetadata(ItemMetadataNames.fusionName, fusionName); + + // nonForwardableMetadata should be null here if relatedFileExtensions, satellites, serializationAssemblyFiles, and scatterFiles were all empty. + if (nonForwardableMetadata != null) + { + foreach (KeyValuePair kvp in nonForwardableMetadata) + { + referenceItem.SetMetadata(kvp.Key, kvp.Value); + } + } + return referenceItem; } @@ -3061,15 +3045,28 @@ internal static UInt16 ReadMachineTypeFromPEHeader(string dllPath) /// /// Some metadata should not be forwarded between the parent and child items. /// - private static void RemoveNonForwardableMetadata(ITaskItem item) + /// The metadata that were removed. + private static Dictionary RemoveNonForwardableMetadata(ITaskItem item) { - item.RemoveMetadata(ItemMetadataNames.winmdImplmentationFile); - item.RemoveMetadata(ItemMetadataNames.imageRuntime); - item.RemoveMetadata(ItemMetadataNames.winMDFile); + Dictionary removedMetadata = new Dictionary(); + RemoveMetadatum(ItemMetadataNames.winmdImplmentationFile, item, removedMetadata); + RemoveMetadatum(ItemMetadataNames.imageRuntime, item, removedMetadata); + RemoveMetadatum(ItemMetadataNames.winMDFile, item, removedMetadata); if (!Traits.Instance.EscapeHatches.TargetPathForRelatedFiles) { - item.RemoveMetadata(ItemMetadataNames.targetPath); + RemoveMetadatum(ItemMetadataNames.targetPath, item, removedMetadata); + } + return removedMetadata; + } + + private static void RemoveMetadatum(string key, ITaskItem item, Dictionary removedMetadata) + { + string meta = item.GetMetadata(key); + if (!String.IsNullOrEmpty(meta)) + { + removedMetadata.Add(key, meta); } + item.RemoveMetadata(key); } /// diff --git a/src/Utilities/TaskItem.cs b/src/Utilities/TaskItem.cs index 0a361a8cd7a..a2bb6693c41 100644 --- a/src/Utilities/TaskItem.cs +++ b/src/Utilities/TaskItem.cs @@ -30,7 +30,7 @@ public sealed class TaskItem : #if FEATURE_APPDOMAIN MarshalByRefObject, #endif - ITaskItem, ITaskItem2 + ITaskItem2 { #region Member Data @@ -297,10 +297,25 @@ public void CopyMetadataTo(ITaskItem destinationItem) if (_metadata != null) { - // Avoid a copy if we can - if (destinationItem is TaskItem destinationAsTaskItem && destinationAsTaskItem.Metadata == null) + if (destinationItem is TaskItem destinationAsTaskItem) { - destinationAsTaskItem.Metadata = _metadata.Clone(); // Copy on write! + CopyOnWriteDictionary copiedMetadata; + // Avoid a copy if we can, and if not, minimize the number of items we have to set. + if (destinationAsTaskItem.Metadata == null) + { + copiedMetadata = _metadata.Clone(); // Copy on write! + } + else if (destinationAsTaskItem.Metadata.Count < _metadata.Count) + { + copiedMetadata = _metadata.Clone(); // Copy on write! + copiedMetadata.SetItems(destinationAsTaskItem.Metadata.Where(entry => !String.IsNullOrEmpty(entry.Value))); + } + else + { + copiedMetadata = destinationAsTaskItem.Metadata.Clone(); + copiedMetadata.SetItems(_metadata.Where(entry => !destinationAsTaskItem.Metadata.TryGetValue(entry.Key, out string val) || String.IsNullOrEmpty(val))); + } + destinationAsTaskItem.Metadata = copiedMetadata; } else {