perf: consolidate per-method TestSource into per-class TestSource (~27k fewer JITs)

[thomhurst]

Summary

• Merges per-method TestSource classes + per-class ClassHelper into one per-class TestSource implementing both ITestSource and ITestDescriptorSource
• Eliminates 135,000 JIT compilations in TUnit.Generated (150k → 15k) for the 10,000-test repro
• 3.3x faster test startup (9.3s → 2.8s) for 10,000 tests

Profiling results (10,000 tests — 1,000 classes × 10 methods)

JIT compilations

Category	Baseline (v1.17.54)	New (per-class TestSource)	Change
TUnit.Generated	150,001	15,000	-90%
Calculator (test code)	11,009	11,009	0
System + Microsoft	2,834	2,824	-10
TUnit framework	1,235	1,236	+1
Other	181	181	0
Total	165,260	30,250	-82%

TUnit.Generated method breakdown

Method	Baseline	New	Saved
.ctor	30,000	1,000	29,000
Initialize	10,000	1,000	9,000
GetTestsAsync / GetTests	10,000	1,000	9,000
Async enumerator methods (5)	50,000	0	50,000
<GetTestsAsync>b__0_* lambdas	40,001	0	40,001
.cctor	10,000	0	10,000
__CreateAttributes_0	—	1,000	—
CreateInstance	—	1,000	—
__InvokeTest_*	—	10,000	—

Runtime duration (3 runs each, no tracing overhead)

	Baseline (v1.17.54)	New (per-class TestSource)
Run 1	9.374s	2.790s
Run 2	9.344s	2.779s
Run 3	9.200s	2.814s
Average	9.31s	2.79s
Speedup		3.3x faster (70% reduction)

Design

