ItemSpec: Do not call MatchCount when count is not needed

[JanKrivanek]

Relates to https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1728887/

Context

ItemSpecFragment.MatchCount result is thrown away when called from ItemExpressionFragment.MatchCount (it just needs to get info about existing match).

The mentioned codepath is on the stack of the detected UI delay. While it might not be the rootcause, it doesn't hurt to improve this behavior.

Thanks @davkean for specific suggestions pointers during ivestigation

[AR-May]

LGTM

[Forgind]

MatchCount just calls IsMatch and returns 0 or 1 according to the result, so this should barely be faster, but I'll take it 🙂

[JanKrivanek]

MatchCount just calls IsMatch and returns 0 or 1 according to the result, so this should barely be faster, but I'll take it 🙂

That's correct. Though there is no harm fixing, and I wanted to give it a try since it's showing on the hot stack in some perfWatson cases

[Forgind]

On the same topic, should we just eliminate MatchCount uses from across MSBuild? It's not only (slightly) slower but can also be confusing if you think you'll get a real number of matches.

[JanKrivanek]

On the same topic, should we just eliminate MatchCount uses from across MSBuild? It's not only (slightly) slower but can also be confusing if you think you'll get a real number of matches.

The second implementations of this virtual method is actualy being used in expected way: https://github.com/dotnet/msbuild/blob/main/src/Build/Evaluation/ItemSpec.cs#L324

Usage of the 0/1 implementation was removed by this PR

[Forgind]

The second implementations of this virtual method is actualy being used in expected way: https://github.com/dotnet/msbuild/blob/main/src/Build/Evaluation/ItemSpec.cs#L324

Usage of the 0/1 implementation was removed by this PR

I first saw this at an awkward time for looking into it, but can you clarify what you meant? When I look at the line you linked and use go-to-definition, it goes to the IsMatch(...) ? 1 : 0 code that is confusing and unnecessary.

[JanKrivanek]

I first saw this at an awkward time for looking into it, but can you clarify what you meant? When I look at the line you linked and use go-to-definition, it goes to the IsMatch(...) ? 1 : 0 code that is confusing and unnecessary.

You are right - the non-overwritten ItemSpecFragment.MatchCount default implementation (returning just 0 or 1) can still be called. One of the code paths (the one that was on the stack of PerfWatson findings) that let to calling it was removed.
I was rather reffereing to 'should we just eliminate MatchCount uses from across MSBuild' - since there are legitimate usages of overriden MatchCount - so removing is not straightforward and beyond scope of this PR.