Skip to content

Commit

Permalink
Perform item updates in mini-batches
Browse files Browse the repository at this point in the history 
Performing all item updates at the end broke the ordering guarantee. This reinstitutes that by pausing to perform the relevant updates between non-batchable updates and other operations.
  • Loading branch information
@Forgind
Forgind committed Nov 3, 2020
1 parent 4d4bb4d commit 14a4a77
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 36 deletions.
2 changes: 1 addition & 1 deletion src/Build/Evaluation/ItemSpec.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ internal abstract class ItemSpecFragment
/// <summary>
/// Path of the project the itemspec is coming from
/// </summary>
protected string ProjectDirectory { get; }
internal string ProjectDirectory { get; }

// not a Lazy to reduce memory
private ref FileSpecMatcherTester FileMatcher
Expand Down
46 changes: 28 additions & 18 deletions src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ internal partial class LazyItemEvaluator<P, I, M, D>
class UpdateOperation : LazyItemOperation
{
private readonly ImmutableList<ProjectMetadataElement> _metadata;
private ImmutableList<ItemBatchingContext>.Builder itemsToUpdate = null;
private ImmutableList<ItemBatchingContext>.Builder _itemsToUpdate = null;
private ItemSpecMatchesItem _matchItemSpec = null;
private bool? _needToExpandMetadataForEachItem = null;

public UpdateOperation(OperationBuilderWithMetadata builder, LazyItemEvaluator<P, I, M, D> lazyEvaluator)
: base(builder, lazyEvaluator)
Expand Down Expand Up @@ -44,32 +46,41 @@ protected override void ApplyImpl(ImmutableList<ItemData>.Builder listBuilder, I
return;
}

ItemSpecMatchesItem matchItemspec = MatchItemSpec(out bool? needToExpandMetadataForEachItem);
itemsToUpdate ??= ImmutableList.CreateBuilder<ItemBatchingContext>();
itemsToUpdate.Clear();
SetMatchItemSpec();
_itemsToUpdate ??= ImmutableList.CreateBuilder<ItemBatchingContext>();
_itemsToUpdate.Clear();

for (int i = 0; i < listBuilder.Count; i++)
{
var itemData = listBuilder[i];

var matchResult = matchItemspec(_itemSpec, itemData.Item);
var matchResult = _matchItemSpec(_itemSpec, itemData.Item);

if (matchResult.IsMatch)
{
listBuilder[i] = UpdateItem(listBuilder[i], matchResult.CapturedItemsFromReferencedItemTypes);
}
}

DecorateItemsWithMetadata(itemsToUpdate.ToImmutableList(), _metadata, needToExpandMetadataForEachItem);
DecorateItemsWithMetadata(_itemsToUpdate.ToImmutableList(), _metadata, _needToExpandMetadataForEachItem);
}

