perf: use ValueStringBuilder to avoid allocations

[TimothyMakkison]

Uses ValueListBuilder and ValueStringBuilder to drastically reduce allocations in TestIdentifierService

• Note that ValueListBuilder has a lot of potential in other areas of TUnit; I see a lot of temporary Lists in use.
• typeSb could be a VLS but it's used so infrequently I don't see any point - Happy to change this.
  • Edit switched this to use VLS as it may see some use if someone uses generic tests.
• removes a lot of intermediary string allocations and ArrayPool usage

Bef ore

After

[thomhurst]

This seems to have broken execution for a bunch of tests. I think using a custom collection is quite risky too. Maybe we should leave this one.

[TimothyMakkison]

This seems to have broken execution for a bunch of tests.

What are the errors? I wouldn't be surprised if I messed up the logic without any tests: 😊

I think using a custom collection is quite risky too. Maybe we should leave this one.

Fair enough, I had a lot of plans to use these but I can adapt. For what it's worth the collections are both internally used by Microsoft in the .Net runtime and IMO are easier to use than ArrayPools

[thomhurst]

How come they don't add it into the core library?

I haven't investigated properly, but it's expected things like 84 tests in a run and only getting 13. Click on the Ubuntu pipeline failures on the pr checks

[TimothyMakkison]

The failed test are mostly TUnit.Engine so I probably messed up some logic when rewriting this method. Going to be a pain debugging this without local tests 😅 Not sure why TUnit.TestProject.Fail4 is failing.

How come they don't add it into the core library?

There is a proposal for this but it's blocked until they add a safety tool. They are worried that people will accidentally misuse it by; copying the ValueStringBuilder (instead of passing by ref), and then accidentally using an array that was returned to the ArrayPool. Andrew lock has a better explanation on his blog.

[thomhurst]

Summary

This PR optimizes TestIdentifierService to reduce allocations using ValueStringBuilder and ValueListBuilder, improving performance.

Critical Issues

1. Code Duplication: ValueStringBuilder already exists

The PR adds ValueStringBuilder to TUnit.Engine/Helpers/, but this type already exists at TUnit.Core/Helpers/ValueStringBuilder.cs (line 9). Since TUnit.Engine.csproj already references TUnit.Core (line 34 of TUnit.Engine.csproj), you can import and use the existing type instead of duplicating the code.

Recommendation:

• Remove TUnit.Engine/Helpers/ValueStringBuilder.cs
• Add using TUnit.Core.Helpers; to TestIdentifierService.cs
• Consider moving ValueListBuilder to TUnit.Core/Helpers/ as well for reusability (the PR description mentions it has potential in other areas of TUnit)

2. Potential Bug in WriteTypeNameWithGenerics

In WriteTypeNameWithGenerics (TestIdentifierService.cs:87), there is a ValueListBuilder initialized with a stack-allocated array of 4 nulls. This may cause the builder to start with Length=4 instead of Length=0, potentially adding null strings to the output when iterating backwards at line 139.

Suggestions

1. Consider DynamicallyAccessedMembers annotation for reflection usage (line 113) per TUnit Rule 5
2. Good job disposing the builders correctly
3. Performance validation looks good based on the PR description

Verdict

REQUEST CHANGES - Critical code duplication and potential bug in ValueListBuilder initialization

[TimothyMakkison]

Should have fixed the issue, on line 138 I looped the list in reverse but didn't update line 140 to reflect this.

Note that it might be possible to remove the allocation on line 124 by changing var typeHierarchy = new ValueListBuilder<Range> where it keeps track of strings in typeVSb.

Edit: I think there is a second bug 😭