-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Optimize Match-on-metadata #6035
Changes from 12 commits
ba4b653
a124438
8db0cab
6c68b2b
c8d7089
17f5225
978bf35
ab951b8
9d335f2
f71a467
c9263be
08d561b
d7ce8b2
87361a6
47492ae
7fe9587
58b8a84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1988,63 +1988,6 @@ public void KeepWithItemReferenceOnNonmatchingMetadata() | |
items.ElementAt(3).GetMetadataValue("d").ShouldBe("d"); | ||
} | ||
|
||
[Fact] | ||
public void FailWithMatchingMultipleMetadata() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't delete the tests, instead replace the exception assertion with an assertion on I2's contents There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new test implies you can match with multiple data now though. If we want to keep that, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed it to "RemoveWithMatchingMultipleMetdata." I did enable matching multiple. |
||
{ | ||
string content = ObjectModelHelpers.CleanupFileContents( | ||
@"<Project ToolsVersion='msbuilddefaulttoolsversion' xmlns='msbuildnamespace'> | ||
<Target Name='t'> | ||
<ItemGroup> | ||
<I1 Include='a1' M1='1' M2='a'/> | ||
<I1 Include='b1' M1='2' M2='x'/> | ||
<I1 Include='c1' M1='3' M2='y'/> | ||
<I1 Include='d1' M1='4' M2='b'/> | ||
|
||
<I2 Include='a2' M1='x' m2='c'/> | ||
<I2 Include='b2' M1='2' m2='x'/> | ||
<I2 Include='c2' M1='3' m2='Y'/> | ||
<I2 Include='d2' M1='y' m2='d'/> | ||
|
||
<I2 Remove='@(I1)' MatchOnMetadata='M1;M2' /> | ||
</ItemGroup> | ||
</Target></Project>"); | ||
IntrinsicTask task = CreateIntrinsicTask(content); | ||
Lookup lookup = LookupHelpers.CreateEmptyLookup(); | ||
Assert.ThrowsAny<InvalidProjectFileException>(() => ExecuteTask(task, lookup)) | ||
.HelpKeyword.ShouldBe("MSBuild.OM_MatchOnMetadataIsRestrictedToOnlyOneReferencedItem"); | ||
} | ||
|
||
[Fact] | ||
public void FailWithMultipleItemReferenceOnMatchingMetadata() | ||
{ | ||
string content = ObjectModelHelpers.CleanupFileContents( | ||
@"<Project ToolsVersion='msbuilddefaulttoolsversion' xmlns='msbuildnamespace'> | ||
<Target Name='t'> | ||
<ItemGroup> | ||
<I1 Include='a1' M1='1' M2='a'/> | ||
<I1 Include='b1' M1='2' M2='x'/> | ||
<I1 Include='c1' M1='3' M2='y'/> | ||
<I1 Include='d1' M1='4' M2='b'/> | ||
|
||
<I2 Include='a2' M1='x' m2='c'/> | ||
<I2 Include='b2' M1='2' m2='x'/> | ||
<I2 Include='c2' M1='3' m2='Y'/> | ||
<I2 Include='d2' M1='y' m2='d'/> | ||
|
||
<I3 Include='a3' M1='1' m2='b'/> | ||
<I3 Include='b3' M1='x' m2='a'/> | ||
<I3 Include='c3' M1='3' m2='2'/> | ||
<I3 Include='d3' M1='y' m2='d'/> | ||
|
||
<I3 Remove='@(I1);@(I2)' MatchOnMetadata='M1' /> | ||
</ItemGroup> | ||
</Target></Project>"); | ||
IntrinsicTask task = CreateIntrinsicTask(content); | ||
Lookup lookup = LookupHelpers.CreateEmptyLookup(); | ||
Assert.ThrowsAny<InvalidProjectFileException>(() => ExecuteTask(task, lookup)) | ||
.HelpKeyword.ShouldBe("MSBuild.OM_MatchOnMetadataIsRestrictedToOnlyOneReferencedItem"); | ||
} | ||
|
||
[Fact] | ||
public void FailWithMetadataItemReferenceOnMatchingMetadata() | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,24 +85,6 @@ public override bool IsMatch(string itemToMatch) | |
return ReferencedItems.Any(v => v.ItemAsValueFragment.IsMatch(itemToMatch)); | ||
} | ||
|
||
public override bool IsMatchOnMetadata(IItem item, IEnumerable<string> metadata, MatchOnMetadataOptions options) | ||
{ | ||
return ReferencedItems.Any(referencedItem => | ||
metadata.All(m => !item.GetMetadataValue(m).Equals(string.Empty) && MetadataComparer(options, item.GetMetadataValue(m), referencedItem.Item.GetMetadataValue(m)))); | ||
} | ||
|
||
private bool MetadataComparer(MatchOnMetadataOptions options, string itemMetadata, string referencedItemMetadata) | ||
{ | ||
if (options.Equals(MatchOnMetadataOptions.PathLike)) | ||
{ | ||
return FileUtilities.ComparePathsNoThrow(itemMetadata, referencedItemMetadata, ProjectDirectory); | ||
} | ||
else | ||
{ | ||
return String.Equals(itemMetadata, referencedItemMetadata, options.Equals(MatchOnMetadataOptions.CaseInsensitive) ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal); | ||
} | ||
} | ||
|
||
public override IMSBuildGlob ToMSBuildGlob() | ||
{ | ||
return MsBuildGlob; | ||
|
@@ -310,26 +292,6 @@ public bool MatchesItem(I item) | |
return false; | ||
} | ||
|
||
/// <summary> | ||
/// Return true if any of the given <paramref name="metadata" /> matches the metadata on <paramref name="item" /> | ||
/// </summary> | ||
/// <param name="item">The item to attempt to find a match for based on matching metadata</param> | ||
/// <param name="metadata">Names of metadata to look for matches for</param> | ||
/// <param name="options">metadata option matching</param> | ||
/// <returns></returns> | ||
public bool MatchesItemOnMetadata(IItem item, IEnumerable<string> metadata, MatchOnMetadataOptions options) | ||
{ | ||
foreach (var fragment in Fragments) | ||
{ | ||
if (fragment.IsMatchOnMetadata(item, metadata, options)) | ||
{ | ||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
|
||
/// <summary> | ||
/// Return the fragments that match against the given <paramref name="itemToMatch" /> | ||
/// </summary> | ||
|
@@ -456,14 +418,6 @@ public virtual bool IsMatch(string itemToMatch) | |
return FileMatcher.IsMatch(itemToMatch); | ||
} | ||
|
||
/// <summary> | ||
/// Returns true if <paramref name="itemToMatch" /> matches any ReferencedItems based on <paramref name="metadata" /> and <paramref name="options" />. | ||
/// </summary> | ||
public virtual bool IsMatchOnMetadata(IItem itemToMatch, IEnumerable<string> metadata, MatchOnMetadataOptions options) | ||
{ | ||
return false; | ||
} | ||
|
||
public virtual IMSBuildGlob ToMSBuildGlob() | ||
{ | ||
return MsBuildGlob; | ||
|
@@ -504,4 +458,74 @@ public GlobFragment(string textFragment, string projectDirectory) | |
&& TextFragment[2] == '*' | ||
&& FileUtilities.IsAnySlash(TextFragment[3]); | ||
} | ||
|
||
internal sealed class MetadataSet<P, I> where P : class, IProperty where I : class, IItem, IMetadataTable | ||
{ | ||
private readonly Dictionary<string, MetadataSet<P, I>> _children; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why use this convoluted data structure instead of dumping all metadata values from the Itemspec into a flat set of normalized metadata values? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still not 100% sure I agree with the data structure, but I want to clarify my understanding from the PR review today. A flat set of normalized metadata values would prevent us from being able to determine exactly which metadata values from which items were matched. We'd need to do this to enable the logic in a test like <I3 Remove='@(I1);@(I2)' MatchOnMetadata='M1' /> This remove attribute effectively translates to:
I believe this also enables something like: <I3 Remove='@(I1);@(I2)' MatchOnMetadata='M1;M2' /> Where this remove attribute translates to:
@Forgind is this understanding correct? If so I think there should be a test for multiple items and multiple metadata. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrote detailed comment for MetadataTrie class. Will push soon. |
||
private readonly Func<string, string> _normalize; | ||
|
||
internal MetadataSet(MatchOnMetadataOptions options, IEnumerable<string> metadata, ItemSpec<P, I> itemSpec) | ||
{ | ||
StringComparer comparer = options == MatchOnMetadataOptions.CaseSensitive ? StringComparer.Ordinal : StringComparer.OrdinalIgnoreCase; | ||
_children = new Dictionary<string, MetadataSet<P, I>>(comparer); | ||
_normalize = options == MatchOnMetadataOptions.PathLike ? p => FileUtilities.NormalizePathForComparisonNoThrow(p, Environment.CurrentDirectory) : p => p; | ||
foreach (ItemSpec<P, I>.ItemExpressionFragment frag in itemSpec.Fragments) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this type check, isn't ItemSpec.Fragments a list of the base class ItemSpecFragment? ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. It might be casting it automatically when it sees that, but I'm not sure. I don't think we should care about other kinds of fragments, since I don't think they can have metadata, so I think this is ok, but that was a lot of "I think"s. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as whether this is actually a safe, I should have mentioned that the type is checked before the MetadataSet is constructed, so it should always be correct. |
||
{ | ||
foreach (ItemSpec<P, I>.ReferencedItem referencedItem in frag.ReferencedItems) | ||
{ | ||
this.Add(metadata.Select(m => referencedItem.Item.GetMetadataValue(m)), comparer); | ||
} | ||
} | ||
} | ||
|
||
private MetadataSet(StringComparer comparer) | ||
{ | ||
_children = new Dictionary<string, MetadataSet<P, I>>(comparer); | ||
} | ||
|
||
// Relies on IEnumerable returning the metadata in a reasonable order. Reasonable? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The question is whether the matching |
||
private void Add(IEnumerable<string> metadata, StringComparer comparer) | ||
{ | ||
MetadataSet<P, I> current = this; | ||
foreach (string m in metadata) | ||
{ | ||
string normalizedString = _normalize(m); | ||
if (!current._children.TryGetValue(normalizedString, out MetadataSet<P, I> child)) | ||
{ | ||
child = new MetadataSet<P, I>(comparer); | ||
current._children.Add(normalizedString, child); | ||
} | ||
current = child; | ||
} | ||
} | ||
|
||
internal bool Contains(IEnumerable<string> metadata) | ||
{ | ||
MetadataSet<P, I> current = this; | ||
foreach (string m in metadata) | ||
{ | ||
if (String.IsNullOrEmpty(m)) | ||
{ | ||
return false; | ||
} | ||
if (!current._children.TryGetValue(_normalize(m), out current)) | ||
{ | ||
return false; | ||
} | ||
} | ||
return true; | ||
} | ||
} | ||
|
||
public enum MatchOnMetadataOptions | ||
{ | ||
CaseSensitive, | ||
CaseInsensitive, | ||
PathLike | ||
} | ||
|
||
public static class MatchOnMetadataConstants | ||
{ | ||
public const MatchOnMetadataOptions MatchOnMetadataOptionsDefaultValue = MatchOnMetadataOptions.CaseSensitive; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,13 +13,22 @@ internal partial class LazyItemEvaluator<P, I, M, D> | |
class RemoveOperation : LazyItemOperation | ||
{ | ||
readonly ImmutableList<string> _matchOnMetadata; | ||
readonly MatchOnMetadataOptions _matchOnMetadataOptions; | ||
private MetadataSet<P, I> _metadataSet; | ||
|
||
public RemoveOperation(RemoveOperationBuilder builder, LazyItemEvaluator<P, I, M, D> lazyEvaluator) | ||
: base(builder, lazyEvaluator) | ||
{ | ||
_matchOnMetadata = builder.MatchOnMetadata.ToImmutable(); | ||
_matchOnMetadataOptions = builder.MatchOnMetadataOptions; | ||
|
||
ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile( | ||
_matchOnMetadata.IsEmpty || _itemSpec.Fragments.All(f => f is ItemSpec<ProjectProperty, ProjectItem>.ItemExpressionFragment), | ||
new BuildEventFileInfo(string.Empty), | ||
"OM_MatchOnMetadataIsRestrictedToOnlyOneReferencedItem"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't forget to change the error message to reflect the relaxed validation. |
||
|
||
if (!_matchOnMetadata.IsEmpty) | ||
{ | ||
_metadataSet = new MetadataSet<P, I>(builder.MatchOnMetadataOptions, _matchOnMetadata, _itemSpec); | ||
} | ||
} | ||
|
||
/// <summary> | ||
|
@@ -31,13 +40,6 @@ public RemoveOperation(RemoveOperationBuilder builder, LazyItemEvaluator<P, I, M | |
/// </remarks> | ||
protected override void ApplyImpl(ImmutableList<ItemData>.Builder listBuilder, ImmutableHashSet<string> globsToIgnore) | ||
{ | ||
var matchOnMetadataValid = !_matchOnMetadata.IsEmpty && _itemSpec.Fragments.Count == 1 | ||
&& _itemSpec.Fragments.First() is ItemSpec<ProjectProperty, ProjectItem>.ItemExpressionFragment; | ||
ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile( | ||
_matchOnMetadata.IsEmpty || (matchOnMetadataValid && _matchOnMetadata.Count == 1), | ||
new BuildEventFileInfo(string.Empty), | ||
"OM_MatchOnMetadataIsRestrictedToOnlyOneReferencedItem"); | ||
|
||
if (_matchOnMetadata.IsEmpty && ItemspecContainsASingleBareItemReference(_itemSpec, _itemElement.ItemType) && _conditionResult) | ||
{ | ||
// Perf optimization: If the Remove operation references itself (e.g. <I Remove="@(I)"/>) | ||
|
@@ -55,13 +57,18 @@ protected override ImmutableList<I> SelectItems(ImmutableList<ItemData>.Builder | |
var items = ImmutableHashSet.CreateBuilder<I>(); | ||
foreach (ItemData item in listBuilder) | ||
{ | ||
if (_matchOnMetadata.IsEmpty ? _itemSpec.MatchesItem(item.Item) : _itemSpec.MatchesItemOnMetadata(item.Item, _matchOnMetadata, _matchOnMetadataOptions)) | ||
if (_matchOnMetadata.IsEmpty ? _itemSpec.MatchesItem(item.Item) : MatchesItemOnMetadata(item.Item)) | ||
items.Add(item.Item); | ||
} | ||
|
||
return items.ToImmutableList(); | ||
} | ||
|
||
private bool MatchesItemOnMetadata(I item) | ||
{ | ||
return _metadataSet.Contains(_matchOnMetadata.Select(m => item.GetMetadataValue(m))); | ||
} | ||
|
||
protected override void SaveItems(ImmutableList<I> items, ImmutableList<ItemData>.Builder listBuilder) | ||
{ | ||
if (!_conditionResult) | ||
|
@@ -100,15 +107,4 @@ public RemoveOperationBuilder(ProjectItemElement itemElement, bool conditionResu | |
} | ||
} | ||
} | ||
|
||
public enum MatchOnMetadataOptions | ||
{ | ||
CaseSensitive, | ||
CaseInsensitive, | ||
PathLike | ||
} | ||
|
||
public static class MatchOnMetadataConstants { | ||
public const MatchOnMetadataOptions MatchOnMetadataOptionsDefaultValue = MatchOnMetadataOptions.CaseSensitive; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both of the tests, assert that the right items are left in I2.