diff --git a/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs b/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs index 9539898ed8b..8d9406a91cb 100644 --- a/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs +++ b/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs @@ -2604,6 +2604,92 @@ public void LastUpdateWins() ObjectModelHelpers.AssertItemHasMetadata(expectedUpdate, items[0]); } + [Theory] + [InlineData("abc", "def", "abc")] + [InlineData("abc", "de*", "abc")] + [InlineData("a*c", "def", "abc")] + [InlineData("abc", "def", "*bc")] + [InlineData("abc", "d*f", "*bc")] + [InlineData("*c", "d*f", "*bc")] + [InlineData("a*", "d*", "abc")] + public void UpdatesProceedInOrder(string first, string second, string third) + { + string contents = $@" + + m1_contents + + + m1_contents + + + first + + + second + + + third + +"; + IList items = ObjectModelHelpers.GetItemsFromFragment(contents, allItems: true); + Dictionary expectedUpdatei = new Dictionary + { + {"m1", "third" } + }; + Dictionary expectedUpdatej = new Dictionary + { + {"m1", "second" } + }; + + ObjectModelHelpers.AssertItemHasMetadata(expectedUpdatei, items[0]); + ObjectModelHelpers.AssertItemHasMetadata(expectedUpdatej, items[1]); + } + + [Fact] + public void UpdatingIndividualItemsProceedsInOrder() + { + string contents = @" + + m1_contents + + + second + + + third + + + fourth + + + + sixth + + + + seventh + + + + eighth + + +"; + IList items = ObjectModelHelpers.GetItemsFromFragment(contents, allItems: true); + ObjectModelHelpers.AssertItemHasMetadata("m1", "second", items[3]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "third", items[4]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "fourth", items[5]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "sixth", items[6]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "sixth", items[7]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "sixth", items[8]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "sixth", items[9]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "seventh", items[10]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "sixth", items[11]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "sixth", items[12]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "seventh", items[13]); + ObjectModelHelpers.AssertItemHasMetadata("m1", "eighth", items[14]); + } + [Fact] public void UpdateWithNoMetadataShouldNotAffectItems() { @@ -2850,6 +2936,25 @@ public void UpdateFromReferencedItemShouldBeCaseInsensitive() ObjectModelHelpers.AssertItemHasMetadata(expectedMetadataA, items[1]); } + [Fact] + public void UpdateMetadataWithoutItemReferenceShouldBeCaseInsensitive() + { + string content = @" + + + "; + + IList items = ObjectModelHelpers.GetItemsFromFragment(content, true); + + var expectedMetadataA = new Dictionary + { + {"m", "m1_contents"}, + }; + + items[0].ItemType.ShouldBe("to"); + ObjectModelHelpers.AssertItemHasMetadata(expectedMetadataA, items[0]); + } + [Fact] public void UndeclaredQualifiedMetadataReferencesInUpdateShouldResolveToEmptyStrings() { diff --git a/src/Build/Evaluation/ItemSpec.cs b/src/Build/Evaluation/ItemSpec.cs index ba38ae601d5..2756ccb9c37 100644 --- a/src/Build/Evaluation/ItemSpec.cs +++ b/src/Build/Evaluation/ItemSpec.cs @@ -414,7 +414,7 @@ internal abstract class ItemSpecFragment /// /// Path of the project the itemspec is coming from /// - protected string ProjectDirectory { get; } + internal string ProjectDirectory { get; } // not a Lazy to reduce memory private ref FileSpecMatcherTester FileMatcher diff --git a/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs index 146e15c4df2..9934759efbf 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs @@ -30,7 +30,8 @@ private abstract class LazyItemOperation : IItemOperation // This is used only when evaluating an expression, which instantiates // the items and then removes them protected readonly IItemFactory _itemFactory; - + internal ItemSpec Spec => _itemSpec; + protected LazyItemOperation(OperationBuilder builder, LazyItemEvaluator lazyEvaluator) { _itemElement = builder.ItemElement; diff --git a/src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs index b1e13d2ed83..5423bcf0286 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs @@ -15,6 +15,9 @@ internal partial class LazyItemEvaluator class UpdateOperation : LazyItemOperation { private readonly ImmutableList _metadata; + private ImmutableList.Builder _itemsToUpdate = null; + private ItemSpecMatchesItem _matchItemSpec = null; + private bool? _needToExpandMetadataForEachItem = null; public UpdateOperation(OperationBuilderWithMetadata builder, LazyItemEvaluator lazyEvaluator) : base(builder, lazyEvaluator) @@ -43,23 +46,77 @@ protected override void ApplyImpl(ImmutableList.Builder listBuilder, I return; } - ItemSpecMatchesItem matchItemspec; - bool? needToExpandMetadataForEachItem = null; + SetMatchItemSpec(); + _itemsToUpdate ??= ImmutableList.CreateBuilder(); + _itemsToUpdate.Clear(); + for (int i = 0; i < listBuilder.Count; i++) + { + var itemData = listBuilder[i]; + + var matchResult = _matchItemSpec(_itemSpec, itemData.Item); + + if (matchResult.IsMatch) + { + listBuilder[i] = UpdateItem(listBuilder[i], matchResult.CapturedItemsFromReferencedItemTypes); + } + } + + DecorateItemsWithMetadata(_itemsToUpdate.ToImmutableList(), _metadata, _needToExpandMetadataForEachItem); + } + + /// + /// Apply the Update operation to the item if it matches. + /// + /// The item to check for a match. + /// The updated item. + internal ItemData UpdateItem(ItemData item) + { + if (_conditionResult) + { + SetMatchItemSpec(); + _itemsToUpdate ??= ImmutableList.CreateBuilder(); + _itemsToUpdate.Clear(); + MatchResult matchResult = _matchItemSpec(_itemSpec, item.Item); + if (matchResult.IsMatch) + { + ItemData clonedData = UpdateItem(item, matchResult.CapturedItemsFromReferencedItemTypes); + DecorateItemsWithMetadata(_itemsToUpdate.ToImmutableList(), _metadata, _needToExpandMetadataForEachItem); + return clonedData; + } + } + return item; + } + + private ItemData UpdateItem(ItemData item, Dictionary capturedItemsFromReferencedItemTypes) + { + // items should be deep immutable, so clone and replace items before mutating them + // otherwise, with GetItems caching enabled, the mutations would leak into the cache causing + // future operations to mutate the state of past operations + ItemData clonedData = item.Clone(_itemFactory, _itemElement); + _itemsToUpdate.Add(new ItemBatchingContext(clonedData.Item, capturedItemsFromReferencedItemTypes)); + return clonedData; + } + + /// + /// This sets the function used to determine whether an item matches an item spec. + /// + private void SetMatchItemSpec() + { if (ItemspecContainsASingleBareItemReference(_itemSpec, _itemElement.ItemType)) { // Perf optimization: If the Update operation references itself (e.g. ) // then all items are updated and matching is not necessary - matchItemspec = (itemSpec, item) => new MatchResult(true, null); + _matchItemSpec = (itemSpec, item) => new MatchResult(true, null); } else if (ItemSpecContainsItemReferences(_itemSpec) - && QualifiedMetadataReferencesExist(_metadata, out needToExpandMetadataForEachItem) + && QualifiedMetadataReferencesExist(_metadata, out _needToExpandMetadataForEachItem) && !Traits.Instance.EscapeHatches.DoNotExpandQualifiedMetadataInUpdateOperation) { - var itemReferenceFragments = _itemSpec.Fragments.OfType.ItemExpressionFragment>().ToArray(); - var nonItemReferenceFragments = _itemSpec.Fragments.Where(f => !(f is ItemSpec.ItemExpressionFragment)).ToArray(); + var itemReferenceFragments = _itemSpec.Fragments.OfType.ItemExpressionFragment>().ToArray(); + var nonItemReferenceFragments = _itemSpec.Fragments.Where(f => !(f is ItemSpec.ItemExpressionFragment)).ToArray(); - matchItemspec = (itemSpec, item) => + _matchItemSpec = (itemSpec, item) => { var isMatch = nonItemReferenceFragments.Any(f => f.IsMatch(item.EvaluatedInclude)); Dictionary capturedItemsFromReferencedItemTypes = null; @@ -84,30 +141,8 @@ protected override void ApplyImpl(ImmutableList.Builder listBuilder, I } else { - matchItemspec = (itemSpec, item) => new MatchResult(itemSpec.MatchesItem(item), null); - } - - var itemsToUpdate = ImmutableList.CreateBuilder(); - - for (int i = 0; i < listBuilder.Count; i++) - { - var itemData = listBuilder[i]; - - var matchResult = matchItemspec(_itemSpec, itemData.Item); - - if (matchResult.IsMatch) - { - // items should be deep immutable, so clone and replace items before mutating them - // otherwise, with GetItems caching enabled, the mutations would leak into the cache causing - // future operations to mutate the state of past operations - var clonedItemData = listBuilder[i].Clone(_itemFactory, _itemElement); - listBuilder[i] = clonedItemData; - - itemsToUpdate.Add(new ItemBatchingContext(clonedItemData.Item, matchResult.CapturedItemsFromReferencedItemTypes)); - } + _matchItemSpec = (itemSpec, item) => new MatchResult(itemSpec.MatchesItem(item), null); } - - DecorateItemsWithMetadata(itemsToUpdate.ToImmutableList(), _metadata, needToExpandMetadataForEachItem); } private bool QualifiedMetadataReferencesExist(ImmutableList metadata, out bool? needToExpandMetadataForEachItem) diff --git a/src/Build/Evaluation/LazyItemEvaluator.cs b/src/Build/Evaluation/LazyItemEvaluator.cs index 64df2a7402f..e5f2a72f61a 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.cs @@ -151,7 +151,7 @@ public ItemData Clone(IItemFactory itemFactory, ProjectItemElement initial private class MemoizedOperation : IItemOperation { - public IItemOperation Operation { get; } + public LazyItemOperation Operation { get; } private Dictionary, ImmutableList> _cache; private bool _isReferenced; @@ -159,7 +159,7 @@ private class MemoizedOperation : IItemOperation private int _applyCalls; #endif - public MemoizedOperation(IItemOperation operation) + public MemoizedOperation(LazyItemOperation operation) { Operation = operation; } @@ -291,6 +291,14 @@ public ImmutableList.Builder GetItemData(ImmutableHashSet glob } } + /// + /// Applies uncached item operations (include, remove, update) in order. Since Remove effectively overwrites Include or Update, + /// Remove operations are preprocessed (adding to globsToIgnore) to create a longer list of globs we don't need to process + /// properly because we know they will be removed. Update operations are batched as much as possible, meaning rather + /// than being applied immediately, they are combined into a dictionary of UpdateOperations that need to be applied. This + /// is to optimize the case in which as series of UpdateOperations, each of which affects a single ItemSpec, are applied to all + /// items in the list, leading to a quadratic-time operation. + /// private static ImmutableList.Builder ComputeItems(LazyItemList lazyItemList, ImmutableHashSet globsToIgnore) { // Stack of operations up to the first one that's cached (exclusive) @@ -315,13 +323,9 @@ private static ImmutableList.Builder ComputeItems(LazyItemList lazyIte // If this is a remove operation, then add any globs that will be removed // to a list of globs to ignore in previous operations - var removeOperation = currentList._memoizedOperation.Operation as RemoveOperation; - if (removeOperation != null) + if (currentList._memoizedOperation.Operation is RemoveOperation removeOperation) { - if (globsToIgnoreStack == null) - { - globsToIgnoreStack = new Stack>(); - } + globsToIgnoreStack ??= new Stack>(); var globsToIgnoreForPreviousOperations = removeOperation.GetRemovedGlobs(); foreach (var globToRemove in globsToIgnoreFromFutureOperations) @@ -342,15 +346,65 @@ private static ImmutableList.Builder ComputeItems(LazyItemList lazyIte ImmutableHashSet currentGlobsToIgnore = globsToIgnoreStack == null ? globsToIgnore : globsToIgnoreStack.Peek(); + Dictionary itemsWithNoWildcards = new Dictionary(StringComparer.OrdinalIgnoreCase); + bool addedToBatch = false; + // Walk back down the stack of item lists applying operations while (itemListStack.Count > 0) { var currentList = itemListStack.Pop(); + if (currentList._memoizedOperation.Operation is UpdateOperation op) + { + bool addToBatch = true; + int i; + // The TextFragments are things like abc.def or x*y.*z. + for (i = 0; i < op.Spec.Fragments.Count; i++) + { + ItemSpecFragment frag = op.Spec.Fragments[i]; + if (MSBuildConstants.CharactersForExpansion.Any(frag.TextFragment.Contains)) + { + // Fragment contains wild cards, items, or properties. Cannot batch over it using a dictionary. + addToBatch = false; + break; + } + + string fullPath = FileUtilities.GetFullPath(frag.TextFragment, frag.ProjectDirectory); + if (itemsWithNoWildcards.ContainsKey(fullPath)) + { + // Another update will already happen on this path. Make that happen before evaluating this one. + addToBatch = false; + break; + } + else + { + itemsWithNoWildcards.Add(fullPath, op); + } + } + if (!addToBatch) + { + // We found a wildcard. Remove any fragments associated with the current operation and process them later. + for (int j = 0; j < i; j++) + { + itemsWithNoWildcards.Remove(currentList._memoizedOperation.Operation.Spec.Fragments[j].TextFragment); + } + } + else + { + addedToBatch = true; + continue; + } + } + + if (addedToBatch) + { + addedToBatch = false; + ProcessNonWildCardItemUpdates(itemsWithNoWildcards, items); + } + // If this is a remove operation, then it could modify the globs to ignore, so pop the potentially // modified entry off the stack of globs to ignore - var removeOperation = currentList._memoizedOperation.Operation as RemoveOperation; - if (removeOperation != null) + if (currentList._memoizedOperation.Operation is RemoveOperation) { globsToIgnoreStack.Pop(); currentGlobsToIgnore = globsToIgnoreStack.Count == 0 ? globsToIgnore : globsToIgnoreStack.Peek(); @@ -359,9 +413,30 @@ private static ImmutableList.Builder ComputeItems(LazyItemList lazyIte currentList._memoizedOperation.Apply(items, currentGlobsToIgnore); } + // We finished looping through the operations. Now process the final batch if necessary. + ProcessNonWildCardItemUpdates(itemsWithNoWildcards, items); + return items; } + private static void ProcessNonWildCardItemUpdates(Dictionary itemsWithNoWildcards, ImmutableList.Builder items) + { +#if DEBUG + ErrorUtilities.VerifyThrow(itemsWithNoWildcards.All(fragment => !MSBuildConstants.CharactersForExpansion.Any(fragment.Key.Contains)), $"{nameof(itemsWithNoWildcards)} should not contain any text fragments with wildcards."); +#endif + if (itemsWithNoWildcards.Count > 0) + { + for (int i = 0; i < items.Count; i++) + { + if (itemsWithNoWildcards.TryGetValue(FileUtilities.GetFullPath(items[i].Item.EvaluatedInclude, items[i].Item.ProjectDirectory), out UpdateOperation op)) + { + items[i] = op.UpdateItem(items[i]); + } + } + itemsWithNoWildcards.Clear(); + } + } + public void MarkAsReferenced() { _memoizedOperation.MarkAsReferenced(); diff --git a/src/Build/Utilities/FileSpecMatchTester.cs b/src/Build/Utilities/FileSpecMatchTester.cs index bdca578f85a..e48fca39e77 100644 --- a/src/Build/Utilities/FileSpecMatchTester.cs +++ b/src/Build/Utilities/FileSpecMatchTester.cs @@ -2,7 +2,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using Microsoft.Build.Shared; -using System; using System.Diagnostics; using System.IO; using System.Text.RegularExpressions; diff --git a/src/Shared/Constants.cs b/src/Shared/Constants.cs index d2f3da6b887..85bb070bf5b 100644 --- a/src/Shared/Constants.cs +++ b/src/Shared/Constants.cs @@ -97,6 +97,7 @@ internal static class MSBuildConstants internal static readonly char[] ForwardSlash = { '/' }; internal static readonly char[] ForwardSlashBackslash = { '/', '\\' }; internal static readonly char[] WildcardChars = { '*', '?' }; + internal static readonly string[] CharactersForExpansion = { "*", "?", "$(", "@(", "%" }; internal static readonly char[] CommaChar = { ',' }; internal static readonly char[] HyphenChar = { '-' }; internal static readonly char[] DirectorySeparatorChar = { Path.DirectorySeparatorChar }; diff --git a/src/Shared/UnitTests/ObjectModelHelpers.cs b/src/Shared/UnitTests/ObjectModelHelpers.cs index 6c62d8044f4..1dc5b0e2b55 100644 --- a/src/Shared/UnitTests/ObjectModelHelpers.cs +++ b/src/Shared/UnitTests/ObjectModelHelpers.cs @@ -424,6 +424,12 @@ internal static void AssertItemHasMetadata(Dictionary expected, AssertItemHasMetadata(expected, new ProjectItemTestItemAdapter(item)); } + internal static void AssertItemHasMetadata(string key, string value, ProjectItem item) + { + item.DirectMetadataCount.ShouldBe(1, () => $"Expected 1 metadata, ({key}), got {item.DirectMetadataCount}"); + item.GetMetadataValue(key).ShouldBe(value); + } + internal static void AssertItemHasMetadata(Dictionary expected, TestItem item) { expected ??= new Dictionary();