-
Notifications
You must be signed in to change notification settings - Fork 723
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
Improve method discovery and filtering performance #4034
Improve method discovery and filtering performance #4034
Conversation
af53f80
to
a2ae584
Compare
94650c9
to
a47fd8d
Compare
For reference running against NUnit's own suite the same test logic: Tests: 388 Before
After
So no regressions, only slightly faster. |
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.
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! 👍
I'll improve test coverage and check if breaking into separate method is easy. |
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.
Some "drive by review", feel free to ignore ...
@ChrisMaddock Thank you for the review, I've addressed your feedback in latest commit (except for the grammar), by moving optimization step into |
0d8acf6
to
480ccd0
Compare
Nice refactor! 👍 That structure looks all a bit cleaner to me, I like it! |
So it's been a month, all horrible things going on aside, is there something I can do to push this forward? |
Sorry it's taken so long to get to this @lahma |
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.
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
|
||
public override TNode AddToXml(TNode parentNode, bool recursive) | ||
{ | ||
throw new NotImplementedException(); |
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.
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).
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.
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?
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.
This looks great thanks! I asked a few questions but no real additional comments or concerns beyond @mikkelbu 's. Nice job.
@stevenaw thank you for your review. I responded to comments, tried to improve the code based on suggestion, but leaving the question about |
Thanks @lahma |
Co-authored-by: Mikkel Nylander Bundgaard <mikkelbu@users.noreply.github.com>
1aed9f2
to
f46f109
Compare
f46f109
to
ea5062e
Compare
ea5062e
to
cf2fa03
Compare
Thanks for the work @lahma 👍. I'll be merging it now. |
Thanks to everyone involved and helping out iterating the final version! |
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 likeArray.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.
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.
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 InFilterThis 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
/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