Improve method discovery and filtering performance

[lahma]

First of all, sorry for not pinging beforehand and discussing with you about the change. I got a bit carried away while profiling and worked with the fixes as I went on.

TLDR;

80k parametrized test case suite (loading suite, counting test cases) down from almost 11 minutes to less than one second when using Rider. So for this specific case it's over 700x improvement.

Backstory

I'm generating test source code for running ECMAScript test suite. This test suite is heavy with 86k+ test cases (about 40k files which are tested in both strict and non-strict mode) and they are ideal use case for using TestCaseAttribute defining the files for each method (where method is usually a JavaScript target like Array.prototype.filter, having many test cases, e.g. files).

Problem

I got the generation working and then started to run them with Rider... or actually I didn't as the whole unit testing sub-system froze when I tried to run the full test suite. I could run small subsets without the IDE's unit testing session hanging in waiting state. The pending to start state before tests would actually run could take minutes. Seems that Rider has different approach than VS (based on earlier issue it sends more like full names) to sending tests - Rider uses OR with all the ids - 80k ids that is. Which will cause the inclusion check algorithm to loop with a large set of data using O(n^2) .

Here's example of generated test case and how I then later profiled NUnit with benchmark test case run in console, using data from Rider invocation (FrameworkController parameters). I tweaked it a bit on the way, but results are calculated using the same code.

It seems that it's enough to have a test case such as below to make Rider freeze with unit testing operations. This should mimic the situation quite well be creating the 86 test cases we are testing here.

public class TestCaseTest
{
    [Test]
    public void Test([Range(1, 86_000)] int i)
    {
    }
}

Results before

Processing 86607 test cases. Loading them and then discovery without filter and then counting with id filter containing every id found. Ryzen 5950X with 32GB RAM. Almost 11 minutes to load tests and go trough them using a OR'ed filter. ExploreTests is using empty filter in Rider's call chain.

LoadTests took: 00:00:27.5665738 // total 86k tests, mostly from TestCase attributes
ExploreTests took: 00:00:00.0037093 // empty filter
CountTests took: 00:10:09.3055713 // 86k OR Id filter items - yes, 10 MINUTES

Fixes

Fixes consist of two parts:

Loading tests: reducing memory allocations and re-calculations when building test cases

It's very costly to do reflection and inspecting same targets multiple times. For a method having 20 test case attributes all introspection against the test method would be done 20 times, when only parameters for the test case in construction will vary.

Now caching static information about IMethodInfo for subsequent accesses.

Filtering tests: combine OR filter with same-kind, non-regexp items to single hash-based InFilter

This builds on the earlier #3786 and its PR #3787 . Now detecting more filter types and target fields and creating efficient hash set for that case. Created new InFilter type dedicated to matching.

/cc @pakrym

Results after

LoadTests took: 00:00:00.6497215 // total 86k tests, mostly from TestCase attributes
ExploreTests took: 00:00:00.0030948 // empty filter
CountTests took: 00:00:00.1815572 // 86k OR Id filter items

/cc @AndreyAkinshin for Rider performance in Unit testing (VS has different filter logic for full test suite I guess)

Related issues: #3601 #3786 #4009 nunit/nunit3-vs-adapter#660

[lahma]

For reference running against NUnit's own suite the same test logic:

Tests: 388
Test cases: 5898

Before

LoadTests took: 00:00:00.1264846
ExploreTests took: 00:00:00.0005564
CountTests took: 00:00:00.0183945

After

LoadTests took: 00:00:00.0812737
ExploreTests took: 00:00:00.0007065
CountTests took: 00:00:00.0130009

So no regressions, only slightly faster.

[ChrisMaddock]

The performance numbers intrigued me on this one so I had a look through. This looks brilliant!

I couldn't find any problem with the code here, and the numbers are really impressive. The only thing that jumped out to me was where this would benefit from some additional testing around the new logic in TestFilter.FromXml for when an InFilter should and shouldn't be built - more to protect against future code changes in this area. (I wonder if it may also be worth breaking that logic out into a separate method?)

Looks like a great improvement @lahma! 👍

[lahma]

I'll improve test coverage and check if breaking into separate method is easy.

[siprbaum]

Some "drive by review", feel free to ignore ...

[lahma]

@ChrisMaddock Thank you for the review, I've addressed your feedback in latest commit (except for the grammar), by moving optimization step into InFilter nullables were introduced so I had to tweak signatures a bit.

[ChrisMaddock]

Nice refactor! 👍 That structure looks all a bit cleaner to me, I like it!

[lahma]

So it's been a month, all horrible things going on aside, is there something I can do to push this forward?

[stevenaw]

Sorry it's taken so long to get to this @lahma
We usually like to have two sets of reviews for changes. I hope to be able to get to this myself this weekend or sometime next week

[mikkelbu]

This looks very promissing. I think we need to handle the NotImplementedException in InFilter.cs. The rest of the comments are minor issues and questions/comments.

Great work @lahma

[mikkelbu]

I think this will give us problems in runners that also write the filters in the XML (e.g. NUnitLite, the console etc). I guess we could write the filter as a OrFilter (https://docs.nunit.org/articles/nunit/technical-notes/usage/Test-Filters.html), but then we need to store the original OrFilter also (or at least have more information).

[lahma]

My thinking was that this is dynamic, e.g. translated runtime so it would not matter. I probably don't understand the full ramifications, should this be addressed in this PR, will this be breaking things?

[stevenaw]

This looks great thanks! I asked a few questions but no real additional comments or concerns beyond @mikkelbu 's. Nice job.

[lahma]

@stevenaw thank you for your review. I responded to comments, tried to improve the code based on suggestion, but leaving the question about AddToXml open. It also seems that CI is now broken due to using some obsolete build infrastructure 😬

[stevenaw]

Thanks @lahma
The build should now be fixed of you merge latest into your branch. I'm less familiar with the implications of the xml line of questions, so I think Mikkel would know better about that

[lahma]

@stevenaw @mikkelbu I've rebased this branch against v3.13-dev and implemented the AddToXml including test coverage. Now AddToXml generates same XML as the original OrFilter that was transformed into InFilter.

[mikkelbu]

Thanks for the work @lahma 👍. I'll be merging it now.

[lahma]

Thanks to everyone involved and helping out iterating the final version!