perf(analyzers): cut TUnit analyzer build time ~60% on large test projects - #6862
Conversation
- Add IsGloballyQualified/IsGloballyQualifiedNonGeneric helpers that reject on the symbol's simple name before building a display string, and use them for every per-attribute / per-base-type / per-parameter / per-invocation name comparison (SingleTUnitAttribute, DependsOnConflict, hooks, data sources, Timeout, ConsoleOut, MixAndOr, ObjectBaseEquals, AwaitValueTaskAssertThat...). - CompilerArgumentsPopulatedAnalyzer: operation action on OperationKind.Argument (non-implicit, ArgumentSyntax) instead of SemanticModel.GetOperation for every ArgumentSyntax in the compilation. - AwaitAssertion/XUnitAssertion/GenerateAssertion/ClassDataSourceConstructor: cheap name pre-filters before display strings. - Pass stable strings to GetTypeByMetadataName (its cache is keyed by string reference); cheaper check ordering in MissingTestAttribute and PublicMethodMissingTestAttribute; skip SingleTUnitAttribute work for < 2 attributes. Diagnostics are unchanged: an equivalence test checks the helpers against plain display-string comparison over a broad symbol matrix, new CompilerArgumentsPopulated tests pass against both the old and new implementation, and the reported diagnostics for TUnit.TestProject and TUnit.Assertions.Tests are identical before/after.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
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 (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds allocation-saving qualified-name predicates, replaces direct display-string comparisons across both analyzer projects, updates analyzer execution paths, and adds comparison and compiler-argument tests. ChangesQualified symbol matching
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The optimization is mergeable; reviewed analyzer behavior and shared helper wiring preserve the existing contracts. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 6.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 35 files. (1 skipped: 1 unsupported.)
✨ 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 reads each line, Comment |
Code ReviewReviewed the full diff (33 files, analyzer perf work cutting build time ~60% by adding Correctness: I traced the new hand-rolled parser ( One structural finding:
No other structural or design concerns. Nice work on making a hot-path allocation optimization provably safe via the fallback-to- |
…alyzer projects Move GloballyQualified/IsGloballyQualified and their name-prefilter helpers into TypeExtensions.GloballyQualified.cs (partial TypeExtensions) in TUnit.Analyzers and link it into TUnit.Assertions.Analyzers. The namespace is selected via TUNIT_ASSERTIONS_ANALYZERS so DisplayFormats still binds to each assembly's own (differing) formats and call sites are unchanged.
|
Addressed the duplication in 1ae9a70. The One thing to know: the code was identical, but each assembly has its own TUnit.Analyzers.Tests (2352 passed, 3 skipped) and TUnit.Assertions.Analyzers.Tests (336 passed) both pass. |
|
Summary
meziantou's framework benchmark found TUnit builds about 2× slower than the other frameworks at 10,000 tests. Profiling with
-p:ReportAnalyzer=trueshowed the source generator itself is cheap. A large share of the extra time was TUnit's Roslyn analyzers doing expensive work on every syntax node, even in a project with no assertions at all:CompilerArgumentsPopulatedAnalyzercalledSemanticModel.GetOperationon every argument in the compilation.symbol.GloballyQualified() == "global::...", which builds a display string for every candidate symbol.BeforeHookAsyncLocallooked upGetTypeByMetadataNamewith a freshly built string on every call, so the per-compilation cache always missed.Changes:
IsGloballyQualified/IsGloballyQualifiedNonGenerichelpers compare the symbol's simple name first and only build the display string when that name matches. All hot-path comparisons now use them. A new test checks that the helpers always agree with the plain string comparison, over more than 10,000 symbol/name pairs covering generics, nested types, arrays, nullables, tuples and methods.CompilerArgumentsPopulatedAnalyzer: now an operation action limited to explicitly written arguments, so it no longer callsGetOperationfor each syntax node.The reported diagnostics are unchanged. Running
main's and this branch's analyzers over the fullTUnit.TestProjectandTUnit.Assertions.Testssources gives identical results (226 and 60 diagnostics, including 3TUnitAssertions0003).Benchmark
Median of 6 interleaved builds of the 10,000-test project with
-p:ReportAnalyzer=true -p:UseSharedCompilation=false, comparing against currentmain. The machine was shared, so treat these as directional:A controlled harness that ran the analyzers warm and single-threaded gave TUnit analyzer time per pass of 343–606 ms on
mainand 130–194 ms here (medians).On
TUnit.TestProject, TUnit.Assertions.Analyzers went from 0.57 s to 0.18 s and total analyzer time from 6.6 s to 5.7 s.Tests
TUnit.Analyzers.Tests: 784 passed, 1 skipped (already skipped onmain).TUnit.Assertions.Analyzers.Tests: 112 passed.TUnit.Assertions.Analyzers.CodeFixers.Tests: 19 passed.tests/SharedTestHelpers/GloballyQualifiedComparisonCases.cs.CompilerArgumentsPopulatedtests covering named arguments, lambdas, local functions, field and property initializers, constructor initializers, nested calls, and non-TUnit caller-info parameters. They pass on both the old and new implementations.Remaining big costs:
DisposableFieldPropertyAnalyzerandTestDataAnalyzer. Their time is mostly method-body binding throughGetOperation, and I left them alone because the cheaper pre-filters I could see risked changing results.Summary by CodeRabbit
Bug Fixes
Tests