internal ItemData UpdateItem(ItemData item)
{
itemsToUpdate ??= ImmutableList.CreateBuilder<ItemBatchingContext>();
itemsToUpdate.Clear();
ItemData clonedData = UpdateItem(item, MatchItemSpec(out bool? needToExpandMetadataForEachItem)(_itemSpec, item.Item).CapturedItemsFromReferencedItemTypes);
DecorateItemsWithMetadata(itemsToUpdate.ToImmutableList(), _metadata, needToExpandMetadataForEachItem);
return clonedData;
if (_conditionResult)
{
SetMatchItemSpec();
_itemsToUpdate ??= ImmutableList.CreateBuilder<ItemBatchingContext>();
_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<string, I> capturedItemsFromReferencedItemTypes)
Expand All @@ -78,27 +89,26 @@ private ItemData UpdateItem(ItemData item, Dictionary<string, I> capturedItemsFr
// 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));
_itemsToUpdate.Add(new ItemBatchingContext(clonedData.Item, capturedItemsFromReferencedItemTypes));
return clonedData;
}

private ItemSpecMatchesItem MatchItemSpec(out bool? needToExpandMetadataForEachItem)
private void SetMatchItemSpec()
{
needToExpandMetadataForEachItem = null;
if (ItemspecContainsASingleBareItemReference(_itemSpec, _itemElement.ItemType))
{
// Perf optimization: If the Update operation references itself (e.g. <I Update="@(I)"/>)
// then all items are updated and matching is not necessary
return (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<ItemSpec<P, I>.ItemExpressionFragment>().ToArray();
var nonItemReferenceFragments = _itemSpec.Fragments.Where(f => !(f is ItemSpec<P, I>.ItemExpressionFragment)).ToArray();

return (itemSpec, item) =>
_matchItemSpec = (itemSpec, item) =>
{
var isMatch = nonItemReferenceFragments.Any(f => f.IsMatch(item.EvaluatedInclude));
Dictionary<string, I> capturedItemsFromReferencedItemTypes = null;
Expand All @@ -123,7 +133,7 @@ private ItemSpecMatchesItem MatchItemSpec(out bool? needToExpandMetadataForEachI
}
else
{
return (itemSpec, item) => new MatchResult(itemSpec.MatchesItem(item), null);
_matchItemSpec = (itemSpec, item) => new MatchResult(itemSpec.MatchesItem(item), null);
}
}

Expand Down
60 changes: 43 additions & 17 deletions src/Build/Evaluation/LazyItemEvaluator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,36 +338,38 @@ private static ImmutableList<ItemData>.Builder ComputeItems(LazyItemList lazyIte

ImmutableHashSet<string> currentGlobsToIgnore = globsToIgnoreStack == null ? globsToIgnore : globsToIgnoreStack.Peek();

Dictionary<string, UpdateOperation> itemsWithNoWildcards = new Dictionary<string, UpdateOperation>();
Dictionary<string, UpdateOperation> itemsWithNoWildcards = new Dictionary<string, UpdateOperation>(FileUtilities.GetIsFileSystemCaseSensitive() ? StringComparer.Ordinal : StringComparer.OrdinalIgnoreCase);
bool addedToBatch = false;

// Walk back down the stack of item lists applying operations
while (itemListStack.Count > 0)
{
var currentList = itemListStack.Pop();

// 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
if (currentList._memoizedOperation.Operation is RemoveOperation)
{
globsToIgnoreStack.Pop();
currentGlobsToIgnore = globsToIgnoreStack.Count == 0 ? globsToIgnore : globsToIgnoreStack.Peek();
}
else if (currentList._memoizedOperation.Operation is UpdateOperation op)
if (currentList._memoizedOperation.Operation is UpdateOperation op)
{
bool addToBatch = true;
int i;
for (i = 0; i < op.ISpec.Fragments.Count; i++)
{
ItemSpecFragment frag = op.ISpec.Fragments[i];
if (frag.TextFragment.IndexOfAny(MSBuildConstants.WildcardChars) != -1 || itemsWithNoWildcards.ContainsKey(frag.TextFragment))
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))
{
// No one to one match between paths and items. Cannot batch over them using a dictionary.
// Another update will already happen on this path. Make that happen before evaluating this one.
addToBatch = false;
break;
}
else
{
itemsWithNoWildcards.Add(frag.TextFragment, op);
itemsWithNoWildcards.Add(fullPath, op);
}
}
if (!addToBatch)
Expand All @@ -380,22 +382,46 @@ private static ImmutableList<ItemData>.Builder ComputeItems(LazyItemList lazyIte
}
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
if (currentList._memoizedOperation.Operation is RemoveOperation)
{
globsToIgnoreStack.Pop();
currentGlobsToIgnore = globsToIgnoreStack.Count == 0 ? globsToIgnore : globsToIgnoreStack.Peek();
}

currentList._memoizedOperation.Apply(items, currentGlobsToIgnore);
}

for (int i = 0; i < items.Count; i++)
ProcessNonWildCardItemUpdates(itemsWithNoWildcards, items);

return items;
}

private static void ProcessNonWildCardItemUpdates(Dictionary<string, UpdateOperation> itemsWithNoWildcards, ImmutableList<ItemData>.Builder items)
{
if (itemsWithNoWildcards.Count > 0)
{
if (itemsWithNoWildcards.TryGetValue(items[i].Item.EvaluatedInclude, out UpdateOperation op))
for (int i = 0; i < items.Count; i++)
{
items[i] = op.UpdateItem(items[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();
}

return items;
}

public void MarkAsReferenced()
Expand Down
1 change: 1 addition & 0 deletions src/Shared/Constants.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down

0 comments on commit 14a4a77

Please sign in to comment.