-
-
Notifications
You must be signed in to change notification settings - Fork 108
perf: add capacity to StringBuilder in ValueAssertion
#4441
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
Conversation
SummaryPre-sizes StringBuilder in ValueAssertion to avoid reallocation on the hot path (test execution). Critical IssuesOff-by-one error in capacity calculation (TUnit.Assertions/Sources/ValueAssertion.cs:19) The current calculation uses "Assert.That(?)" // 14 characters, not 13Breakdown:
Fix: Change line 19 to: var expressionBuilder = new StringBuilder((expression?.Length ?? 1) + 13);This ensures when expression is null, we allocate When expression is provided (common case), we allocate
SuggestionsNone - the performance optimization approach is sound and aligns with TUnit's "Performance First" rule for hot paths. Verdict |
|
MB I've just updated the benchmarks screenshots because I was screenshotting the wrong thing |
30a45b2 to
d518c42
Compare
By default
StringBuilderstarts with a capacity of 16. This is a problem inValueAssertionas it initialisesContextwith aStringBuilderbefore appending$"Assert.That({expression ?? "?"})", at a minimum this is 14 characters which is fine, but in the common case where the expression is not null, it will exceed the default of 16 and immediately have to add anotherStringBuilder.This pretty much guaranteed that any instance of
ValueAssertionwould allocate 2StringBuilderand 2char[]on initialisation.I solved this by initialising the
StringBuilderwith13 + expression.Length.The question is: how much additional capacity should be given? Too small and you frequently pay the cost to create a new
StringBuilderandchar[], too large and you over allocate achar[]. Making this decision based too much on the benchmarks would be optimising for the benchmarks and not real life. WDYTBefore
(expression?.Length ?? 1) + 13no unused space, the follwing assertion will have to create a newStringBuilder(expression?.Length ?? 1) + 24aka unused length of 11###
(expression?.Length ?? 1) + 32aka unused length of 19Note that the benchmarks give a general idea