Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve performance of projects with large numbers of consecutive item updates without wildcards Fixes #5776 #5853

Merged
merged 15 commits into from
Jan 9, 2021
Merged

Improve performance of projects with large numbers of consecutive item updates without wildcards Fixes #5776 #5853

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1cc6fd0
Use a dictionary to store update operations that don't include wild c…
Forgind Nov 3, 2020
68d8d32
Add tests
Forgind Nov 4, 2020
2128721
PR feedback
Forgind Nov 5, 2020
b69cf22
Another test
Forgind Nov 5, 2020
e4816f2
Make case-insensitive?
Forgind Nov 6, 2020
2f91f98
stringcomparison
Forgind Nov 6, 2020
c4763c0
Should ALWAYS be case-insensitive
Forgind Nov 11, 2020
fa1eb3b
Update src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs
Forgind Nov 16, 2020
d6a7a46
Clean up case-insensitive path comparison slightly
Forgind Nov 25, 2020
afd6a69
comments and error
Forgind Nov 25, 2020
7841914
Apply suggestions from code review
Forgind Dec 4, 2020
61b9eca
Merge https://github.com/dotnet/msbuild into winform-perf
Forgind Dec 7, 2020
9e5fe73
Update Win32.Registry version
Forgind Dec 7, 2020
07d3927
Simplify assert
Forgind Dec 11, 2020
e0bf461
update name and undo package update
Forgind Jan 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 160 additions & 20 deletions src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
using Shouldly;
using InvalidProjectFileException = Microsoft.Build.Exceptions.InvalidProjectFileException;
using Xunit;
using System.Runtime.InteropServices;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted to avoid merge conflict.


namespace Microsoft.Build.UnitTests.OM.Definition
{
Expand Down Expand Up @@ -2607,6 +2606,141 @@ 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 = $@"
<i Include='abc'>
<m1>m1_contents</m1>
</i>
<j Include='def'>
<m1>m1_contents</m1>
</j>
<i Update='{first}'>
<m1>first</m1>
</i>
<j Update='{second}'>
<m1>second</m1>
</j>
<i Update='{third}'>
<m1>third</m1>
</i>
";
IList<ProjectItem> items = ObjectModelHelpers.GetItemsFromFragment(contents, allItems: true);
Dictionary<string, string> expectedUpdatei = new Dictionary<string, string>
{
{"m1", "third" }
};
Dictionary<string, string> expectedUpdatej = new Dictionary<string, string>
{
{"m1", "second" }
};

ObjectModelHelpers.AssertItemHasMetadata(expectedUpdatei, items[0]);
ObjectModelHelpers.AssertItemHasMetadata(expectedUpdatej, items[1]);
}

