diff --git a/src/Build.UnitTests/BackEnd/Lookup_Tests.cs b/src/Build.UnitTests/BackEnd/Lookup_Tests.cs index f046936a255..8d79c5c2f95 100644 --- a/src/Build.UnitTests/BackEnd/Lookup_Tests.cs +++ b/src/Build.UnitTests/BackEnd/Lookup_Tests.cs @@ -264,14 +264,14 @@ public void Removes() lookup.AddNewItem(item2); // Remove one item - lookup.RemoveItem(item1); + lookup.RemoveItems("i1", [item1]); // We see one item Assert.Single(lookup.GetItems("i1")); Assert.Equal("a2", lookup.GetItems("i1").First().EvaluatedInclude); // Remove the other item - lookup.RemoveItem(item2); + lookup.RemoveItems("i1", [item2]); // We see no items Assert.Empty(lookup.GetItems("i1")); @@ -326,7 +326,7 @@ public void RemoveItemPopulatedInLowerScope() Assert.Single(lookup.GetItems("i1")); // Remove that item - lookup.RemoveItem(item1); + lookup.RemoveItems("i1", [item1]); // We see no items Assert.Empty(lookup.GetItems("i1")); @@ -372,7 +372,7 @@ public void RemoveItemAddedInLowerScope() Assert.Single(lookup.GetItems("i1")); // Remove that item - lookup.RemoveItem(item1); + lookup.RemoveItems("i1", [item1]); // We see no items Assert.Empty(lookup.GetItems("i1")); @@ -824,7 +824,7 @@ public void ModifyItemModifiedInPreviousScope() // Add an item with m=m1 and n=n1 ProjectItemInstance item1 = new ProjectItemInstance(project, "i1", "a2", project.FullPath); item1.SetMetadata("m", "m1"); - lookup.PopulateWithItem(item1); + table1.Add(item1); lookup.EnterScope("x"); @@ -865,7 +865,7 @@ public void ModifyItemTwiceInSameScope1() // Add an item with m=m1 and n=n1 ProjectItemInstance item1 = new ProjectItemInstance(project, "i1", "a2", project.FullPath); item1.SetMetadata("m", "m1"); - lookup.PopulateWithItem(item1); + table1.Add(item1); lookup.EnterScope("x"); @@ -902,7 +902,7 @@ public void ModifyItemTwiceInSameScope2() item1.SetMetadata("m", "m1"); item1.SetMetadata("n", "n1"); item1.SetMetadata("o", "o1"); - lookup.PopulateWithItem(item1); + table1.Add(item1); Lookup.Scope enteredScope = lookup.EnterScope("x"); @@ -1028,7 +1028,7 @@ public void ModifyItemPreviouslyModifiedAndGottenThroughGetItem() // Add an item with m=m1 and n=n1 ProjectItemInstance item1 = new ProjectItemInstance(project, "i1", "a2", project.FullPath); item1.SetMetadata("m", "m1"); - lookup.PopulateWithItem(item1); + table1.Add(item1); Lookup.Scope enteredScope = lookup.EnterScope("x"); @@ -1132,7 +1132,7 @@ public void RemoveItemPreviouslyModifiedAndGottenThroughGetItem() // Add an item with m=m1 and n=n1 ProjectItemInstance item1 = new ProjectItemInstance(project, "i1", "a2", project.FullPath); item1.SetMetadata("m", "m1"); - lookup.PopulateWithItem(item1); + table1.Add(item1); lookup.EnterScope("x"); @@ -1149,7 +1149,7 @@ public void RemoveItemPreviouslyModifiedAndGottenThroughGetItem() ProjectItemInstance item1b = group2.First(); // Remove the item - lookup.RemoveItem(item1b); + lookup.RemoveItems(item1.ItemType, [item1b]); // There's now no items at all ICollection group3 = lookup.GetItems(item1.ItemType); @@ -1186,7 +1186,7 @@ public void RemoveItemFromProjectPreviouslyModifiedAndGottenThroughGetItem() ProjectItemInstance item1b = group2.First(); // Remove the item - lookup.RemoveItem(item1b); + lookup.RemoveItems(item1.ItemType, [item1b]); // There's now no items at all ICollection group3 = lookup.GetItems(item1.ItemType); diff --git a/src/Build.UnitTests/BackEnd/TargetUpToDateChecker_Tests.cs b/src/Build.UnitTests/BackEnd/TargetUpToDateChecker_Tests.cs index d54a77ac90a..a9f0d72ff3a 100644 --- a/src/Build.UnitTests/BackEnd/TargetUpToDateChecker_Tests.cs +++ b/src/Build.UnitTests/BackEnd/TargetUpToDateChecker_Tests.cs @@ -291,7 +291,8 @@ public void MultiInputItemsThatCorrelatesWithMultipleTransformOutputItems() // Even though they were all up to date, we still expect to see an empty marker // so that lookups can correctly *not* find items of that type - Assert.True(changedTargetInputs.HasEmptyMarker("MoreItems")); + Assert.True(changedTargetInputs.ItemTypes.Contains("MoreItems")); + Assert.Empty(changedTargetInputs["MoreItems"]); } [Fact] diff --git a/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs b/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs index ce24deabc92..042928f1b13 100644 --- a/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs +++ b/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs @@ -3,6 +3,7 @@ using System; using System.Collections; +using System.Collections.Frozen; using System.Collections.Generic; using System.IO; using System.Linq; @@ -1282,7 +1283,7 @@ private void InitializeHost(bool throwOnExecute) itemsByName.Add(item2); _twoItems = new ITaskItem[] { new TaskItem(item), new TaskItem(item2) }; - _bucket = new ItemBucket(new Dictionary>().Keys, new Dictionary(), new Lookup(itemsByName, new PropertyDictionary()), 0); + _bucket = new ItemBucket(FrozenSet.Empty, new Dictionary(), new Lookup(itemsByName, new PropertyDictionary()), 0); _bucket.Initialize(null); _host.FindTask(null); _host.InitializeForBatch(talc, _bucket, null); diff --git a/src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs b/src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs index 23d9c14397a..04f0e019bcd 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Frozen; using System.Collections.Generic; using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Collections; @@ -311,6 +312,7 @@ private static List BucketConsumedItems( ErrorUtilities.VerifyThrow(consumedMetadataReferences.Count > 0, "Need item metadata consumed by the batchable object."); var buckets = new List(); + FrozenSet itemNames = itemListsToBeBatched.Keys.ToFrozenSet(MSBuildNameIgnoreCaseComparer.Default); // Get and iterate through the list of item names that we're supposed to batch on. foreach (KeyValuePair> entry in itemListsToBeBatched) @@ -346,7 +348,7 @@ private static List BucketConsumedItems( } else { - matchingBucket = new ItemBucket(itemListsToBeBatched.Keys, itemMetadataValues, lookup, buckets.Count); + matchingBucket = new ItemBucket(itemNames, itemMetadataValues, lookup, buckets.Count); if (loggingContext != null) { matchingBucket.Initialize(loggingContext); diff --git a/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs b/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs index fb5a1921ac8..59b87d9f4a3 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs @@ -281,7 +281,7 @@ private void ExecuteRemove(ProjectItemGroupTaskItemInstance child, ItemBucket bu child.Location); } - bucket.Lookup.RemoveItems(itemsToRemove); + bucket.Lookup.RemoveItems(child.ItemType, itemsToRemove); } } diff --git a/src/Build/BackEnd/Components/RequestBuilder/ItemBucket.cs b/src/Build/BackEnd/Components/RequestBuilder/ItemBucket.cs index 298524b0da5..a2308871cfd 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/ItemBucket.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/ItemBucket.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Frozen; using System.Collections.Generic; using System.Diagnostics; using Microsoft.Build.BackEnd.Logging; @@ -74,7 +75,7 @@ private ItemBucket(Dictionary metadata) /// The to use for the items in the bucket. /// A sequence number indication what order the buckets were created in. internal ItemBucket( - Dictionary>.KeyCollection itemNames, // PERF: directly use the KeyCollection to avoid boxing the enumerator. + FrozenSet itemNames, Dictionary metadata, Lookup lookup, int bucketSequenceNumber) @@ -87,15 +88,9 @@ internal ItemBucket( // Push down the items, so that item changes in this batch are not visible to parallel batches _lookupEntry = _lookup.EnterScope("ItemBucket()"); - // Add empty item groups for each of the item names, so that (unless items are added to this bucket) there are + // Truncate lookups for each of the item names, so that (unless items are added to this bucket) there are // no item types visible in this bucket among the item types being batched on - if (itemNames != null) - { - foreach (string name in itemNames) - { - _lookup.PopulateWithItems(name, new List()); - } - } + _lookup.TruncateLookupsForItemTypes(itemNames); _metadata = metadata; diff --git a/src/Build/BackEnd/Components/RequestBuilder/Lookup.cs b/src/Build/BackEnd/Components/RequestBuilder/Lookup.cs index 8a275483719..b03b2570f19 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/Lookup.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/Lookup.cs @@ -3,6 +3,7 @@ using System; using System.Collections; +using System.Collections.Frozen; using System.Collections.Generic; using System.Linq; using Microsoft.Build.Collections; @@ -58,6 +59,15 @@ internal class Lookup : IPropertyProvider, IItemProvide { #region Fields + /// + /// The first set of items used to create the lookup. + /// + /// + /// This represents the primary table for the outer Scope. This is tracked separately as we don't have control + /// over the incoming implementation dictionary, while Scope uses a simplified item dictionary for perf. + /// + private readonly IItemDictionary _baseItems; + /// /// Ordered stack of scopes used for lookup, where each scope references its parent. /// Each scope contains multiple tables: @@ -94,7 +104,8 @@ internal Lookup(IItemDictionary projectItems, PropertyDicti ErrorUtilities.VerifyThrowInternalNull(projectItems); ErrorUtilities.VerifyThrowInternalNull(properties); - _lookupScopes = new Lookup.Scope(this, "Lookup()", projectItems, properties); + _baseItems = projectItems; + _lookupScopes = new Lookup.Scope(this, "Lookup()", properties); } /// @@ -103,6 +114,7 @@ internal Lookup(IItemDictionary projectItems, PropertyDicti private Lookup(Lookup that) { // Add the same tables from the original. + _baseItems = that._baseItems; _lookupScopes = that._lookupScopes; // Clones need to share an (item)clone table; the batching engine asks for items from the lookup, @@ -117,19 +129,19 @@ private Lookup(Lookup that) // Convenience private properties // "Primary" is the "top" or "innermost" scope // "Secondary" is the next from the top. - private IItemDictionary PrimaryTable + private ItemDictionarySlim PrimaryTable { get { return _lookupScopes.Items; } set { _lookupScopes.Items = value; } } - private ItemDictionary PrimaryAddTable + private ItemDictionarySlim PrimaryAddTable { get { return _lookupScopes.Adds; } set { _lookupScopes.Adds = value; } } - private ItemDictionary PrimaryRemoveTable + private ItemDictionarySlim PrimaryRemoveTable { get { return _lookupScopes.Removes; } set { _lookupScopes.Removes = value; } @@ -147,19 +159,13 @@ private PropertyDictionary PrimaryPropertySets set { _lookupScopes.PropertySets = value; } } - private IItemDictionary SecondaryTable - { - get { return _lookupScopes.Parent.Items; } - set { _lookupScopes.Parent.Items = value; } - } - - private ItemDictionary SecondaryAddTable + private ItemDictionarySlim SecondaryAddTable { get { return _lookupScopes.Parent.Adds; } set { _lookupScopes.Parent.Adds = value; } } - private ItemDictionary SecondaryRemoveTable + private ItemDictionarySlim SecondaryRemoveTable { get { return _lookupScopes.Parent.Removes; } set { _lookupScopes.Parent.Removes = value; } @@ -243,7 +249,7 @@ internal Lookup Clone() internal Lookup.Scope EnterScope(string description) { // We don't create the tables unless we need them - Scope scope = new Scope(this, description, null, null); + Scope scope = new Scope(this, description, null); _lookupScopes = scope; return scope; } @@ -311,16 +317,11 @@ private void MergeScopeIntoNotLastScope() } else { - // When merging remove lists from two or more batches both tables (primary and secondary) may contain identical items. The reason is when removing the items we get the original items rather than a clone - // so the same item may have already been added to the secondary table. If we then try and add the same item from the primary table we will get a duplicate key exception from the - // dictionary. Therefore we must not merge in an item if it already is in the secondary remove table. - foreach (ProjectItemInstance item in PrimaryRemoveTable) - { - if (!SecondaryRemoveTable.Contains(item)) - { - SecondaryRemoveTable.Add(item); - } - } + // When merging remove lists from two or more batches both tables (primary and secondary) may contain + // identical items. The reason is when removing the items we get the original items rather than a clone, + // so the same item may have already been added to the secondary table. + // For perf, we concatenate these lists without checking for duplicates, deferring until used. + SecondaryRemoveTable.ImportItems(PrimaryRemoveTable); } } @@ -376,22 +377,25 @@ private void MergeScopeIntoLastScope() // adds to the world if (PrimaryAddTable != null) { - SecondaryTable ??= new ItemDictionary(); - SecondaryTable.ImportItems(PrimaryAddTable); + foreach (KeyValuePair> kvp in PrimaryAddTable) + { + _baseItems.ImportItemsOfType(kvp.Key, kvp.Value); + } } if (PrimaryRemoveTable != null) { - SecondaryTable ??= new ItemDictionary(); - SecondaryTable.RemoveItems(PrimaryRemoveTable); + foreach (KeyValuePair> kvp in PrimaryRemoveTable) + { + _baseItems.RemoveItems(kvp.Value); + } } if (PrimaryModifyTable != null) { foreach (KeyValuePair> entry in PrimaryModifyTable) { - SecondaryTable ??= new ItemDictionary(); - ApplyModificationsToTable(SecondaryTable, entry.Key, entry.Value); + ApplyModificationsToTable(_baseItems, entry.Key, entry.Value); } } @@ -433,11 +437,6 @@ public ProjectPropertyInstance GetProperty(string name, int startIndex, int endI } } - if (scope.TruncateLookupsAtThisScope) - { - break; - } - scope = scope.Parent; } @@ -467,46 +466,36 @@ public ICollection GetItems(string itemType) // plus the first set of regular items we encounter // minus any removes - List allAdds = null; - List allRemoves = null; + List> allAdds = null; + List> allRemoves = null; Dictionary allModifies = null; ICollection groupFound = null; + // Iterate through all scopes *except* the outer scope. + // The outer scope will always be empty and is currently only used for tracking base properties. + // We use the base items only when no other item group is found. Scope scope = _lookupScopes; - while (scope != null) + while (scope.Parent != null) { // Accumulate adds while we look downwards if (scope.Adds != null) { - ICollection adds = scope.Adds[itemType]; - if (adds.Count != 0) + List adds = scope.Adds[itemType]; + if (adds != null) { - if (allAdds == null) - { - // Use the List(IEnumerable) constructor to avoid an intermediate array allocation. - allAdds = new List(adds); - } - else - { - allAdds.AddRange(adds); - } + allAdds ??= []; + allAdds.Add(adds); } } // Accumulate removes while we look downwards if (scope.Removes != null) { - ICollection removes = scope.Removes[itemType]; - if (removes.Count != 0) + List removes = scope.Removes[itemType]; + if (removes != null) { - if (allRemoves == null) - { - allRemoves = new List(removes); - } - else - { - allRemoves.AddRange(removes); - } + allRemoves ??= []; + allRemoves.Add(removes); } } @@ -534,21 +523,23 @@ public ICollection GetItems(string itemType) if (scope.Items != null) { groupFound = scope.Items[itemType]; - if (groupFound.Count != 0 || scope.Items.HasEmptyMarker(itemType)) + if (groupFound?.Count > 0 + || (scope.ItemTypesToTruncateAtThisScope != null && scope.ItemTypesToTruncateAtThisScope.Contains(itemType))) { // Found a group: we go no further break; } } - if (scope.TruncateLookupsAtThisScope) - { - break; - } - scope = scope.Parent; } + // If we've made it to the root scope, use the original items. + if (groupFound == null && scope.Parent == null) + { + groupFound = _baseItems[itemType]; + } + if ((allAdds == null) && (allRemoves == null) && (allModifies == null)) @@ -561,7 +552,6 @@ public ICollection GetItems(string itemType) } // Set the initial sizes to avoid resizing during import - int itemsTypesCount = 1; // We're only ever importing a single item type int itemsCount = groupFound?.Count ?? 0; // Start with initial set itemsCount += allAdds?.Count ?? 0; // Add all the additions itemsCount -= allRemoves?.Count ?? 0; // Remove the removals @@ -574,11 +564,26 @@ public ICollection GetItems(string itemType) // We can't modify the group, because that might // be visible to other batches; we have to create // a new one. - ItemDictionary result = new ItemDictionary(itemsTypesCount, itemsCount); + List result = new(itemsCount); - if (groupFound != null) + if (groupFound?.Count > 0) { - result.ImportItemsOfType(itemType, groupFound); + if (allRemoves == null) + { + // No removes, so use fast path for ICollection. + result.AddRange(groupFound); + } + else + { + // Otherwise, need to filter any items marked for removal. + foreach (ProjectItemInstance item in groupFound) + { + if (!ShouldRemoveItem(item, allRemoves)) + { + result.Add(item); + } + } + } } // Removes are processed after adds; this means when we remove there's no need to concern ourselves // with the case where the item being removed is in an add table somewhere. The converse case is not possible @@ -586,12 +591,25 @@ public ICollection GetItems(string itemType) // a unique new item. if (allAdds != null) { - result.ImportItemsOfType(itemType, allAdds); - } - - if (allRemoves != null) - { - result.RemoveItems(allRemoves); + foreach (List adds in allAdds) + { + if (allRemoves == null) + { + // No removes, so use fast path for ICollection. + result.AddRange(adds); + } + else + { + // Otherwise, need to filter any items marked for removal. + foreach (ProjectItemInstance item in adds) + { + if (!ShouldRemoveItem(item, allRemoves)) + { + result.Add(item); + } + } + } + } } // Modifies can be processed last; if a modified item was removed, the modify will be ignored @@ -600,42 +618,90 @@ public ICollection GetItems(string itemType) ApplyModifies(result, allModifies); } - return result[itemType]; + return result; + + // Helper to perform a linear search across the removes. + // PERF: This linear search is still cheaper than combining all removes into hashsets and performing a lookup + // due to allocation and hashcode costs, provided that we can quickly filter out false matches. + static bool ShouldRemoveItem(ProjectItemInstance item, List> allRemoves) + { + ITaskItem2 itemAsTaskItem = item; + string evaluatedInclude = itemAsTaskItem.EvaluatedIncludeEscaped; + + foreach (List removes in allRemoves) + { + foreach (ProjectItemInstance remove in removes) + { + // Get access to allocation-free item spec property. + ITaskItem2 removeAsTaskItem = remove; + + // Start with the item spec length as a fast filter for false matches. + if (evaluatedInclude.Length == removeAsTaskItem.EvaluatedIncludeEscaped.Length + && StringComparer.Ordinal.Equals(evaluatedInclude, removeAsTaskItem.EvaluatedIncludeEscaped) + && itemAsTaskItem == removeAsTaskItem) + { + return true; + } + } + } + + return false; + } } /// /// Populates with an item group. This is done before the item lookup is used in this scope. /// Assumes all the items in the group have the same, provided, type. /// Assumes there is no item group of this type in the primary table already. - /// Should be used only by batching buckets, and if no items are passed, - /// explicitly stores a marker for this item type indicating this. + /// Should be used only by batching buckets. /// internal void PopulateWithItems(string itemType, ICollection group) { - PrimaryTable ??= new ItemDictionary(); - ICollection existing = PrimaryTable[itemType]; - ErrorUtilities.VerifyThrow(existing.Count == 0, "Cannot add an itemgroup of this type."); + // The outer scope should never have primary table populated. + MustNotBeOuterScope(); - if (group.Count > 0) - { - PrimaryTable.ImportItemsOfType(itemType, group); - } - else + if (group.Count == 0) { - PrimaryTable.AddEmptyMarker(itemType); + return; } + + PrimaryTable ??= new ItemDictionarySlim(); + ICollection existing = PrimaryTable[itemType]; + ErrorUtilities.VerifyThrow(existing == null, "Cannot add an itemgroup of this type."); + + PrimaryTable.ImportItemsOfType(itemType, group); } /// /// Populates with an item. This is done before the item lookup is used in this scope. /// There may or may not already be a group for it. + /// Should be used only by batching buckets. /// internal void PopulateWithItem(ProjectItemInstance item) { - PrimaryTable ??= new ItemDictionary(); + // The outer scope should never have primary table populated. + MustNotBeOuterScope(); + + PrimaryTable ??= new ItemDictionarySlim(); PrimaryTable.Add(item); } + /// + /// Sets the item types to truncate at the current scope. + /// + /// + /// This can only be setup once per-scope, as the truncate set will be frozen for perf. + /// + internal void TruncateLookupsForItemTypes(ICollection itemTypes) + { + ErrorUtilities.VerifyThrow(_lookupScopes.ItemTypesToTruncateAtThisScope == null, "Cannot add an itemgroup of this type."); + + // Add the item types to truncate at this scope + _lookupScopes.ItemTypesToTruncateAtThisScope = + itemTypes?.ToFrozenSet(MSBuildNameIgnoreCaseComparer.Default) + ?? FrozenSet.Empty; + } + /// /// Apply a property to this scope. /// @@ -670,7 +736,7 @@ internal void AddNewItemsOfItemType(string itemType, ICollection(); + PrimaryAddTable ??= new ItemDictionarySlim(); IEnumerable itemsToAdd = group; if (doNotAddDuplicates) { @@ -721,36 +787,30 @@ internal void AddNewItem(ProjectItemInstance item) #endif // Put in the add table - PrimaryAddTable ??= new ItemDictionary(); + PrimaryAddTable ??= new ItemDictionarySlim(); PrimaryAddTable.Add(item); } /// /// Remove a bunch of items from this scope /// - internal void RemoveItems(IEnumerable items) - { - foreach (ProjectItemInstance item in items) - { - RemoveItem(item); - } - } - - /// - /// Remove an item from this scope - /// - internal void RemoveItem(ProjectItemInstance item) + internal void RemoveItems(string itemType, ICollection items) { // Removing from outer scope could be easily implemented, but our code does not do it at present MustNotBeOuterScope(); - item = RetrieveOriginalFromCloneTable(item); + if (items.Count == 0) + { + return; + } + + PrimaryRemoveTable ??= new ItemDictionarySlim(); + PrimaryRemoveTable.EnsureCapacityForItemType(itemType, items.Count); - // Put in the remove table - PrimaryRemoveTable ??= new ItemDictionary(); - PrimaryRemoveTable.Add(item); + IEnumerable itemsToRemove = items.Select(RetrieveOriginalFromCloneTable); + PrimaryRemoveTable.ImportItemsOfType(itemType, itemsToRemove); - // No need to remove this item from the primary add table if it's + // No need to remove these items from the primary add table if it's // already there -- we always apply removes after adds, so that add // will be reversed anyway. } @@ -820,7 +880,7 @@ internal void ModifyItems(string itemType, ICollection grou /// Apply modifies to a temporary result group. /// Items to be modified are virtual-cloned so the original isn't changed. /// - private void ApplyModifies(ItemDictionary result, Dictionary allModifies) + private void ApplyModifies(List result, Dictionary allModifies) { // Clone, because we're modifying actual items, and this would otherwise be visible to other batches, // and would be "published" even if a target fails. @@ -829,20 +889,18 @@ private void ApplyModifies(ItemDictionary result, Dictionar // Store the clone, in case we're asked to modify or remove it later (we will record it against the real item) _cloneTable ??= new Dictionary(); - foreach (var modify in allModifies) + // Iterate through the result group while replacing any items that have pending modifications. + for (int i = 0; i < result.Count; i++) { - ProjectItemInstance originalItem = modify.Key; - - if (result.Contains(originalItem)) + ProjectItemInstance originalItem = result[i]; + if (allModifies.TryGetValue(originalItem, out MetadataModifications modificationsToApply)) { - var modificationsToApply = modify.Value; - // Modify the cloned item and replace the original with it. - ProjectItemInstance cloneItem = modify.Key.DeepClone(); + ProjectItemInstance cloneItem = originalItem.DeepClone(); ApplyMetadataModificationsToItem(modificationsToApply, cloneItem); - result.Replace(originalItem, cloneItem); + result[i] = cloneItem; // This will be null if the item wasn't in the result group, ie, it had been removed after being modified ErrorUtilities.VerifyThrow(!_cloneTable.ContainsKey(cloneItem), "Should be new, not already in table!"); @@ -972,11 +1030,11 @@ private void MergeModificationsIntoModificationTable(Dictionary /// Verify item is not in the table /// - private void MustNotBeInTable(ItemDictionary table, ProjectItemInstance item) + private void MustNotBeInTable(ItemDictionarySlim table, ProjectItemInstance item) { - if (table?.ItemTypes.Contains(item.ItemType) == true) + if (table?.ContainsKey(item.ItemType) == true) { - ICollection tableOfItemsOfSameType = table[item.ItemType]; + List tableOfItemsOfSameType = table[item.ItemType]; if (tableOfItemsOfSameType != null) { ErrorUtilities.VerifyThrow(!tableOfItemsOfSameType.Contains(item), "Item should not be in table"); @@ -1025,7 +1083,7 @@ private void MustNotBeInAnyTables(ProjectItemInstance item) /// private void MustNotBeOuterScope() { - ErrorUtilities.VerifyThrow(_lookupScopes.Count > 1, "Operation in outer scope not supported"); + ErrorUtilities.VerifyThrow(_lookupScopes.Parent != null, "Operation in outer scope not supported"); } #endregion @@ -1335,17 +1393,17 @@ internal class Scope /// /// Contains all of the original items at this level in the Lookup /// - private IItemDictionary _items; + private ItemDictionarySlim _items; /// /// Contains all of the items which have been added at this level in the Lookup /// - private ItemDictionary _adds; + private ItemDictionarySlim _adds; /// /// Contails all of the items which have been removed at this level in the Lookup /// - private ItemDictionary _removes; + private ItemDictionarySlim _removes; /// /// Contains all of the metadata which has been changed for items at this level in the Lookup. @@ -1363,11 +1421,6 @@ internal class Scope /// private PropertyDictionary _propertySets; - /// - /// The managed thread id which entered this scope. - /// - private int _threadIdThatEnteredScope; - /// /// A description of this scope, for error checking /// @@ -1380,22 +1433,21 @@ internal class Scope /// /// Indicates whether or not further levels in the Lookup should be consulted beyond this one - /// to find the actual value for the desired item or property. + /// to find the actual value for the desired item type. /// - private bool _truncateLookupsAtThisScope; + private FrozenSet _itemTypesToTruncateAtThisScope; - internal Scope(Lookup lookup, string description, IItemDictionary items, PropertyDictionary properties) + internal Scope(Lookup lookup, string description, PropertyDictionary properties) { _owningLookup = lookup; _description = description; - _items = items; + _items = null; _adds = null; _removes = null; _modifies = null; _properties = properties; _propertySets = null; - _threadIdThatEnteredScope = Environment.CurrentManagedThreadId; - _truncateLookupsAtThisScope = false; + _itemTypesToTruncateAtThisScope = null; Parent = lookup._lookupScopes; } @@ -1429,7 +1481,7 @@ internal int Count /// include adds or removes unless it's the table in /// the outermost scope. /// - internal IItemDictionary Items + internal ItemDictionarySlim Items { get { return _items; } set { _items = value; } @@ -1437,7 +1489,7 @@ internal IItemDictionary Items /// /// Adds made in this scope or above. /// - internal ItemDictionary Adds + internal ItemDictionarySlim Adds { get { return _adds; } set { _adds = value; } @@ -1445,7 +1497,7 @@ internal ItemDictionary Adds /// /// Removes made in this scope or above. /// - internal ItemDictionary Removes + internal ItemDictionarySlim Removes { get { return _removes; } set { _removes = value; } @@ -1476,19 +1528,14 @@ internal PropertyDictionary PropertySets get { return _propertySets; } set { _propertySets = value; } } + /// - /// ID of thread owning this scope - /// - internal int ThreadIdThatEnteredScope - { - get { return _threadIdThatEnteredScope; } - } - /// - /// Whether to stop lookups going beyond this scope downwards + /// Whether to stop lookups going beyond this scope downwards for item types in the set. /// - internal bool TruncateLookupsAtThisScope + internal FrozenSet ItemTypesToTruncateAtThisScope { - get { return _truncateLookupsAtThisScope; } + get { return _itemTypesToTruncateAtThisScope; } + set { _itemTypesToTruncateAtThisScope = value; } } /// diff --git a/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs b/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs index 39673fa0ace..b97e92a03ce 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs @@ -504,6 +504,10 @@ internal async Task ExecuteTarget(ITaskBuilder taskBuilder, BuildRequestEntry re // as a result we need separate sets of item and property collections to track changes if (dependencyResult == DependencyAnalysisResult.IncrementalBuild) { + // Ensure that these lookups stop at the current scope, regardless of whether items were added. + lookupForInference.TruncateLookupsForItemTypes(upToDateTargetInputs.ItemTypes); + lookupForExecution.TruncateLookupsForItemTypes(changedTargetInputs.ItemTypes); + // subset the relevant items to those that are up-to-date foreach (string itemType in upToDateTargetInputs.ItemTypes) { diff --git a/src/Build/BackEnd/Components/RequestBuilder/TargetUpToDateChecker.cs b/src/Build/BackEnd/Components/RequestBuilder/TargetUpToDateChecker.cs index 58e250f6c97..16afcef0c2e 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TargetUpToDateChecker.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TargetUpToDateChecker.cs @@ -698,14 +698,14 @@ private DependencyAnalysisResult PerformDependencyAnalysisIfCorrelatedInputsOutp // them. if (!changedTargetInputs.ItemTypes.Contains(inputItems[0].ItemType)) { - changedTargetInputs.AddEmptyMarker(inputItems[0].ItemType); + changedTargetInputs.ImportItemsOfType(inputItems[0].ItemType, Array.Empty()); } // We need to perform the same operation on the up-to-date side // too. if (!upToDateTargetInputs.ItemTypes.Contains(inputItems[0].ItemType)) { - upToDateTargetInputs.AddEmptyMarker(inputItems[0].ItemType); + upToDateTargetInputs.ImportItemsOfType(inputItems[0].ItemType, Array.Empty()); } } } diff --git a/src/Build/Collections/IItemDictionary.cs b/src/Build/Collections/IItemDictionary.cs index cca36bf088d..5977f324617 100644 --- a/src/Build/Collections/IItemDictionary.cs +++ b/src/Build/Collections/IItemDictionary.cs @@ -65,12 +65,6 @@ internal interface IItemDictionary : IEnumerable, IItemProvider /// void Add(T projectItem); - /// - /// Adds each new item to the collection, at the - /// end of the list of other items with the same key. - /// - void AddRange(IEnumerable projectItems); - /// /// Removes an item, if it is in the collection. /// Returns true if it was found, otherwise false. @@ -78,18 +72,9 @@ internal interface IItemDictionary : IEnumerable, IItemProvider /// /// If a list is emptied, removes the list from the enclosing collection /// so it can be garbage collected. - /// + /// bool Remove(T projectItem); - /// - /// Replaces an existing item with a new item. This is necessary to preserve the original ordering semantics of Lookup.GetItems - /// when items with metadata modifications are being returned. See Dev10 bug 480737. - /// If the item is not found, does nothing. - /// - /// The item to be replaced. - /// The replacement item. - void Replace(T existingItem, T newItem); - /// /// Add the set of items specified to this dictionary. /// @@ -109,21 +94,5 @@ internal interface IItemDictionary : IEnumerable, IItemProvider /// /// An enumerator over the items to remove. void RemoveItems(IEnumerable other); - - /// - /// Special method used for batching buckets. - /// Adds an explicit marker indicating there are no items for the specified item type. - /// In the general case, this is redundant, but batching buckets use this to indicate that they are - /// batching over the item type, but their bucket does not contain items of that type. - /// See HasEmptyMarker. - /// - void AddEmptyMarker(string itemType); - - /// - /// Special method used for batching buckets. - /// Lookup can call this to see whether there was an explicit marker placed indicating that - /// there are no items of this type. See comment on AddEmptyMarker. - /// - bool HasEmptyMarker(string itemType); } } diff --git a/src/Build/Collections/ItemDictionary.cs b/src/Build/Collections/ItemDictionary.cs index 9604a630507..3a35b70ccce 100644 --- a/src/Build/Collections/ItemDictionary.cs +++ b/src/Build/Collections/ItemDictionary.cs @@ -6,7 +6,9 @@ using System.Collections.Generic; using System.Diagnostics; using Microsoft.Build.Evaluation; +#if DEBUG using Microsoft.Build.Shared; +#endif #nullable disable @@ -34,8 +36,6 @@ internal sealed class ItemDictionary : ICollection, IItemDictionary { /// /// Dictionary of item lists used as a backing store. - /// An empty list should never be stored in here unless it is an empty marker. - /// See AddEmptyMarker. /// This collection provides quick access to the ordered set of items of a particular type. /// private readonly Dictionary> _itemLists; @@ -247,17 +247,6 @@ public void Add(T projectItem) } } - public void AddRange(IEnumerable projectItems) - { - lock (_itemLists) - { - foreach (var projectItem in projectItems) - { - AddProjectItem(projectItem); - } - } - } - /// /// Removes an item, if it is in the collection. /// Returns true if it was found, otherwise false. @@ -290,35 +279,20 @@ public bool Remove(T projectItem) } /// - /// Replaces an exsting item with a new item. This is necessary to preserve the original ordering semantics of Lookup.GetItems - /// when items with metadata modifications are being returned. See Dev10 bug 480737. - /// If the item is not found, does nothing. + /// Add the set of items specified to this dictionary /// - /// The item to be replaced. - /// The replacement item. - public void Replace(T existingItem, T newItem) + /// An enumerator over the items to remove. + public void ImportItems(IEnumerable other) { - ErrorUtilities.VerifyThrow(existingItem.Key == newItem.Key, "Cannot replace an item {0} with an item {1} with a different name.", existingItem.Key, newItem.Key); lock (_itemLists) { - if (_nodes.TryGetValue(existingItem, out LinkedListNode node)) + foreach (var projectItem in other) { - node.Value = newItem; - _nodes.Remove(existingItem); - _nodes.Add(newItem, node); + AddProjectItem(projectItem); } } } - /// - /// Add the set of items specified to this dictionary - /// - /// An enumerator over the items to remove. - public void ImportItems(IEnumerable other) - { - AddRange(other); - } - /// /// Add the set of items specified, all sharing an item type, to this dictionary. /// @@ -359,40 +333,6 @@ public void RemoveItems(IEnumerable other) } } - /// - /// Special method used for batching buckets. - /// Adds an explicit marker indicating there are no items for the specified item type. - /// In the general case, this is redundant, but batching buckets use this to indicate that they are - /// batching over the item type, but their bucket does not contain items of that type. - /// See HasEmptyMarker. - /// - public void AddEmptyMarker(string itemType) - { - lock (_itemLists) - { - ErrorUtilities.VerifyThrow(!_itemLists.ContainsKey(itemType), "Should be none"); - _itemLists.Add(itemType, new LinkedList()); - } - } - - /// - /// Special method used for batching buckets. - /// Lookup can call this to see whether there was an explicit marker placed indicating that - /// there are no items of this type. See comment on AddEmptyMarker. - /// - public bool HasEmptyMarker(string itemType) - { - lock (_itemLists) - { - if (_itemLists.TryGetValue(itemType, out LinkedList list) && list.Count == 0) - { - return true; - } - - return false; - } - } - private void AddProjectItem(T projectItem) { if (!_itemLists.TryGetValue(projectItem.Key, out LinkedList list)) diff --git a/src/Build/Collections/ItemDictionarySlim.cs b/src/Build/Collections/ItemDictionarySlim.cs new file mode 100644 index 00000000000..fb425487e4e --- /dev/null +++ b/src/Build/Collections/ItemDictionarySlim.cs @@ -0,0 +1,113 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections; +using System.Collections.Generic; +using Microsoft.Build.Execution; + +namespace Microsoft.Build.Collections +{ + /// + /// A simple implementation of ItemDictionary intended for populating Scope tables and merging adds/removes, + /// without the overhead of locking and additional lookups. + /// + internal sealed class ItemDictionarySlim : IEnumerable>> + { + private readonly Dictionary> _itemLists; + + public ItemDictionarySlim() => + _itemLists = new Dictionary>(MSBuildNameIgnoreCaseComparer.Default); + + /// + /// Gets all items of the given type, or null if there are none. + /// + public List? this[string itemType] => + _itemLists.TryGetValue(itemType, out List? itemsOfType) ? itemsOfType : null; + + /// + /// Returns true if there are any items of the given type. + /// + public bool ContainsKey(string itemType) => _itemLists.ContainsKey(itemType); + + /// + /// Adds a single item to the matching list for its type. + /// + public void Add(ProjectItemInstance projectItem) + { + if (!_itemLists.TryGetValue(projectItem.ItemType, out List? list)) + { + list = []; + _itemLists[projectItem.ItemType] = list; + } + + list.Add(projectItem); + } + + /// + /// Imports and merges all items from an existing item dictionary, appending to any existing list. + /// + /// + /// This should only be called when merging between scopes, as it may take list references from the other dictionary. + /// This is safe since we create these internal lists, and the owning Scope will always be discarded after a merge. + /// + public void ImportItems(ItemDictionarySlim other) + { + foreach (KeyValuePair> kvp in other._itemLists) + { + string itemType = kvp.Key; + List itemsToAdd = kvp.Value; + + if (_itemLists.TryGetValue(itemType, out List? list)) + { + // There are already items of this type, so append to the existing list + list.AddRange(itemsToAdd); + } + else + { + // Otherwise, steal the reference instead of copying items out. + _itemLists[itemType] = itemsToAdd; + } + } + } + + /// + /// Imports and merges all items of the given type, appending to any existing list. + /// + public void ImportItemsOfType(string itemType, IEnumerable items) + { + if (!_itemLists.TryGetValue(itemType, out List? list)) + { + list = []; + _itemLists[itemType] = list; + } + + list.AddRange(items); + } + + /// + /// Sets the capacity for the item list matching the given type. + /// + /// + /// Useful for dealing with IEnumerable if we know the upper bound or estimate of items to be added. + /// + internal void EnsureCapacityForItemType(string itemType, int capacity) + { + if (!_itemLists.TryGetValue(itemType, out List? list)) + { + list = new List(capacity); + _itemLists[itemType] = list; + } + else if (capacity > list.Capacity) + { + // Conditional since List.Capacity will throw if set less than its current value. + list.Capacity = capacity; + } + } + + public Dictionary>.Enumerator GetEnumerator() => _itemLists.GetEnumerator(); + + IEnumerator>> IEnumerable>>.GetEnumerator() => GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } +} diff --git a/src/Build/Instance/ImmutableProjectCollections/ImmutableItemDictionary.cs b/src/Build/Instance/ImmutableProjectCollections/ImmutableItemDictionary.cs index 00d34f295f2..a05ddb25abb 100644 --- a/src/Build/Instance/ImmutableProjectCollections/ImmutableItemDictionary.cs +++ b/src/Build/Instance/ImmutableProjectCollections/ImmutableItemDictionary.cs @@ -63,12 +63,6 @@ public ICollection this[string itemType] /// public void Add(T projectItem) => throw new NotSupportedException(); - /// - public void AddEmptyMarker(string itemType) => throw new NotSupportedException(); - - /// - public void AddRange(IEnumerable projectItems) => throw new NotSupportedException(); - /// public void Clear() => throw new NotSupportedException(); @@ -155,9 +149,6 @@ public ICollection GetItems(string itemType) return Array.Empty(); } - /// - public bool HasEmptyMarker(string itemType) => _itemsByType.Values.Any(list => list.Count == 0); - /// public void ImportItems(IEnumerable other) => throw new NotSupportedException(); @@ -170,9 +161,6 @@ public ICollection GetItems(string itemType) /// public void RemoveItems(IEnumerable other) => throw new NotSupportedException(); - /// - public void Replace(T existingItem, T newItem) => throw new NotSupportedException(); - private sealed class ListConverter : ICollection { private readonly ICollection _list; diff --git a/src/Build/Microsoft.Build.csproj b/src/Build/Microsoft.Build.csproj index 749b54b3c63..c4cf679eeeb 100644 --- a/src/Build/Microsoft.Build.csproj +++ b/src/Build/Microsoft.Build.csproj @@ -388,6 +388,7 @@ +