perf(source-gen): bound generated test-entry methods (data-driven startup JIT -45%) - #6909
Conversation
…ethods Entries for data-driven tests construct nested objects (ArgumentsAttribute, ParameterMetadata, return types) while the outer factory call's arguments sit on the evaluation stack. The JIT spills them to fresh temporaries at every nested call, so a 100-test class's static constructor (~17KB IL) took ~60ms to JIT versus ~6ms for plain tests: superlinear in entry count. Classes with more than 10 such entries now fill the Entries array from __FillEntriesN methods of 10 entries each. Data-driven 10k suite: generated code JIT 3.55s -> 1.96s CPU. Plain test classes are emitted unchanged.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe source generator uses a direct array initializer unless nested-construction entries exceed 10. Above that threshold, it allocates the entry array and populates it through fill methods. New tests cover chunking limits, entry ordering, and generated output. ChangesTest-entry generation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to No identified issue needs resolution before merging; the changed generation paths retain the intended entry ordering and chunking behavior. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The generated entries are now built in smaller methods, but they retain the existing registration and factory behavior. No new security exposure was identified. Security-review coverage is incomplete, so the assessment is low risk rather than minimal risk. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks each entry row Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23a3ee3235
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
 |
Code Review — PR #6909This PR splits the per-class generated `Entries` array into chunked `__FillEntriesN` methods (batches of ~10 entries) to reduce JIT compilation time for data-driven test classes with many entries. Changes are confined to `src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs` plus 11 mechanically regenerated `.verified.txt` snapshots. Review approach: Traced the chunk-boundary math, `CodeWriter` indentation/brace state across the refactor, the `HasNestedEntryConstruction` heuristic against every `PreGenerate*` helper it depends on (confirming `categories`/`properties`/`dependsOn` array literals are correctly excluded since they contain no nested calls), naming-collision risk for the new `__CreateEntries`/`__FillEntriesN` methods, evaluation-order/side-effect preservation, and AOT/trim-safety parity with the existing `__CreateInstance`/`__Invoke`/`__Attributes` generated methods. Cross-checked the regenerated `ArgsAsArrayTests.Test.verified.txt` and `MatrixTests` snapshots (13-entry classes splitting correctly into indices 0–9 and 10–12) to confirm the generator's actual output matches the intended behavior. Findings: None. No correctness bugs, dropped invariants, or violations of the project's AGENTS.md constraints (source-generator/engine parity doesn't apply here since this is generator-only output shaping; snapshot `.verified.txt` files were updated as required, no `.received.txt` committed). Minor observation (non-blocking): There's no test targeting the exact chunk boundary (10 vs. 11 entries, with/without nested construction) in isolation — coverage is incidental via existing large test-project classes that happen to cross the boundary. Not a defect in the diff as written, just a potential gap if the chunk size constant is ever tuned in the future. 🤖 Generated with Claude Code |
Only entries that build nested objects add JIT spill temporaries, so plain entries no longer count toward a fill method's budget. A class is chunked only when it has more than 10 nested-construction entries, and runs of plain entries stay in the current fill method. Fully data-driven classes produce the same output as before, while a large class of plain tests with a few data-driven ones keeps its single array initializer instead of being split into one method per 10 entries. Adds snapshot coverage for the 10/11 boundary with and without nested construction, a plain-heavy class with one data-driven test, and interleaved plain and data-driven entries.
GitHub Actions
Error
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review — PR #6909Reviewed the source-generator chunking change in What I checked
Result: No correctness bugs, regressions, or convention violations found. This aligns with the existing Codex and CodeRabbit automated reviews on this PR, which also reported no actionable findings — nothing outstanding from those to reconcile. Approving from a correctness/design standpoint; nice minimal fix for the IL-stack-spill issue with solid boundary-condition test coverage. |
Plain entries did not count toward a fill method's budget, so a chunked class with a long run of plain tests could put hundreds of factory calls in one __FillEntriesN method. A fill method now also closes at 100 entries in total. 100 matches the measured plain-class shape (~6ms JIT per 100 plain entries), and each extra fill method costs ~0.15ms (from the 1-entry-per-method measurement), about 2.5% of such a chunk. Classes with 10 or fewer nested-construction entries are still never chunked, and fully data-driven classes are unchanged. Adds a fixture with 11 data-driven entries around 100 plain entries, split at the total cap.
Review: bound generated test-entry methods (#6909)Solid, well-measured performance fix. Summary of what I checked: Correctness of the chunking logic ( Iteration history: the PR's three commits show the design tightening itself in exactly the right direction — from "chunk whenever nested-construction entries exceed 10" to also capping total entries per fill method at 100, which correctly closes the gap where a chunked class with a long run of cheap plain tests could otherwise dump hundreds of factory calls into a single Engine/generator parity: AOT/trimming/hot-path constraints: no reflection introduced; all new logic ( Tests: good coverage — snapshot tests for the 10/11 nested-entry boundary, plain-only, interleaved plain/nested, and the new 100-entry total cap, plus I didn't find any functional bugs, ordering hazards, or convention violations. No blocking changes requested. Minor, non-blocking observations:
Nice work isolating the JIT-cost root cause (stack-spilled temporaries from nested constructor calls) and validating the chunk size empirically rather than guessing. |
This branch was successfully deployed
1 participant
Summary
For data-driven tests, TUnit's generated test-source static constructors were extremely expensive to JIT. At 10,000
[Arguments]tests, the generated code cost 3.5 seconds of JIT CPU during startup. The equivalent plain tests cost 0.8 seconds. This is a large part of TUnit's lag behind MSTest in the DataDriven scenario of meziantou's framework benchmark.Root cause
Each
Entrieselement is aTestEntryFactory.CreateWithClassMetadata(...)call. For data-driven tests, some of its arguments build nested objects:new IDataSourceAttribute[] { new ArgumentsAttribute(...) },ParameterMetadataFactory.Create(typeof(...), ..., new ConcreteType(...), ...), andtypeof(...)return types. Each of these nested calls runs while the outer call's already-evaluated arguments are still on the IL evaluation stack. The JIT spills those pending arguments into new temporaries at every nested call.As a result, the number of temporaries grows with the number of entries in the method, and tier-0 JIT time grows faster than linearly:
[Test][Test, Arguments(n)]Plain entries pass only constants and cached fields, so they are unaffected.
Fix
When a class has more than 10 entries and its entries contain nested construction (data sources, parameters, dependencies, or a non-void return type),
Entriesis now filled by__FillEntriesNmethods of 10 entries each:Plain test classes and classes with 10 or fewer entries are emitted exactly as before. Only 5 snapshot tests change: the large parameterised classes.
I chose the chunk size by measuring a 10,000-test data-driven suite. Generated-code JIT CPU for each chunk size:
Benchmarks
Packages built from
mainvs this branch; meziantou's harness project shapes; 10,000 tests, 100 per class.JIT CPU from runtime JIT events (
MethodJittingStartedtoMethodLoadVerbose). This metric is robust to machine load; two runs each:Wall clock, DataDriven 10k, interleaved A/B (4 rounds of 5 runs, medians in ms). Another heavy process was running on the machine, so absolute values are inflated; the relative difference was consistent across rounds:
Test plan
TUnit.Core.SourceGenerator.Testspass on net10.0, net9.0, net8.0 and net472; 5 snapshots updated (the large parameterised classes:ArgsAsArrayTests,ConflictingNamespaceTests,DecimalArgumentTests,MatrixTests,STAThreadTests)TUnit.TestProject(source-gen mode):MatrixTests(275),ArgsAsArrayTests(15),DecimalArgumentTests(18) andSTAThreadTests(1515) all pass, with the same test counts asmainSummary by CodeRabbit