Batch metadata updates for better performance (attempt 2) (#8240)

Improves https://github.com/dotnet/sdk/issues/27738
It is another attempt to introduce changes proposed in https://github.com/dotnet/msbuild/pull/8098, which was reverted due to a bug.

src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs

@@ -6,6 +6,7 @@ using System.Collections;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Globalization;
+using System.Linq;
 using System.Reflection;
 #if FEATURE_APPDOMAIN
 using System.Runtime.Remoting;
@@ -123,7 +124,7 @@ namespace Microsoft.Build.BackEnd
         private List<TaskItem> _remotedTaskItems;
 
         /// <summary>
-        /// We need access to the build component host so that we can get at the 
+        /// We need access to the build component host so that we can get at the
         /// task host node provider when running a task wrapped by TaskHostTask
         /// </summary>
         private readonly IBuildComponentHost _buildComponentHost;
@@ -813,15 +814,15 @@ namespace Microsoft.Build.BackEnd
         /// 2) checks the global task declarations (in *.TASKS in MSbuild bin dir), searching by exact name and task identity parameters
         /// 3) checks the tasks declared by the project, searching by fuzzy match (missing namespace, etc.) and task identity parameters
         /// 4) checks the global task declarations (in *.TASKS in MSbuild bin dir), searching by fuzzy match (missing namespace, etc.) and task identity parameters
-        /// 5) 1-4 again in order without the task identity parameters, to gather additional information for the user (if the task identity 
-        ///    parameters don't match, it is an error, but at least we can return them a more useful error in this case than just "could not 
+        /// 5) 1-4 again in order without the task identity parameters, to gather additional information for the user (if the task identity
+        ///    parameters don't match, it is an error, but at least we can return them a more useful error in this case than just "could not
         ///    find task")
-        /// 
+        ///
         /// The search ordering is meant to reduce the number of assemblies we scan, because loading assemblies can be expensive.
         /// The tasks and assemblies declared by the project are scanned first, on the assumption that if the project declared
         /// them, they are likely used.
-        /// 
-        /// If the set of task identity parameters are defined, only tasks that match that identity are chosen. 
+        ///
+        /// If the set of task identity parameters are defined, only tasks that match that identity are chosen.
         /// </summary>
         /// <returns>The Type of the task, or null if it was not found.</returns>
         private TaskFactoryWrapper FindTaskInRegistry(IDictionary<string, string> taskIdentityParameters)
@@ -874,7 +875,7 @@ namespace Microsoft.Build.BackEnd
                                 taskRuntime ?? XMakeAttributes.MSBuildRuntimeValues.any,
                                 taskArchitecture ?? XMakeAttributes.MSBuildArchitectureValues.any);
 
-                        // if we've logged this error, even though we've found something, we want to act like we didn't.  
+                        // if we've logged this error, even though we've found something, we want to act like we didn't.
                         return null;
                     }
                 }
@@ -1240,8 +1241,8 @@ namespace Microsoft.Build.BackEnd
             bool success;
             IList<TaskItem> finalTaskItems = _batchBucket.Expander.ExpandIntoTaskItemsLeaveEscaped(parameterValue, ExpanderOptions.ExpandAll, parameterLocation);
 
-            // If there were no items, don't change the parameter's value.  EXCEPT if it's marked as a required 
-            // parameter, in which case we made an explicit decision to pass in an empty array.  This is 
+            // If there were no items, don't change the parameter's value.  EXCEPT if it's marked as a required
+            // parameter, in which case we made an explicit decision to pass in an empty array.  This is
             // to avoid project authors having to add Conditions on all their tasks to avoid calling them
             // when a particular item list is empty.  This way, we just call the task with an empty list,
             // the task will loop over an empty list, and return quickly.
