-
Notifications
You must be signed in to change notification settings - Fork 727
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
Optimize progress reporting by reusing StringBuilder #4360
Conversation
@@ -30,6 +31,9 @@ public TestProgressReporter(ICallbackEventHandler handler) | |||
|
|||
#region ITestListener Members | |||
|
|||
[ThreadStatic] | |||
private static StringBuilder? _testStartedStringBuilder; |
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 feels like a good step forward here, thanks! I don't have a great perfect model of the framework's approach to parallelization but this feels safe since we internally only use it on-stack.
I think this will still create a builder per thread. This change makes me curious if this opens the door to take this further in future PRs by making a formal pooling mechanism with rent/release mechanics so it's only at most a builder per thread in the worst case (ie, when all worker threads happen to be processing a TestStarted
at the same time).
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 the easiest approach might be just taking the source from .NET: https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/System/Text/StringBuilderCache.cs , it's MIT licensed so should not be a problem and already battle-tested. Here I just tried to avoid bringing too much new stuff in, but I agree that general solution would be better.
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.
Seems that NUnit would require larger than 360 chars default as it's building XML, Jint is using a pool which is not bound to threads which might work better here: https://github.com/sebastienros/jint/blob/main/Jint/Pooling/StringBuilderPool.cs
@@ -39,19 +43,39 @@ public void TestStarted(ITest test) | |||
var parent = GetParent(test); | |||
try | |||
{ | |||
string report; | |||
var report = _testStartedStringBuilder ??= new StringBuilder(); |
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.
question: Should we set an initial capacity? I think it'll default to 16
but in either path below we likely require more
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.
With MS version it would be 16, without initial the call after first will have larger StringBuilder with this original code.
if (test is TestSuite) | ||
{ | ||
report | ||
.Append("<start-suite id=\"").Append(test.Id).Append('"') |
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.
comment: There was some duplicate of attributes in both paths here, but this move to StringBuilder also probably opens some opportunities for deduplication of XML attribute writing too in the future. Thanks!
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.
Fixed in the latest force-push, adding common once before checking the type of requested output
3136660
to
d874a5c
Compare
So this might need some confirmation from the NUnit team, but by understanding, the |
8e2762e
to
f5b0192
Compare
@lahma Some conflicts here now |
f5b0192
to
26d6267
Compare
Rebased, I believe these two of my PR will probably nicely conflict with each other so if either one gets merged, I'll rebase the other. |
4a5cdcb
to
54b6f5e
Compare
@lahma I merged your other PR so could you resolve the merge conflicts in this one please? |
54b6f5e
to
ba181eb
Compare
@manfred-brands rebased and even got a green build! |
ba181eb
to
7579d15
Compare
I removed the TNode |
@manfred-brands @stevenaw Is this ready to be merged now? Any comments actually unresolved? |
@OsirisTerje I did not review as @stevenaw had some comments. |
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.
Thanks @lahma ! I've been revisiting this PR periodically hoping to have had the time to take it all in and process it. I think I've finally been able to give it the attention it deserves.
This looks great to me, thank you once again for the careful profiling. I've left one question requesting clarification from someone more in-the-know about the sealing of TestProgressListener
and would appreciate a second pair of eyes in general, but overall this looks great ✅
if (text is null) | ||
throw new ArgumentNullException(nameof(text)); | ||
|
||
text = EscapeInvalidXmlCharacters(text); |
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.
question: @lahma I noticed we added the call to EscapeInvalidXmlCharacters
. Were there any other changes as part of moving this into an extensions file? It's tough to tell for sure
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.
No other changes than ensuring the common fast path for "no need to encode anything".
.Replace("'", "'") | ||
.Replace("<", "<") | ||
.Replace(">", ">"); | ||
_stringBuilder.Clear(); |
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.
👍
{ | ||
static readonly Logger log = InternalTrace.GetLogger("TestProgressReporter"); | ||
private static readonly Logger log = InternalTrace.GetLogger("TestProgressReporter"); |
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.
Thanks for making the visibility explicit
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.
We should enable the StyleCop rule and do it for all fields.
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'm also partly confused about usage of regions (I personally don't like them) and braces (I usually always use them), maybe they could be part of the rule too if either one should not be used.
@@ -11,19 +14,22 @@ namespace NUnit.Framework.Internal | |||
/// the async callbacks that are used to inform the client | |||
/// software about the progress of a test run. | |||
/// </summary> | |||
public class TestProgressReporter : ITestListener | |||
public sealed class TestProgressReporter : ITestListener |
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 understand how sealing a class can help performance, but I'm a little cautious about this one. I think ITestEventListener
was removed as an extension point in the framework but is still an extension point within the engine. It's plausible that someone may've extended TestProgressReporter
in their codebase to do this.
https://docs.nunit.org/articles/nunit-engine/extensions/creating-extensions/Event-Listeners.html?q=ITestListener
@OsirisTerje you have more experience at this level, but am I reading the docs right? I'm a little confused as we also have this, but I believe this page only indicates the removal as a framework extension point: https://docs.nunit.org/articles/nunit/technical-notes/usage/Addin-Replacement-in-the-Framework.html?q=ITestListener
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.
Sorry for being a little late for the game. Would v4 be good time to seal (separate PR) all things that imaginable that doesn't break the current upstream (adapters, runners) and then if someone cries out just open in small point releases, opening sealed classes later isn't a breaking change but sealing too late is...
writer.WriteAttributeStringSafe("fullname", test.FullName); | ||
writer.WriteAttributeStringSafe("type", test.TestType); | ||
|
||
if (isSuite) |
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.
Thank you 😄
I hadn't expected my earlier comment about this to make it into the PR but it's a welcome addition
@manfred-brands seems we were both looking at this at the same time 😄 No obligation, but I'd appreciate a second pair of eyes if you're able. |
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.
Only a tiny extra '!' null suppression operator that isn't needed.
I'll remove it on my next nullable change.
{ | ||
TNode prop = properties.AddElement("property"); | ||
|
||
// TODO: Format as string | ||
prop.AddAttribute("name", pair.Key); | ||
prop.AddAttribute("value", value.ToString()!); | ||
prop.AddAttribute("value", list[i]!.ToString()!); |
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.
prop.AddAttribute("value", list[i]!.ToString()!); | |
prop.AddAttribute("value", list[i].ToString()!); |
list elements are not-null. Null suppression operator not needed.
{ | ||
static readonly Logger log = InternalTrace.GetLogger("TestProgressReporter"); | ||
private static readonly Logger log = InternalTrace.GetLogger("TestProgressReporter"); |
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.
We should enable the StyleCop rule and do it for all fields.
When lookin at memory allocations during Jint.Tests.Test262 run, seems that most of the allocations come from NUnit itself (12%). Reporting test progress generates a lot of string so there's some room for improvement. Using reused
StringBuilder
to report progress.The targets are
OuterXml
which always creates a newStringBuilder
andTestStarted
which manually concatenates a larger string. Now newGetOuterXml()
allows providing reusableStringBuilder
. Introduced new internalXmlExtensions
that allows writing things in concise manner.The root cause is more on the NUnit.Console side, but this should also help a bit.