Repository navigation
perf(core): cache the hardware facades, hoist per-message reflection - #28
Conversation
Two small allocations and one repeated computation on the per-message path.
`orchestrator.hardware()` built a new `HardwareActions` on every call. Hot
subscribers call it once per message, so a teleop handler allocated a facade
per gamepad event for no reason. `HardwareActions` and `HardwareView` are
immutable one-field views over the orchestrator, so both are now built once in
the constructor and shared. `scheduleHardwareBulkRead` likewise reused a single
`HardwareView` instead of allocating one per registration.
Behaviour note: `hardware()` now returns the same instance on every call.
The object holds no per-call state, so nothing could observe a difference
except code that relied on getting a fresh object, which was never a
documented guarantee.
In the annotation binder, the primitive-to-wrapper normalisation for a
`@SubscribedTo` parameter was recomputed on *every delivered message*:
paramType.isPrimitive() ? boxed(paramType).isInstance(msg)
: paramType.isInstance(msg)
`boxed` returns non-primitives unchanged, so both branches already collapsed
to the same check; normalising once at bind time and testing the result is
equivalent and does the work once instead of per message. Also drops a dead
`handlerBody` lambda in that method, which was constructed, given an
unreachable try/catch, and never invoked.
No API or behaviour change. Covered by new tests pinning primitive-parameter
delivery, the silent-drop branch, the shared facade, and bulkRead.
…cycle Follow-up to the facade-caching commit. Two documentation problems it introduced or left behind. The Javadoc on `OrchestratorImpl.hardware()` was placed *after* the `@Override` annotation. Javadoc only associates a doc comment with a declaration when nothing but annotations sit between them, so the block was discarded entirely — the generated HTML showed only "Description copied from interface: Orchestrator". Since the build publishes a `javadocJar`, the one place meant to state that the facade is now safe to capture and reuse never reached consumers. The `javadoc` task emits no warning for this, so the build could not have caught it. The block now precedes `@Override`, matching every other documented method in the class, and verifies in the regenerated output. `HardwareView`'s class Javadoc still claimed "Instances are created per-callback by `HardwareActions.bulkRead`". There is now one instance per orchestrator, built in the constructor and shared by every registration, so the description is corrected and links `bulkRead` properly. Also: - Move the two new facade fields into the state block beside `hardwareThread` and the other finals, rather than declaring them after the constructor. - Trim comments to match CONTRIBUTING's "minimal comments" convention. The equivalence argument in the annotation binder is kept — it is the part a future "optimization" could plausibly try to undo — while the restatements of adjacent code are not. - Drop a tautological `assertFalse(seen.isEmpty())` from the new bulkRead test, which only re-checked what the preceding latch assertion already guaranteed, along with its unused list. - Add a CHANGELOG entry under [Unreleased] for both behavior changes, noting the caching is not a behavioral change beyond object identity. No logic changes. Full suite green: 55 tests, 0 failures. `javadoc` builds with no new warnings.
- Use explicit Float.valueOf assertion instead of relying on assertEquals overload resolution. - Replace Thread.sleep with assertFalse(latch.await) for the mismatched-message negative case so a regression fails fast instead of sleeping then checking.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🔇 Additional comments (5)
📝 WalkthroughWalkthroughThe orchestrator now reuses one ChangesHardware object reuse
Subscribed message dispatch
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The changes reuse hardware wrappers and move type normalization out of message dispatch without an established behavioral regression. Mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The shared objects remain tied to their original orchestrator and retain no callback-specific mutable state. Hardware scheduling, cancellation, shutdown, and message-type filtering remain unchanged. No introduced or worsened security risk was identified in the reviewed changes. 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)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches📝 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. Comment |
|
We've triggered an ultrareview automatically — This rewrites the core dispatch hot path: hardware facades and views become shared long-lived objects used from the hardware thread, and the annotation binder's per-message type normalisation is hoisted, so a subtle lifetime, threading, or type-matching bug could silently affect every.... I'll post findings when complete. An ultrareview is cubic's deepest review, catching hard-to-find bugs in the most critical PRs. It runs a longer, multi-pass analysis using cubic's most capable review models, and typically takes around 30 minutes. It consumes your team's reviewed-lines allowance at 3× the standard rate. Automated ultrareviews are disabled by default. We triggered this run as part of your trial. Want cubic to do this for every high-risk PR? Enable auto-ultrareview in your settings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 4m 55s
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
The drop test only asserted the handler did not run, which cannot tell an intentional silent drop apart from a delivery that threw and was swallowed by the binder's catch (Throwable). Removing the isInstance guard in wireSubscribedTos makes m.invoke raise IllegalArgumentException, the handler still never runs, and the test still passes — so the regression it guards against was undetectable. Build the orchestrator with a recording LogSink and assert no error was logged after the mismatched publish. A dropped message leaves no trace; a failed delivery is logged.
1 participant
Summary
Two allocation/CPU cleanups on the core dispatch hot path, both behaviour-preserving:
Orchestrator.hardware()returns a shared facade.HardwareActionswasallocated on every call, and
bulkReadallocated a freshHardwareViewperregistration. Both are immutable single-reference views over the
orchestrator, so they are now built once in the constructor and shared. The
only observable change is object identity — which means it is now safe for
callers to capture the facade once and reuse it on a hot path.
@SubscribedTodispatch no longer re-boxes the parameter type per message.AnnotationBinderrecomputedparamType.isPrimitive() ? boxed(paramType) : paramTypeinside the delivery lambda. The normalisation is now done once at bind time
and captured. Equivalent, because
boxed()already returns non-primitivesunchanged.
Also drops a dead
handlerBodyRunnablein the binder that was constructedand immediately discarded — its own comment admitted it was unused.
Changes
OrchestratorImpl— cachehardwareActions/hardwareViewas final fields;bulkReadreuses the shared view;hardware()returns the cached facade.AnnotationBinder— hoist primitive→wrapper normalisation out of thedelivery lambda; remove the dead
handlerBody.HardwareView— Javadoc corrected: instances are per-orchestrator and sharedacross
bulkReadregistrations, not per-callback as previously documented.hardware()— Javadoc documenting the shared-identity guarantee.CHANGELOG.md— both changes under[Unreleased] / Changed.Tests
New
CoreHotPathTestcovering the specific behaviour these changes touch:hardware()returns the same instance across calls, and the cached facadestill reaches the hardware thread (guards against caching something frozen or
detached).
reference-typed messages — the two sides of the hoisted normalisation.
on the same topic still delivered. The topic is created as
Objectfirstbecause topic types are first-writer-wins, which is what makes the drop
branch reachable.
bulkReadcallbacks receive a workingHardwareView(publish +getLatestValue).Full suite passes:
gradle test→ BUILD SUCCESSFUL.Review notes
Two things worth a look during review:
HardwareViewlifetime is now tied to the orchestrator rather than to aregistration. That is fine because the view only holds a reference back to
the orchestrator and nothing else, but it does mean a
HardwareViewis nowvalid after its
BulkReadHandleis cancelled. Nothing in the tree relies onthe old per-registration lifetime.
OrchestratorImplassignshardwareActions/hardwareViewat the end of theconstructor, after the executors. They only need
this, but the orderingkeeps the final-field initialisation block together.