• All code pre-generated as strings during the transform step (where ISymbol is available), stored in equatable ClassTestGroup for incremental caching
• Attribute factory methods deduplicated by content — methods with identical [Test] attributes share one __CreateAttributes_0
• Per-method __Materialize_{id} methods ensure filtering correctness (never JIT'd in normal runs)
• Generic types/methods and inherited tests (InheritanceDepth > 0) continue using the per-method pipeline unchanged
• The large JIT reduction vs baseline comes from eliminating the async enumerator pattern — the old GetTestsAsync generated 5 async state machine methods per class (50k JITs), replaced by a single synchronous GetTests returning IReadOnlyList<TestMetadata>

Test plan

• All 114 snapshot tests pass on net10.0, net9.0, net8.0, net472
• TUnit.TestProject runtime verification: BasicTests, DataDrivenTests, ClassConstructorTest, GenericMethodTests, STAThreadTests (1515 tests), DependsOnTests, NumberArgumentTests, RepeatTests, CategoryTests, PropertySetterTests
• Single-method filter works (exercises EnumerateTestDescriptors → Materializer path)
• Profiled with 10,000-test repro: 150k → 15k TUnit.Generated JITs, 9.3s → 2.8s runtime

[claude]

Code Review

This is a well-designed, impactful performance PR. The core concept — consolidating N per-method TestSource classes + 1 ClassHelper into a single per-class TestSource — is sound and the JIT reduction numbers are compelling.

Design: Strong

The pre-generation architecture is particularly well thought-out:

• Storing all data as strings/primitives in ClassTestGroup (no ISymbol refs) is the right approach for incremental caching correctness.
• The __Materialize_{id} pattern for filter paths is clever: separating the cold path (filtering via --treenode-filter) from the hot path (GetTests) means filter methods are never JIT'd in normal runs.
• Attribute body deduplication via __CreateAttributes_{index} is a nice bonus optimization.
• The guard to skip normal tests in GenerateTestMethodSource is clean and the comment explains the intent well.

---

Issue 1: Duplication between PreGenerateMetadataBlock and PreGenerateMaterializerMethod

Both methods emit an almost-identical new TestMetadata<T> { ... } initialization block. If the metadata structure ever changes, it must be updated in two places.

The __Materialize_{id} body is essentially: "what GetTests() builds for this one method, wrapped in a method that returns it". A cleaner approach would be a shared private BuildMetadata_{id}(string testSessionId) method that both GetTests() and __Materialize_{id} call:

// Generated output (illustrative)
private static TestMetadata<T> BuildMetadata_Test1(string testSessionId) {
    var m = new TestMetadata<T> { ... };
    m.UseRuntimeDataGeneration(testSessionId);
    return m;
}
public IReadOnlyList<TestMetadata> GetTests(string s) => new[] { BuildMetadata_Test1(s), BuildMetadata_Test2(s) };
private static IReadOnlyList<TestMetadata> __Materialize_Test1(string s) => new[] { BuildMetadata_Test1(s) };

This eliminates the duplication at the cost of one extra method call per test per run. Given the JIT context, these BuildMetadata_* methods would themselves be JIT'd — so this might negate some of the benefit. The current approach is defensible, but worth a comment noting the tradeoff.

---

Issue 2: TestMethodSourceCode is a sealed class but ClassTestGroup is a record

TestMethodSourceCode implements value equality manually with the classic 17 * 31 + hash pattern. Since the project targets LangVersion preview, a record would auto-generate the correct Equals/GetHashCode/ToString and remove ~25 lines of boilerplate:

public sealed record TestMethodSourceCode
{
    public required string MethodId { get; init; }
    public required string MetadataCode { get; init; }
    // ... etc
}

The HashCode.Combine() API is also available as a more modern alternative if keeping the class form.

---

Issue 3: GetSafeMethodId lacks truncation

GetDeterministicFileNameForMethod and GetSafeTestSourceName both truncate to MaxHintNameLength with a stable hash suffix. GetSafeMethodId doesn't:

// FileNameHelper.cs:159
public static string GetSafeMethodId(IMethodSymbol methodSymbol)
{
    // ... no truncation
    return sb.ToString();
}

For test methods with long names and many complex parameter types (e.g. generic types with long display names), the __InvokeTest_{id} and __Materialize_{id} method names could get very long. While C# doesn't have a symbol name limit, a method named __InvokeTest_MyMethod__Dictionary_String_List_MyVeryLongGenericType_ is awkward. Consider applying the same truncation+hash pattern here.

---

Minor: O(n²) in overload uniqueness loop

// GroupMethodsByClass, ~line 2665
while (methodSourceCodes.Any(existing => existing.MethodId == methodId))

This is O(n²) over methods in a class. For typical test classes (10–50 methods) it's inconsequential, but it could be replaced with a HashSet<string> for O(n) overall. Not blocking.

---

Minor: distinctBodies.IndexOf(body) is O(n) per method

var attrIndexMap = attrBodies.Select(body => distinctBodies.IndexOf(body)).ToList();

A Dictionary<string, int> mapping body → index would make this O(1) per lookup. Again, not blocking for typical class sizes.

---

Summary

Performance impact	Excellent — 27k fewer JITs is meaningful
Correctness	Looks solid; generic/inherited paths unchanged; snapshot tests all pass
Incremental caching	Well-designed; primitives-only in cached models
Maintainability concern	Metadata block duplication between PreGenerateMetadataBlock and PreGenerateMaterializerMethod
Naming consistency	TestMethodSourceCode should be a record like ClassTestGroup

The PR is in good shape. The main thing worth addressing before merge is the metadata block duplication (or at least a comment explaining the intentional tradeoff), and making TestMethodSourceCode a record for consistency.

[thomhurst]

Addressed the review feedback:

Issue 1 (metadata duplication): Intentional — extracting BuildMetadata_* helpers would add 10,000 JIT compilations (one per test method), negating the optimization. Materializers are never JIT'd in normal runs. Added a comment to MaterializerMethod documenting this tradeoff.

Issue 2 (TestMethodSourceCode as record): Done. Converted to sealed record, removing ~25 lines of manual equality boilerplate.

Issue 3 (GetSafeMethodId truncation): Leaving as-is. Method IDs are C# symbol names in generated code, not file names — no path length constraint applies. Truncation would hurt stack trace readability (e.g. __InvokeTest_MyMethod__String_Int32 is far more useful for debugging than __InvokeTest_MyMeth_A3B2C1D0). File name truncation already happens in GetSafeTestSourceName.

Minor (O(n²) loop + O(n) IndexOf): Fixed both — HashSet<string> for method ID uniqueness, Dictionary<string, int> for attribute body deduplication.

[claude]

Follow-up Review

All five issues from the previous review have been properly resolved. Here's a verification:

Previous Issues — All Resolved ✅

Issue 1: Metadata duplication (PreGenerateMetadataBlock / PreGenerateMaterializerMethod)
✅ Addressed. The comment added to TestMethodSourceCode.MaterializerMethod clearly explains the deliberate tradeoff: extracting BuildMetadata_* helpers would add 10k JIT compilations (one per test method), negating the optimization. Since materializers are only called via --treenode-filter (never in normal runs), the duplication has zero runtime cost. This is the correct decision — the comment makes the intent future-proof.

Issue 2: TestMethodSourceCode should be a record
✅ Done. Converted to sealed record, removing the manual equality boilerplate. Consistent with ClassTestGroup.

Issue 3: GetSafeMethodId truncation
✅ Reasonable decision to leave as-is. The explanation is sound — method IDs appear in generated C# symbol names (not file paths), so there's no path-length constraint, and readable names like __InvokeTest_MyMethod__String_Int32 are more useful for debugging than hash-truncated versions.

Minor: O(n²) loop for method ID uniqueness
✅ Fixed. HashSet<string> usedMethodIds replaces the linear scan.

Minor: O(n) IndexOf for attribute deduplication
✅ Fixed. Dictionary<string, int> bodyToIndex gives O(1) lookup.

---

New Observations

Generated output verified clean. Inspecting BasicTests.Test.DotNet10_0.verified.txt confirms the consolidated structure works exactly as designed: three methods share __CreateAttributes_0, each gets their own __InvokeTest_* and __Materialize_*, and the single ModuleInitializer registers one instance. The attribute deduplication is working correctly.

Minor nit (non-blocking): In GroupMethodsByClass:

var methodsList = g.Where(m => m is not null).Select(m => m!).ToList();

Could be written more idiomatically as .OfType<TestMethodMetadata>().ToList(), avoiding the null-forgiving operator. No behaviour change, purely stylistic.

Confirmed design correctness: The separation of paths is clean — the early return in GenerateTestMethodSource for non-generic/non-inherited tests (feeding the per-method pipeline) is correctly complemented by GroupMethodsByClass feeding the per-class pipeline. The 114 snapshot tests passing across net8.0/net9.0/net10.0/net4.7 are a strong correctness signal.

---

Summary

This is a well-executed, high-impact optimization. The architecture is sound, all prior feedback addressed, and the tradeoffs are clearly documented. The 90% reduction in TUnit.Generated JITs and 3.3x startup improvement are significant real-world wins.

Ready to merge.

[claude]

Third Review — Commit 3 (AppendRaw Bug Fix)

This review covers the new commit added after the second review: fix: preserve blank lines in CodeWriter.AppendRaw.

Previous Issues — All Confirmed Resolved ✅

All five issues from review #1 (metadata duplication, record conversion, truncation, O(n²) loop, O(n) IndexOf) remain properly addressed as confirmed in review #2. No regressions.

---

New Commit: AppendRaw Blank Line Fix

The fix is correct and necessary. The root cause was subtle: AppendLine("") → Append("") → early return (empty string check), so _isNewLine was never set, and blank lines were silently dropped.

The fix correctly handles the state machine:

• Since AppendRaw strips leading blank lines, the loop always begins with a non-empty line
• After AppendLine(nonEmptyLine), _isNewLine = true
• A blank line then calls _builder.AppendLine() directly — _isNewLine remains true, which is accurate (the builder is now on a fresh line)
• The next non-empty line will correctly prepend indentation via the _isNewLine check

The fix is minimal, well-commented, and correctly scoped to in-content blank lines only (leading/trailing still stripped). The commit message explains the concrete breakage (6 failing tests in Tests2117 for multi-line error message strings in [Arguments] attributes).

---

Remaining Nit (Non-Blocking, Unchanged)

The OfType<> style suggestion from review #2 was not addressed:

// Current
var methodsList = g.Where(m => m is not null).Select(m => m\!).ToList();

// More idiomatic
var methodsList = g.OfType<TestMethodMetadata>().ToList();

This was marked non-blocking in the previous review and remains so — it's a style preference. Either form is correct.

---

Summary

Area	Status
AppendRaw blank line bug	✅ Fixed correctly
All prior review issues	✅ Remain addressed
Architecture	✅ Sound; no regressions introduced
Snapshot tests (114)	✅ All passing
Runtime verification	✅ Confirmed in PR description

The PR remains in excellent shape. The bug fix is the right call — blank lines in multi-line string arguments to [Arguments] are a legitimate use case that was silently broken. Ready to merge.

[claude]

Third Review — Commit 3 (AppendRaw Bug Fix)

This review covers the new commit added after the second review: fix: preserve blank lines in CodeWriter.AppendRaw.

Previous Issues — All Confirmed Resolved ✅

All five issues from review #1 (metadata duplication, record conversion, truncation, O(n²) loop, O(n) IndexOf) remain properly addressed as confirmed in review #2. No regressions.

---

New Commit: AppendRaw Blank Line Fix

The fix is correct and necessary. The root cause was subtle: AppendLine("") → Append("") → early return (empty string check), so _isNewLine was never set, and blank lines were silently dropped.

The fix correctly handles the state machine:

• Since AppendRaw strips leading blank lines, the loop always begins with a non-empty line
• After AppendLine(nonEmptyLine), _isNewLine = true
• A blank line then calls _builder.AppendLine() directly — _isNewLine remains true, which is accurate (the builder is now on a fresh line)
• The next non-empty line will correctly prepend indentation via the _isNewLine check

The fix is minimal, well-commented, and correctly scoped to in-content blank lines only (leading/trailing still stripped). The commit message explains the concrete breakage (6 failing tests in Tests2117 for multi-line error message strings in [Arguments] attributes).

---

Remaining Nit (Non-Blocking, Unchanged from Review #2)

The OfType<> style suggestion from review #2 was not addressed:

// Current
var methodsList = g.Where(m => m is not null).Select(m => m!).ToList();

// More idiomatic
var methodsList = g.OfType<TestMethodMetadata>().ToList();

This was marked non-blocking in the previous review and remains so — style preference only.

---

Summary

Area	Status
AppendRaw blank line bug	Fixed correctly
All prior review issues	Remain addressed
Architecture	Sound; no regressions introduced
Snapshot tests (114)	All passing
Runtime verification	Confirmed in PR description

The PR remains in excellent shape. The bug fix is the right call — blank lines in multi-line string arguments to [Arguments] are a legitimate use case that was silently broken. Ready to merge.