ResolveAssemblyReference CPU optimizations (redo) (#9044)

#8916 broke some .NET Framework scenarios and was reverted. This PR is a redo of the change with the bug fixed. It turned out that the destination of the optimized CopyMetadataTo may be a transparent proxy, typically a TaskItem object living in another appdomain, which does not work well with Linq. The fix and the test coverage are in their own commits.

Context
Low-hanging fruit is showing in RAR performance profiles.

Changes Made
Avoided constructing AssemblyName on a hot path as the constructor makes expensive Fusion calls on .NET Framework. The problematic code was introduced in [RAR] Don't do I/O on SDK-provided references #8688.

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.

Author: ladipro

src/Build.OM.UnitTests/Instance/ProjectItemInstance_Tests.cs

@@ -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>
@@ -106,6 +149,56 @@ public void GetMissingMetadata()
             Assert.Equal(String.Empty, item.GetMetadataValue("X"));
         }
 
+        [Fact]
+        public void CopyMetadataToTaskItem()
+        {
+            ProjectItemInstance fromItem = GetItemInstance();
+
+            fromItem.SetMetadata("m1", "v1");
+            fromItem.SetMetadata("m2", "v2");
+
+            ITaskItem toItem = new Utilities.TaskItem();
+
+            ((ITaskItem)fromItem).CopyMetadataTo(toItem);
+
+            Assert.Equal("v1", toItem.GetMetadata("m1"));
+            Assert.Equal("v2", toItem.GetMetadata("m2"));
+        }
+
+#if FEATURE_APPDOMAIN
+        private sealed class RemoteTaskItemFactory : MarshalByRefObject
+        {
+            public ITaskItem CreateTaskItem() => new Utilities.TaskItem();
+        }
+
+        [Fact]
+        public void CopyMetadataToRemoteTaskItem()
+        {
+            ProjectItemInstance fromItem = GetItemInstance();
+
+            fromItem.SetMetadata("m1", "v1");
+            fromItem.SetMetadata("m2", "v2");
+
+            AppDomain appDomain = null;
+            try
+            {
+                appDomain = AppDomain.CreateDomain("CopyMetadataToRemoteTaskItem", null, AppDomain.CurrentDomain.SetupInformation);
+                RemoteTaskItemFactory itemFactory = (RemoteTaskItemFactory)appDomain.CreateInstanceFromAndUnwrap(typeof(RemoteTaskItemFactory).Module.FullyQualifiedName, typeof(RemoteTaskItemFactory).FullName);
+
+                ITaskItem toItem = itemFactory.CreateTaskItem();
+
+                ((ITaskItem)fromItem).CopyMetadataTo(toItem);
+
+                Assert.Equal("v1", toItem.GetMetadata("m1"));
+                Assert.Equal("v2", toItem.GetMetadata("m2"));
+            }
+            finally
+            {
+                AppDomain.Unload(appDomain);
+            }
+        }
+#endif
+
         /// <summary>
         /// Set include
         /// </summary>

src/Build/Instance/ProjectItemInstance.cs

@@ -8,6 +8,9 @@
 using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Linq;
+#if FEATURE_APPDOMAIN
+using System.Runtime.Remoting;
+#endif
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.Collections;
 using Microsoft.Build.Construction;
@@ -521,6 +524,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>
@@ -1034,6 +1039,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.
@@ -1371,9 +1389,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);
 
@@ -1391,6 +1407,24 @@ 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;
+                    IEnumerable<KeyValuePair<string, string>> metadataToImport = metadataEnumerable
+                        .Where(metadatum => string.IsNullOrEmpty(destinationItem.GetMetadata(metadatum.Name)))
+                        .Select(metadatum => new KeyValuePair<string, string>(metadatum.Name, GetMetadataEscaped(metadatum.Name)));
+
+#if FEATURE_APPDOMAIN
+                    if (RemotingServices.IsTransparentProxy(destinationItem))
+                    {
+                        // Linq is not serializable so materialize the collection before making the call.
+                        metadataToImport = metadataToImport.ToList();
+                    }
+#endif
+
+                    destinationItemAsMetadataContainer.ImportMetadata(metadataToImport);
+                }
                 else
                 {
                     // OK, most likely the destination item was a Microsoft.Build.Utilities.TaskItem.

src/Framework/IMetadataContainer.cs

@@ -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);
     }
 }

src/Framework/TaskItemData.cs

@@ -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 InvalidOperationException($"{nameof(TaskItemData)} does not support write operations");
+
         public int MetadataCount => Metadata.Count;
 
         public ICollection MetadataNames => (ICollection)Metadata.Keys;

src/Shared/TaskParameter.cs

@@ -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);
+                }
+            }
         }
     }
 }

src/Tasks/AssemblyDependency/ReferenceTable.cs

@@ -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>
@@ -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
@@ -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>

src/Utilities.UnitTests/TaskItem_Tests.cs

@@ -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.  

src/Utilities/TaskItem.cs

@@ -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)