Optimize WorkItemTracker

[lahma]

While investigating large memory allocations in NUnit test run, the culprit actually seems to be both WorkItemTracker and Rider's internal NUnitTestListener which seem to share some implementation details as they both allocate the same way and most allocations come from use of costly XmlDocument. Cancelling large 60k test suite seems also to stall for a long while and it seems that notifying the cancellation was N^2 loop due to notification iterating full list when finding item to to notify - dictionary-based solution will help here.

• Use dictionary keyed by id instead of list for in-process items
• Use XmlWriter to write XML
• Use XmlReader to read XML
• Use custom data holder to capture item order number, name and attributes
• Reuse StringBuilder instance when sending notifications

Tested locally (pretty hard with the current legacy frameworks) in debugger and output and behavior seems to be the same. As there's no benchmark suite it's a bit hard to show actual numbers, but hopefully the technical aspect for the change is sound.

I did this against main (4.x?) but probably wouldn't hurt to have in 3.x too, code should be compatible.

[OsirisTerje]

Awesome @lahma ! Agree it should go into the V3 branches too. Right now we have two of those, the version3 branch (handling what is in version 3.16.3 and forward) and release-3.15.3 which handles the engine for the adapter.

There are acceptance tests, called package tests, that can be run to verify everything is working as before.

[lahma]

Ah, I think we need to have separate order number counter as items can be removed in such way that items.Count could be less than largest in collection, will still fix that.

[lahma]

OK now there's separate counter for determining the incrementing ordering id for items. Should be ready for your reviewing pleasure! 😉

[CharliePoole]

I added WorkItemTracker to deal with a problem in the framework, which was not handling a cancel request properly. IIRC I created issue nunit/nunit#3682 at the same time. Currently, the bug has not even been confirmed by the framework team, so I assume that NUnit itself still does not report completion of cancelled items.

If the problem were fixed in some future release of the framework, then a much greater optimization would be possible... to disable the tracker entirely when using that version or greater. :-)

[manfred-brands]

@CharliePoole I added a comment to that ticket.
I actually noticed today that the runner stop tests only run under .net framework as later .net versions don't support Thread.Abort.

[CharliePoole]

Yes, see also nunit/nunit#166, which is the actual reason for the existence of the tracker. Unfortunately, in version 3.x of the engine, it not so easy to know in advance if all the tests could suffer from this problem. In V4, with the elimination of in-process running and the execution of each assembly in a separate process, it will be easier. Of course, back a few years, I imagined we would release V4 much sooner, so I didn't try to solve the issue for V3.

If you wanted to do something about it, I would look at what happens as assemblies are loaded. Each driver could pass back info about the framework under which it was running tests and the master test runner could aggregate them before deciding whether to use the tracker at all. A further improvement would be to establish separate trackers for each assembly runner but only where needed. "Needed" would be defined as "not capable of forced stop OR not reporting completion correctly."

[lahma]

So is this something that could be considered as band-aid or just a lost cause?

I'm afraid I'm not qualified enough in the code base to fix such fundamental things. I can just read performance profiling and act (hopefully) accordingly.

[CharliePoole]

@lahma I'm afraid we got a bit off topic for your PR. If the fix is harming performance, then of course it makes sense to fix that. I am just a little frustrated that the fix has been around for two years without any work on the underlying causes. I'm not a team member so I haven't reviewed your PR, but I hope somebody who is will do that soon!

[lahma]

@CharliePoole Thank you for clarifying. And saying not being a team member is something I would call inaccurate - even though you've stepped aside, your insights are invaluable! You have my deepest gratitude for creating this library I've used for over a decade now as I was a jUnit convert back in the day.

I'm here trying to iron some rough edges even though it would mean scraping my code later on, which is fine.

[OsirisTerje]

@lahma Are there existing tests that cover the changes?

[lahma]

@OsirisTerje there's https://github.com/nunit/nunit-console/blob/8f472b0ad29404593f3084622dac88ba6e5c5191/src/NUnitEngine/nunit.engine.tests/Runners/WorkItemTrackerTests.cs which seems to cover the output and behavior. Can we trust in it? 🙂

[OsirisTerje]

@lahma I've not forgotten this. I just need some time to look through the code of the console in general, and then your code changes here. At a first glance it looks good, as usual, but please bear with me :-)

[lahma]

A kind reminder about this PR 😉

[lahma]

@OsirisTerje just to double check, should I prepare another PR against some specific branch to see this swim towards current test adapter and ideally a published package that also JetBrains Rider would hopefully consume?

[OsirisTerje]

Yes, there is a few branches to keep track of here:

main: Upcoming V4, which is in an unknown state
version315: The current version 3.15.X which is used by the adapter, and thus Visual Studio and .net test.
version3: The branch for version 3.16.X which is broken on loading assemblies, and thus is prevented from working with VS and .net. Does not support NUnit 4 (iirc).

Rider, they have embedded version 3.12.1 (iirc), and we don't have a branch for them. I've not managed to get in contact with them lately, and 3.12 is very old now.

The most probable version for being consumed by anyone is the branch version315.