perf: skip receiver registration for ordinary objects - #6790
Conversation
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. |
|
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 (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change filters non-receiver objects during registration and validates class-instance receivers. New unit tests cover objects with throwing hash codes, receiver deduplication, and single event delivery. ChangesEvent receiver registration
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to Receiver registration now skips ordinary objects before deduplication while preserving valid receiver delivery and deduplication behavior. No merge-blocking risk remains. 🚥 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 reads each line, Comment |
Greptile SummaryThis PR avoids receiver-registration work for objects that cannot receive events.
Confidence Score: 5/5The PR appears safe to merge; the optimization preserves all currently supported receiver registration paths. Every receiver interface registered by the orchestrator inherits
|
| Filename | Overview |
|---|---|
| src/TUnit.Engine/Services/EventReceiverOrchestrator.cs | Adds an early marker-interface guard before receiver deduplication and registration without changing valid receiver behavior. |
| tests/TUnit.UnitTests/EventReceiverRegistrationTests.cs | Verifies ordinary objects bypass hashing and valid attribute and class receivers remain deduplicated and invoked. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Eligible event object] --> B{Implements IEventReceiver?}
B -->|No| C[Skip registration]
B -->|Yes| D{Already initialized?}
D -->|Yes| C
D -->|No| E[Register implemented event interfaces]
Reviews (1): Last reviewed commit: "perf: skip receiver registration for ord..." | Re-trigger Greptile
Code ReviewReviewed the diff for #6790 ( SummaryThe change is well-targeted: it filters non- The benchmark methodology in the description is unusually rigorous (isolated ALCs per baseline/candidate, AB/BA paired whole-executable runs, honest reporting that the end-to-end CI is Minor findingDead defensive code in if (classInstance is not IEventReceiver)
{
return;
}
// Defense-in-depth: SkippedTestInstance is a sentinel singleton for tests skipped
// at registration time and should never be treated as an event receiver. Callers
// already short-circuit on this sentinel, but guard here too.
if (classInstance is SkippedTestInstance)
{
return;
}
No other issues found — filtering logic, dedup semantics, and the null-handling replacement ( |
Updated [TUnit.Core](https://github.com/thomhurst/TUnit) from 1.66.27 to 1.67.0. <details> <summary>Release notes</summary> _Sourced from [TUnit.Core's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.67.0 <!-- Release notes generated using configuration in .github/release.yml at v1.67.0 --> ## What's Changed ### Other Changes * docs: clarified and updated attributes comparison for xUnit 3 by @304NotModified in thomhurst/TUnit#6774 * perf: read inline argument metadata without reflection by @thomhurst in thomhurst/TUnit#6778 * perf: limit converter discovery to declarations by @thomhurst in thomhurst/TUnit#6779 * perf: skip teardown analysis when no disposable members need cleanup by @thomhurst in thomhurst/TUnit#6780 * perf: avoid line allocations when writing generated source by @thomhurst in thomhurst/TUnit#6781 * perf: avoid formatting interface names for data-source checks by @thomhurst in thomhurst/TUnit#6782 * perf: skip unannotated property data-source candidates by @thomhurst in thomhurst/TUnit#6784 * fix: fold inner exceptions into IDE test failure output by @thomhurst in thomhurst/TUnit#6777 * perf: reuse argument-free attribute initializer text by @thomhurst in thomhurst/TUnit#6788 * perf: extract test metadata in attribute transforms by @thomhurst in thomhurst/TUnit#6789 * perf: skip receiver registration for ordinary objects by @thomhurst in thomhurst/TUnit#6790 * perf: cache reporting properties on test contexts by @thomhurst in thomhurst/TUnit#6791 * fix: preserve executor registration, limiter precedence, and timeout classification by @Nice3point in thomhurst/TUnit#6768 ### Dependencies * chore(deps): update tunit to 1.66.27 by @thomhurst in thomhurst/TUnit#6742 * chore(deps): update dependency bunit to 2.10.3 by @thomhurst in thomhurst/TUnit#6745 * chore(deps): update dependency imposter to 0.1.11 by @thomhurst in thomhurst/TUnit#6744 * chore(deps): update dependency microsoft.kiota.abstractions to 2.1.2 by @thomhurst in thomhurst/TUnit#6747 * chore(deps): update dependency microsoft.templateengine.authoring.cli to v10.0.401 by @thomhurst in thomhurst/TUnit#6750 * chore(deps): update dependency fsharp.core to 10.1.401 by @thomhurst in thomhurst/TUnit#6748 * chore(deps): update dependency microsoft.templateengine.authoring.templateverifier to 10.0.401 by @thomhurst in thomhurst/TUnit#6751 * chore(deps): update dependency system.commandline to 2.0.12 by @thomhurst in thomhurst/TUnit#6752 * chore(deps): update dependency dotnet-sdk to v10.0.401 by @thomhurst in thomhurst/TUnit#6754 * chore(deps): update microsoft.extensions to 10.0.12 by @thomhurst in thomhurst/TUnit#6755 * chore(deps): update microsoft.aspnetcore to 10.0.12 by @thomhurst in thomhurst/TUnit#6753 * chore(deps): update dependency microsoft.entityframeworkcore to 10.0.12 by @thomhurst in thomhurst/TUnit#6749 * chore(deps): update mcr.microsoft.com/dotnet/sdk docker tag to v11 by @thomhurst in thomhurst/TUnit#6756 * chore(deps): update dependency microsoft.net.test.sdk to 18.10.0 by @thomhurst in thomhurst/TUnit#6761 * chore(deps): update microsoft.extensions to 10.10.0 by @thomhurst in thomhurst/TUnit#6762 * chore(deps): update react to ^19.3.0 by @thomhurst in thomhurst/TUnit#6763 * chore(deps): update dependency awssdk.sqs to 4.0.100.13 by @thomhurst in thomhurst/TUnit#6764 * chore(deps): update dependency polyfill to 11.3.0 by @thomhurst in thomhurst/TUnit#6765 * chore(deps): update dependency polyfill to 11.3.0 by @thomhurst in thomhurst/TUnit#6766 * chore(deps): update dependency stackexchange.redis to 3.2.0 by @thomhurst in thomhurst/TUnit#6769 * chore(deps): update dependency microsoft.net.stringtools to 18.10.1 by @thomhurst in thomhurst/TUnit#6771 * chore(deps): update dependency dotnet-trace to v10.0.745401 by @thomhurst in thomhurst/TUnit#6773 * chore(deps): bump colord from 2.9.3 to 2.10.0 in /docs by @dependabot[bot] in thomhurst/TUnit#6759 * chore(deps): bump joi from 17.13.4 to 17.13.7 in /docs by @dependabot[bot] in thomhurst/TUnit#6758 * chore(deps): bump js-yaml from 4.3.1 to 4.3.2 in /docs by @dependabot[bot] in thomhurst/TUnit#6757 * chore(deps): update dependency yaml to v2.9.1 by @thomhurst in thomhurst/TUnit#6785 * chore(deps): update verify to 32.0.1 by @thomhurst in thomhurst/TUnit#6786 * chore(deps): update dependency nunit.analyzers to 4.15.0 by @thomhurst in thomhurst/TUnit#6792 ## New Contributors * @304NotModified made their first contribution in thomhurst/TUnit#6774 * @Nice3point made their first contribution in thomhurst/TUnit#6768 ... (truncated) Commits viewable in [compare view](thomhurst/TUnit@v1.66.27...v1.67.0). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
EventReceiverOrchestrator currently inserts every eligible attribute, argument, context and test instance into its concurrent deduplication set, then scans all event interfaces, even when an object implements none. Filter on IEventReceiver before deduplication in both registration entry points. Actual receivers retain the existing ordering and registration paths.
For a fresh 1,000-test session, registration of plain tests takes 117.3 us instead of 575.1 us (79.6% less) and allocates 112.01 KB instead of 965.11 KB (88.4% less). With a real ITestStartEventReceiver on every class instance, time falls 32.3%, with 10.6% fewer allocations. These measurements include eligible-object cache reconstruction and both registration stages; they exclude constructing TestContext objects and running test bodies.
Validation: all 290 net10.0 unit tests passed, including new checks that ordinary objects are not hashed and that attribute/class receivers still receive callbacks. The generated 10,000-test executable passed in source-generated and reflection modes.
Whole-executable check: 20 alternating AB/BA pairs after three warmups per variant, each run required exactly 10,000 successful tests. Before: mean 980.79 ms, median 969.34 ms. After: mean 963.89 ms, median 957.15 ms. Paired mean reduction: 16.89 ms; approximate 95% t interval [-1.04, 34.83] ms. This does not establish a significant end-to-end speedup. The strong result is the isolated registration cost reduction.
Baseline:
656b66e723; candidate:57f4d2857e. BenchmarkDotNet 0.15.8, SDK 11.0.100-preview.7.26381.103, .NET 10.0.12, Windows 11, Intel i7-12700K. Both saved engine DLLs use the same Core/MTP dependencies and isolated AssemblyLoadContexts. InProcessEmitToolchain, 20 iterations and six warmups, sequential execution; no other builds/tests launched by this task during measurement. A fresh orchestrator is created per operation; every context's receiver caches are reset before registration, avoiding a warmed-dedup benchmark.Reproduce the microbenchmark
Save the project and source below in an external RuntimeBench directory. Replace the signing-key checkout path in the project. Build the baseline and PR into sibling baseline-runner and idea3-runner directories:
RuntimeBench.csproj:
Program.cs:
ReceiverBench.cs:
Raw whole-executable samples (milliseconds)
```csv "Pair","Variant","Milliseconds" "1","Before","969.5005" "1","After","945.695" "2","After","939.51" "2","Before","994.2921" "3","Before","1059.775" "3","After","1011.7146" "4","After","935.254" "4","Before","949.9259" "5","Before","956.6343" "5","After","971.3426" "6","After","979.22" "6","Before","973.7481" "7","Before","986.6671" "7","After","951.7484" "8","After","955.8747" "8","Before","1047.395" "9","Before","990.9323" "9","After","961.0989" "10","After","934.141" "10","Before","954.4484" "11","Before","1014.3478" "11","After","950.412" "12","After","958.4172" "12","Before","948.5221" "13","Before","944.0799" "13","After","990.4228" "14","After","955.0782" "14","Before","969.1712" "15","Before","959.1917" "15","After","935.0701" "16","After","991.0562" "16","Before","957.9144" "17","Before","977.7508" "17","After","947.2239" "18","After","979.8796" "18","Before","1048.004" "19","Before","961.7752" "19","After","1002.1741" "20","After","982.5291" "20","Before","951.6738"