[Fact]
public void UpdatingIndividualItemsProceedsInOrder()
{
string contents = @"
<i Include='a;b;c'>
<m1>m1_contents</m1>
</i>
<i Update='a'>
<m1>second</m1>
</i>
<i Update='b'>
<m1>third</m1>
</i>
<i Update='c'>
<m1>fourth</m1>
</i>
<afterFirst Include='@(i)' />
rainersigwald marked this conversation as resolved.
Show resolved Hide resolved
<i Update='*'>
<m1>sixth</m1>
</i>
<afterSecond Include='@(i)' />
<i Update='b'>
<m1>seventh</m1>
</i>
<afterThird Include='@(i)' />
<i Update='c'>
<m1>eighth</m1>
</i>
<afterFourth Include='@(i)' />
";
IList<ProjectItem> items = ObjectModelHelpers.GetItemsFromFragment(contents, allItems: true);
Dictionary<string, string> expectedAfterFirsta = new Dictionary<string, string>
{
{"m1", "second" }
};
Dictionary<string, string> expectedAfterFirstb = new Dictionary<string, string>
{
{"m1", "third" }
};
Dictionary<string, string> expectedAfterFirstc = new Dictionary<string, string>
{
{"m1", "fourth" }
};
Dictionary<string, string> expectedAfterSeconda = new Dictionary<string, string>
{
{"m1", "sixth" }
};
Dictionary<string, string> expectedAfterSecondb = new Dictionary<string, string>
{
{"m1", "sixth" }
};
Dictionary<string, string> expectedAfterSecondc = new Dictionary<string, string>
{
{"m1", "sixth" }
};
Dictionary<string, string> expectedAfterThirda = new Dictionary<string, string>
{
{"m1", "sixth" }
};
Dictionary<string, string> expectedAfterThirdb = new Dictionary<string, string>
{
{"m1", "seventh" }
};
Dictionary<string, string> expectedAfterThirdc = new Dictionary<string, string>
{
{"m1", "sixth" }
};
Dictionary<string, string> expectedAfterFourtha = new Dictionary<string, string>
{
{"m1", "sixth" }
};
Dictionary<string, string> expectedAfterFourthb = new Dictionary<string, string>
{
{"m1", "seventh" }
};
Dictionary<string, string> expectedAfterFourthc = new Dictionary<string, string>
{
{"m1", "eighth" }
};

ObjectModelHelpers.AssertItemHasMetadata(expectedAfterFirsta, items[3]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should AssertItemHasMetadata have an overload that accepts a tuple name-value, so you can eliminate all these dictionaries in the test?

ObjectModelHelpers.AssertItemHasMetadata(expectedAfterFirstb, items[4]);
ObjectModelHelpers.AssertItemHasMetadata(expectedAfterFirstc, items[5]);
ObjectModelHelpers.AssertItemHasMetadata(expectedAfterSeconda, items[6]);
ObjectModelHelpers.AssertItemHasMetadata(expectedAfterSecondb, items[7]);
ObjectModelHelpers.AssertItemHasMetadata(expectedAfterSecondc, items[8]);
ObjectModelHelpers.AssertItemHasMetadata(expectedAfterThirda, items[9]);
ObjectModelHelpers.AssertItemHasMetadata(expectedAfterThirdb, items[10]);
ObjectModelHelpers.AssertItemHasMetadata(expectedAfterThirdc, items[11]);
ObjectModelHelpers.AssertItemHasMetadata(expectedAfterFourtha, items[12]);
ObjectModelHelpers.AssertItemHasMetadata(expectedAfterFourthb, items[13]);
ObjectModelHelpers.AssertItemHasMetadata(expectedAfterFourthc, items[14]);
}

[Fact]
public void UpdateWithNoMetadataShouldNotAffectItems()
{
Expand Down Expand Up @@ -2842,6 +2976,25 @@ public void UpdateFromReferencedItemShouldBeCaseInsensitive()
ObjectModelHelpers.AssertItemHasMetadata(expectedMetadataA, items[1]);
}

[Fact]
public void UpdateFromReferencedItemShouldBeCaseInsensitive2()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you name this more explicitly?

{
string content = @"
<to Include='a' />

<to Update='A' m='m1_contents' />";

IList<ProjectItem> items = ObjectModelHelpers.GetItemsFromFragment(content, true);

var expectedMetadataA = new Dictionary<string, string>
{
{"m", "m1_contents"},
};

items[0].ItemType.ShouldBe("to");
ObjectModelHelpers.AssertItemHasMetadata(expectedMetadataA, items[0]);
}

[Fact]
public void UndeclaredQualifiedMetadataReferencesInUpdateShouldResolveToEmptyStrings()
{
Expand Down Expand Up @@ -3180,28 +3333,15 @@ public void UpdateAndRemoveShouldUseCaseInsensitiveMatching()

IList<ProjectItem> items = ObjectModelHelpers.GetItemsFromFragment(content);

if (FileUtilities.GetIsFileSystemCaseSensitive())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about this removal. Isn't it a behavior change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but we decided this was best. See #5888 (comment)

{
var expectedUpdated = new Dictionary<string, string>
{
{"m1", "m1_contents"},
{"m2", "m2_contents"},
};
items.ShouldHaveSingleItem();

ObjectModelHelpers.AssertItemHasMetadata(expectedUpdated, items[0]);
}
else
var expectedUpdated = new Dictionary<string, string>
{
items.ShouldHaveSingleItem();

var expectedUpdated = new Dictionary<string, string>
{
{"m1", "m1_updated"},
{"m2", "m2_updated"},
};
{"m1", "m1_updated"},
{"m2", "m2_updated"},
};

ObjectModelHelpers.AssertItemHasMetadata(expectedUpdated, items[0]);
}
ObjectModelHelpers.AssertItemHasMetadata(expectedUpdated, items[0]);
}

