Optimize Match-on-metadata #6035
There are no files selected for viewing
| 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 IMSBuildGlob ToMSBuildGlob() | ||
| { | ||
| return MsBuildGlob; | ||
|
|
@@ -310,26 +292,6 @@ public bool MatchesItem(I item) | |
| 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); | ||
| } | ||
|
|
||
| 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. 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. 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. 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. How does this type check, isn't ItemSpec.Fragments a list of the base class ItemSpecFragment? ( There was a problem hiding this comment. 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. 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. 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; | ||
| } | ||
| } | ||
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.