Optimize item Remove operations

[ladipro]

Fixes #6696

Context

Removing items by referencing other item types or by multiple single-item Remove operations has bad performance characteristics because it leads to N*M comparisons, each involving path normalization of both sides. For projects with as few as small thousands of items, evaluating them becomes a bottleneck with direct impact on build time and VS responsiveness.

Changes Made

Replaced ImmutableList<ItemData> with a new collection type OrderedItemDataCollection to be used when representing the intermediate list of items. In addition to being a list, the new collection also lazily creates a dictionary of items keyed by normalized paths, which allows for fast matching.

Testing

Functional:

• Existing unit tests, one new unit test for a previously uncovered scenario.
• Experimental VS insertion.

Performance:
Used a benchmark project with 2000 files in a subdirectory.

Evaluation time of:

    <A Include="**/*.*" />
    <B Include="**/*.*" />
    <B Remove="@(A)" />

went from 7300 ms down to 180 ms.

Evaluation time of:

    <A Include="**/*.*" />    
    <A Remove='subdir\file1'/>
    <A Remove='subdir\file2'/>
    ...
    <A Remove='subdir\file2000'/>

went from 7900 ms down to 330 ms.

Evaluation time of a Hello World .NET Core project went got slightly faster as well (520 -> 515 ms, note that this was without NGEN so most of it is JITting).

Notes

I opted for a rather simple implementation of OrderedItemDataCollection based on ImmutableList and Dictionary. It has limitations but seems to work well for the scenarios of interest. It should be possible to implement a completely custom data structure for even better results (requirements: immutable, maintains insertion order for enumeration).

[cdmihai]

Wouldn't it be better design-wise to ask the ItemSpec to compute its intersection with a given OrderedItemDataCollection via a new IntersectWith(OrderedItemDataCollection) instead of passing the ItemSpec to RemoveMatchingItems?

That way you:

• prevent leaking the implementation detail that some itemspec fragments do not support enumeration
• can neatly cache normalized values of the itemspec values without leaking that implementation detail. This would be useful if itemspec values get repeatedly normalized (for example by repeatedly calling any of the IsMatch, MatchCount, or IntersectWith methods on the same ItemSpec instance)
• can reuse the IntersectWith operation to remove the same O(n^2) issue with the Update operation.

To support this, OrderedItemDataCollection (or the Builder) could provide a string indexer which would be the normalized key. Ideally you could even hide OrderedItemDataCollection behind some collection interface but maybe that's too much for now.

[ladipro]

This is a great suggestion. I think I had the code structured differently but hit a technical issue which had me settle for this mess. I'll wait for CI to start working again to see where I stand with unit tests and will then tackle this. Thank you!

[ladipro]

I have refactored it a bit as suggested and it is indeed nicer. For Update operation, it is possible that we can now revert the previous optimization (#5853) and achieve the same using the new datastructure but I'd like to try this separately later.

[Forgind]

I don't see anything wrong with this, but it's late for me, so I don't want to approve.

[ladipro]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ladipro]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Forgind]

I'm a little curious how few things you would have to remove for it to be not worth it to make this ordered collection. Probably not important, since it would be fast either way. Thanks!

[Forgind]

Just for my own benefit—Except would look pretty here, but it would both remove all the items in itemsToRemove and remove duplicates, possibly reordering the list, so it is not ideal.

[ladipro]

I'm a little curious how few things you would have to remove for it to be not worth it to make this ordered collection. Probably not important, since it would be fast either way.

The remove operation is dominated by normalization of item values. This will run on the first remove either way. The extra overhead of building the dictionary should be relatively small and I'd be surprised if two removes were not already faster. I will measure and confirm, it's a fair question.

[ladipro]

There is a measurable regression in "small scale" removes, just as you suspected @Forgind. It slows down evaluation of a Hello World .NET Core project by a few percent. I will profile and make changes to address this.

[ladipro]

I've made the following changes:

• No longer maintaining the dictionary as immutable so it is forgotten when the list is added to the cache in MemoizedOperation. As @Therzok pointed out earlier, ImmutableDictionary has a cost and it turns out that we rarely take advantage of this memoization in real-world projects (when building OC I see <300 cache hits where we further modify the list as opposed to 20k hits with no modification).
• Enabled the new code path only for larger items lists (env variable configurable, >= 100 items by default) because the dictionary set-up work still showed in profiles for small ones.

I have updated the description with fresh measurements. There is a positive impact on evaluation time even if the dictionary is not used, due to other smaller optimizations, primarily caching the normalized value on ItemData and refactoring FileSpecMatchTester to prevent repeated normalizations.

[rainersigwald]

Can you give a bit more color on the types of scenarios where the difference is observable?

[ladipro]

Trying to unify the code to use the same path normalization function for pattern matching and for single path matching resulted in unit test failures for me. I can revisit if you want but generally I treat this as something that should not be changed just for code cleanliness, even if it affects only corner cases.

[rainersigwald]

I agree.