public static IEnumerable<Object[]> UpdateAndRemoveShouldWorkWithEscapedCharactersTestData
Expand Down
2 changes: 1 addition & 1 deletion src/Build/Evaluation/ItemSpec.cs
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<I, I> _itemFactory;

internal ItemSpec<P, I> Spec => _itemSpec;

protected LazyItemOperation(OperationBuilder builder, LazyItemEvaluator<P, I, M, D> lazyEvaluator)
{
_itemElement = builder.ItemElement;
Expand Down
95 changes: 65 additions & 30 deletions src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +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 ItemSpecMatchesItem _matchItemSpec = null;
private bool? _needToExpandMetadataForEachItem = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking my own review of this file, it looks like the only changes made here are caching the items needed to update and reorganizing some code into functions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: +10 points for general code cleanup of legacy code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, pretty much. I should note that I didn't do the caching part because of expected perf gains but because I wanted to access these from different functions.


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

ItemSpecMatchesItem matchItemspec;
bool? needToExpandMetadataForEachItem = null;
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have while you're here: A comment here suggesting what's happening, ideally with a small example. Something like: "see if, for example, *.cs matches somefilename.cs" ( don't use this example 🙂)


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

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

/// <summary>
/// Apply the Update operation to the item if it matches.
/// </summary>
/// <param name="item">The item to check for a match.</param>
/// <returns>The updated item.</returns>
internal ItemData UpdateItem(ItemData item)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely summarize this method since it's internal.
A very rough idea:
"Set the item spec, do X with _itemsToUpdate, and return the cloned matching item"

{
if (_conditionResult)
{
SetMatchItemSpec();
_itemsToUpdate ??= ImmutableList.CreateBuilder<ItemBatchingContext>();
_itemsToUpdate.Clear();
MatchResult matchResult = _matchItemSpec(_itemSpec, item.Item);
if (matchResult.IsMatch)
{
ItemData clonedData = UpdateItem(item, matchResult.CapturedItemsFromReferencedItemTypes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you call the private UpdateItem method here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? That's what it's doing.

DecorateItemsWithMetadata(_itemsToUpdate.ToImmutableList(), _metadata, _needToExpandMetadataForEachItem);
return clonedData;
}
}
return item;
}

private ItemData UpdateItem(ItemData item, Dictionary<string, I> 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;
}

/// <summary>
/// This sets the function used to determine whether an item matches an item spec.
/// </summary>
private void SetMatchItemSpec()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a quick description here or at the call site. Preferably here.

{
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
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<ItemSpec<P,I>.ItemExpressionFragment>().ToArray();
var nonItemReferenceFragments = _itemSpec.Fragments.Where(f => !(f is ItemSpec<P,I>.ItemExpressionFragment)).ToArray();
var itemReferenceFragments = _itemSpec.Fragments.OfType<ItemSpec<P, I>.ItemExpressionFragment>().ToArray();
var nonItemReferenceFragments = _itemSpec.Fragments.Where(f => !(f is ItemSpec<P, I>.ItemExpressionFragment)).ToArray();

matchItemspec = (itemSpec, item) =>
_matchItemSpec = (itemSpec, item) =>
{
var isMatch = nonItemReferenceFragments.Any(f => f.IsMatch(item.EvaluatedInclude));
Dictionary<string, I> capturedItemsFromReferencedItemTypes = null;
Expand All @@ -84,30 +141,8 @@ protected override void ApplyImpl(ImmutableList<ItemData>.Builder listBuilder, I
}
else
{
matchItemspec = (itemSpec, item) => new MatchResult(itemSpec.MatchesItem(item), null);
}

var itemsToUpdate = ImmutableList.CreateBuilder<ItemBatchingContext>();

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<ProjectMetadataElement> metadata, out bool? needToExpandMetadataForEachItem)
Expand Down
Loading