@@ -1367,7 +1368,7 @@ namespace Microsoft.Build.BackEnd
 
                             if (outputAsProjectItem != null)
                             {
-                                // The common case -- all items involved are Microsoft.Build.Execution.ProjectItemInstance.TaskItems.  
+                                // The common case -- all items involved are Microsoft.Build.Execution.ProjectItemInstance.TaskItems.
                                 // Furthermore, because that is true, we know by definition that they also implement ITaskItem2.
                                 newItem = new ProjectItemInstance(_projectInstance, outputTargetName, outputAsProjectItem.IncludeEscaped, parameterLocationEscaped);
 
@@ -1377,25 +1378,21 @@ namespace Microsoft.Build.BackEnd
                             {
                                 if (output is ITaskItem2 outputAsITaskItem2)
                                 {
-                                    // Probably a Microsoft.Build.Utilities.TaskItem.  Not quite as good, but we can still preserve escaping. 
+                                    // Probably a Microsoft.Build.Utilities.TaskItem.  Not quite as good, but we can still preserve escaping.
                                     newItem = new ProjectItemInstance(_projectInstance, outputTargetName, outputAsITaskItem2.EvaluatedIncludeEscaped, parameterLocationEscaped);
 
-                                    // It would be nice to be copy-on-write here, but Utilities.TaskItem doesn't know about CopyOnWritePropertyDictionary. 
-                                    foreach (DictionaryEntry entry in outputAsITaskItem2.CloneCustomMetadataEscaped())
-                                    {
-                                        newItem.SetMetadataOnTaskOutput((string)entry.Key, (string)entry.Value);
-                                    }
+                                    // It would be nice to be copy-on-write here, but Utilities.TaskItem doesn't know about CopyOnWritePropertyDictionary.
+                                    newItem.SetMetadataOnTaskOutput(outputAsITaskItem2.CloneCustomMetadataEscaped().Cast<KeyValuePair<string, string>>());
                                 }
                                 else
                                 {
-                                    // Not a ProjectItemInstance.TaskItem or even a ITaskItem2, so we have to fake it.  
-                                    // Setting an item spec expects the escaped value, as does setting metadata. 
+                                    // Not a ProjectItemInstance.TaskItem or even a ITaskItem2, so we have to fake it.
+                                    // Setting an item spec expects the escaped value, as does setting metadata.
                                     newItem = new ProjectItemInstance(_projectInstance, outputTargetName, EscapingUtilities.Escape(output.ItemSpec), parameterLocationEscaped);
 
-                                    foreach (DictionaryEntry entry in output.CloneCustomMetadata())
-                                    {
-                                        newItem.SetMetadataOnTaskOutput((string)entry.Key, EscapingUtilities.Escape((string)entry.Value));
-                                    }
+                                    newItem.SetMetadataOnTaskOutput(output.CloneCustomMetadata()
+                                        .Cast<KeyValuePair<string, string>>()
+                                        .Select(x => new KeyValuePair<string, string>(x.Key, EscapingUtilities.Escape(x.Value))));
                                 }
                             }
 
@@ -1418,7 +1415,7 @@ namespace Microsoft.Build.BackEnd
                     // to store an ITaskItem array in a property, join all the item-specs with semi-colons to make the
                     // property value, and ignore/discard the attributes on the ITaskItems.
                     //
-                    // An empty ITaskItem[] should create a blank value property, for compatibility.                 
+                    // An empty ITaskItem[] should create a blank value property, for compatibility.
                     StringBuilder joinedOutputs = (outputs.Length == 0) ? new StringBuilder() : null;
 
                     foreach (ITaskItem output in outputs)
@@ -1463,7 +1460,7 @@ namespace Microsoft.Build.BackEnd
         /// </summary>
         private void GatherArrayStringAndValueOutputs(bool outputTargetIsItem, string outputTargetName, string[] outputs, ElementLocation parameterLocation, TaskPropertyInfo parameter)
         {
-            // if the task has generated outputs (if it didn't, don't do anything)            
+            // if the task has generated outputs (if it didn't, don't do anything)
             if (outputs != null)
             {
                 if (outputTargetIsItem)
@@ -1494,7 +1491,7 @@ namespace Microsoft.Build.BackEnd
                     // to store an object array in a property, join all the string representations of the objects with
                     // semi-colons to make the property value
                     //
-                    // An empty ITaskItem[] should create a blank value property, for compatibility.                 
+                    // An empty ITaskItem[] should create a blank value property, for compatibility.
                     StringBuilder joinedOutputs = (outputs.Length == 0) ? new StringBuilder() : null;
 
                     foreach (string output in outputs)

src/Build/Instance/ProjectItemInstance.cs

@@ -628,11 +628,11 @@ namespace Microsoft.Build.Execution
         /// which legally have built-in metadata. If necessary we can calculate it on the new items we're making if requested.
         /// We don't copy them too because tasks shouldn't set them (they might become inconsistent)
         /// </summary>
-        internal void SetMetadataOnTaskOutput(string name, string evaluatedValueEscaped)
+        internal void SetMetadataOnTaskOutput(IEnumerable<KeyValuePair<string, string>> items)
         {
             _project.VerifyThrowNotImmutable();
 
-            _taskItem.SetMetadataOnTaskOutput(name, evaluatedValueEscaped);
+            _taskItem.SetMetadataOnTaskOutput(items);
         }
 
         /// <summary>
@@ -1489,8 +1489,8 @@ namespace Microsoft.Build.Execution
             /// </summary>
             public override int GetHashCode()
             {
-                // This is ignore case to ensure that task items whose item specs differ only by 
-                // casing still have the same hash code, since this is used to determine if we have duplicates when 
+                // This is ignore case to ensure that task items whose item specs differ only by
+                // casing still have the same hash code, since this is used to determine if we have duplicates when
                 // we do duplicate removal.
                 return StringComparer.OrdinalIgnoreCase.GetHashCode(ItemSpec);
             }
@@ -1785,6 +1785,18 @@ namespace Microsoft.Build.Execution
                 }
             }
 
+            internal void SetMetadataOnTaskOutput(IEnumerable<KeyValuePair<string, string>> items)
+            {
+                ProjectInstance.VerifyThrowNotImmutable(_isImmutable);
+                _directMetadata ??= new CopyOnWritePropertyDictionary<ProjectMetadataInstance>();
+
+                var metadata = items
+                    .Where(item => !FileUtilities.ItemSpecModifiers.IsDerivableItemSpecModifier(item.Key))
+                    .Select(item => new ProjectMetadataInstance(item.Key, item.Value, true /* may be built-in metadata name */));
+
+                _directMetadata.ImportProperties(metadata);
+            }
+
             /// <summary>
             /// Deep clone this into another TaskItem
             /// </summary>

src/Tasks.UnitTests/RegressionTests.cs

@@ -80,5 +80,29 @@ namespace Microsoft.Build.Tasks.UnitTests
             bool result = project.Build(logger);
             Assert.True(result, "Output:" + Environment.NewLine + logger.FullLog);
         }
+
+        /// <summary>
+        /// Test for https://github.com/dotnet/msbuild/issues/8153
+        /// </summary>
+        [Fact]
+        public void IsWellKnownAttributeValuePreserved()
+        {
+            ObjectModelHelpers.DeleteTempProjectDirectory();
+
+            ObjectModelHelpers.CreateFileInTempProjectDirectory("Myapp.proj", @"
+                <Project ToolsVersion=`msbuilddefaulttoolsversion` xmlns=`msbuildnamespace`>
+                  <Target Name =`Repro`>
+                    <CreateItem Include=`*.txt` AdditionalMetadata=`MyProperty=Identity`>
+                      <Output TaskParameter=`Include` ItemName=`TestItem`/>
+                    </CreateItem>
+                    <Error Text=`@(TestItem)` Condition=""'%(MyProperty)' != 'Identity' ""/>
+                  </Target>
+                </Project>
+                ");
+
+            ObjectModelHelpers.CreateFileInTempProjectDirectory("Foo.txt", "foo");
+            MockLogger logger = new MockLogger(_output);
+            ObjectModelHelpers.BuildTempProjectFileExpectSuccess("Myapp.proj", logger);
+        }
     }
 }