Repository navigation
feat(topic): add Topic.latest() for a single-read value + timestamp - #30
Closed
IamCoder18 wants to merge 6 commits into
Closed
IamCoder18 wants to merge 6 commits into
IamCoder18 wants to merge 6 commits into
Conversation
`latestValue` and `latestPublishNanos` were each `synchronized` on the Topic instance. That monitor is the most-used read on the bus, so it serialised every reader against every writer. The value and its timestamp are now published together as one immutable `Latest` pair behind a single volatile field, which makes both accessors plain volatile reads. The write path keeps a monitor, but only around the `nanoTime()` sample and the store. Two volatile fields would not do: `publish` is reachable from the OpMode loop, the hardware thread and the callback pool, so two publishers can interleave between a value write and a timestamp write and leave the pair describing two different publishes -- a regression against the old monitor, which ordered those writes. Sampling the clock outside mutual exclusion is not sufficient either: a publisher that samples and is then preempted installs an OLDER snapshot after a newer one, so a reader can hold an older value beside a newer timestamp and an age computed from that stamp is under-reported. Holding the monitor across the sample and the store keeps install order equal to timestamp order, which is the property the old body had. The type work is hoisted out of the publish path: - `boxedType` is computed once in the constructor instead of re-normalising the declared type on every publish and every lookup. `Topic.type()` still reports the type the topic was created with; only the internal comparison field is boxed. - `acceptsType(Class)` is the old `boxed(t.type()).isAssignableFrom(boxed(other))` with the topic side hoisted, replacing that expression at five call sites. `acceptsValueClass(Class)` delegates to it, so the two cannot drift. `boxed()` moves from `OrchestratorImpl` to `Topic`, next to the field it reads. Two separate accessors can still straddle a publish, so the read order remains: sample the timestamp first. That can only over-report the value's age, never under-report it. This is documented on the public accessors, in the CHANGELOG and in the topics guide, and the guide's example had the unsafe order. No API or behaviour change; `latestPublishNanos()` still returns 0 before the first publish. Measured on the repo benchmark (`gradle :benchmarks:run --args='run --quick --scenarios S0 --styles synapse'`, 8 alternating runs of baseline and branch on one box, p50 ns/op, median): micro.topic.latestValue 44 ns -> 28 ns (-36%) micro.topic.recordLatest.1p1c 360 ns -> 192 ns (-47%) micro.topic.recordLatest.4p4c 340 ns -> 268 ns (-21%) micro.topic.recordLatest 89 ns -> 83 ns (-7%) micro.publish.subscribers0 140 ns -> 161 ns (+15%) The read-path win is the durable one. The contended publish numbers are much smaller than a fully lock-free write path would give, because the write path keeps its monitor; that is the cost of not regressing the timestamp ordering the old code had. `publish.subscribers0` and the `rawDirectCall` control both moved by more than this box's noise floor in the same runs, so read those two as inconclusive rather than as a regression. Treat every number as a ratio on the box that produced it.
Reviewer's GuideAdds Sequence diagram for consistent topic snapshot readssequenceDiagram
participant Caller
participant Topic
participant Snapshot as Topic.Latest
Caller->>Topic: latest()
Topic-->>Caller: Optional<Latest<T>>
Caller->>Snapshot: value()
Snapshot-->>Caller: published value
Caller->>Snapshot: ageNanos()
Snapshot-->>Caller: age from publishNanos()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/main/java/com/aaravlabs/synapse/Topic.java" line_range="234-235" />
<code_context>
+ *
+ * @return an {@link Optional} holding the latest value and its publish timestamp
+ */
+ public Optional<Latest<T>> latest() {
+ return Optional.ofNullable(latest);
}
</code_context>
<issue_to_address>
**issue (performance):** Every non-empty call to `latest()` creates a new `Optional` wrapper through `Optional.ofNullable(latest)`, so the new snapshot API allocates on each read and can add GC pressure on periodic or high-frequency readers. This contradicts the description's claim that the `Optional<Topic.Latest<T>>` shape allocates nothing.
**Triggers:** When callers poll `latest()` frequently, especially on the small-young-generation deployment described in the PR.
**Suggested fix:** Return a nullable snapshot from a separate allocation-free API, or document that `latest()` allocates an `Optional` wrapper and provide an allocation-free alternative for hot paths.
</issue_to_address>
### Comment 2
<location path="src/test/java/com/aaravlabs/synapse/TopicLockFreeTest.java" line_range="139-142" />
<code_context>
+ return;
+ }
+ // ageNanos must agree with the stamp it was taken from.
+ long age = snap.ageNanos();
+ if (age <= 0 || age > System.nanoTime() - snap.publishNanos() + 1) {
+ failure.compareAndSet(null, new AssertionError(
+ "implausible age " + age + " for stamp " + snap.publishNanos()));
return;
}
</code_context>
<issue_to_address>
**issue (testing):** The new tests require a strictly positive age and a strictly smaller age for the later publish, but `System.nanoTime()` is allowed to return the same tick for adjacent reads. A valid snapshot can therefore have `ageNanos() == 0`, and two publishes can temporarily report equal ages, causing the test to fail without a production bug.
**Triggers:** On platforms or runs where the monotonic clock has coarser resolution than the time between the snapshot and age read, or between the two publish checks.
**Suggested fix:** Use non-strict comparisons where clock resolution permits equality, or wait until the clock has advanced before asserting a strictly positive/younger age.
</issue_to_address>
### Comment 3
<location path="website/src/content/docs/concepts/topics.mdx" line_range="58-60" />
<code_context>
+// One snapshot read gives the value and its timestamp together, so the two can never
+// come from different publishes. ageNanos() is the age of this exact value, and needs no
+// special handling before the first publish -- latest() is simply empty then.
+Optional<Topic.Latest<Double>> flSnapshot = flTopic.latest();
+double fl2 = flSnapshot.map(Topic.Latest::value).orElse(0.0);
+long nanosSinceUpdate = flSnapshot.map(Topic.Latest::ageNanos).orElse(Long.MAX_VALUE);
```
</code_context>
<issue_to_address>
**nitpick:** The added documentation presents `Topic.latest()` as public API, but the API reference's `Topic<T>` table still omits both `latest()` and the public `Topic.Latest<T>` type. Users consulting the advertised complete public surface cannot discover or correctly use the new API.
**Triggers:** When users rely on `website/src/content/docs/api/index.mdx` or generated site navigation rather than the topics guide.
**Suggested fix:** Add `latest()` and the `Latest<T>` accessors (`value()`, `publishNanos()`, and `ageNanos()`) to the API reference and other maintained API-surface references.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: src/main/java/com/aaravlabs/synapse/Topic.java:235, src/test/java/com/aaravlabs/synapse/TopicLockFreeTest.java:142
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
IamCoder18
force-pushed
the
feat/latest-snapshot-accessor
branch
from
October 2, 2026 01:40
d6d339a to
5cf48a7
Compare
Checking whether a published value is stale today means two calls:
long stamp = topic.latestPublishNanos();
T v = topic.latestValueOr(null);
long age = System.nanoTime() - stamp;
Both accessors are individually correct, but a publish can land between
them, leaving the caller holding a value from one publish and a timestamp
from another. Reading the timestamp first makes that safe in one
direction -- the age can only be over-reported, never under-reported --
but over-reporting still discards a fresh value as stale, by up to a full
publish interval.
How often that happens depends on how long the two reads take. Normally
they are nanoseconds apart and a publish landing between them is
vanishingly rare. It stops being rare when the reader is preempted
between the two calls by a GC pause or the scheduler, since the window
becomes milliseconds. This library configures a small young gen, so young
GCs are part of the deployment rather than a hypothetical.
`latest()` returns the value and its timestamp from one snapshot read, so
the pair provably comes from a single publish and the window does not
exist:
Optional<Topic.Latest<T>> snap = topic.latest();
T v = snap.map(Topic.Latest::value).orElse(default);
long age = snap.map(Topic.Latest::ageNanos).orElse(Long.MAX_VALUE);
Additive only. No existing signature or behaviour changes, and callers
that never check an age can keep using `latestValueOr` -- same single
volatile read.
`Topic.Latest` becomes public with `value()`, `publishNanos()` and
`ageNanos()`. `ageNanos()` is the age of *this* value computed from its
own stamp, which also removes the pre-publish `0` sentinel that the
two-call form has to guard against by hand.
`latestPublishNanos()` still returns 0 before the first publish, so the
guard stays necessary for anyone composing it by hand. It is documented as
such in the javadoc, the CHANGELOG and the topics guide.
The concurrency test's reader now uses `latest()`, which is the stronger
property: no straddle is representable, so the "timestamp first"
ordering rule has nothing left to guard. That test previously carried a
cross-check between `latest()` being empty and `latestPublishNanos()`
being nonzero; it is gone, because a publish can legitimately land
between those two reads and a nonzero stamp there says nothing about
consistency.
Suite: 61 tests, 0 failures.
IamCoder18
force-pushed
the
feat/latest-snapshot-accessor
branch
from
October 2, 2026 01:52
5cf48a7 to
eeef85d
Compare
IamCoder18
force-pushed
the
perf/topic-lock-free
branch
from
October 2, 2026 01:53
54d881e to
f801502
Compare
`latestValue` and `latestPublishNanos` were each `synchronized` on the Topic instance. That monitor is the most-used read on the bus, so it serialised every reader against every writer. The value and its timestamp are now published together as one immutable `Latest` pair behind a single volatile field, which makes both accessors plain volatile reads. The write path keeps a monitor, but only around the `nanoTime()` sample and the store. Two volatile fields would not do: `publish` is reachable from the OpMode loop, the hardware thread and the callback pool, so two publishers can interleave between a value write and a timestamp write and leave the pair describing two different publishes -- a regression against the old monitor, which ordered those writes. Sampling the clock outside mutual exclusion is not sufficient either: a publisher that samples and is then preempted installs an OLDER snapshot after a newer one, so a reader can hold an older value beside a newer timestamp and an age computed from that stamp is under-reported. Holding the monitor across the sample and the store keeps install order equal to timestamp order, which is the property the old body had. The type work is hoisted out of the publish path: - `boxedType` is computed once in the constructor instead of re-normalising the declared type on every publish and every lookup. `Topic.type()` still reports the type the topic was created with; only the internal comparison field is boxed. - `acceptsType(Class)` is the old `boxed(t.type()).isAssignableFrom(boxed(other))` with the topic side hoisted, replacing that expression at five call sites. `acceptsValueClass(Class)` delegates to it, so the two cannot drift. `boxed()` moves from `OrchestratorImpl` to `Topic`, next to the field it reads. Two separate accessors can still straddle a publish, so the read order remains: sample the timestamp first. That can only over-report the value's age, never under-report it. This is documented on the public accessors, in the CHANGELOG and in the topics guide, and the guide's example had the unsafe order. No API or behaviour change; `latestPublishNanos()` still returns 0 before the first publish. Measured on the repo benchmark (`gradle :benchmarks:run --args='run --quick --scenarios S0 --styles synapse'`, 8 alternating runs of baseline and branch on one box, p50 ns/op, median): micro.topic.latestValue 44 ns -> 28 ns (-36%) micro.topic.recordLatest.1p1c 360 ns -> 192 ns (-47%) micro.topic.recordLatest.4p4c 340 ns -> 268 ns (-21%) micro.topic.recordLatest 89 ns -> 83 ns (-7%) micro.publish.subscribers0 140 ns -> 161 ns (+15%) The read-path win is the durable one. The contended publish numbers are much smaller than a fully lock-free write path would give, because the write path keeps its monitor; that is the cost of not regressing the timestamp ordering the old code had. `publish.subscribers0` and the `rawDirectCall` control both moved by more than this box's noise floor in the same runs, so read those two as inconclusive rather than as a regression. Treat every number as a ratio on the box that produced it.
IamCoder18
force-pushed
the
perf/topic-lock-free
branch
from
October 2, 2026 01:55
f801502 to
b4dcf33
Compare
Brings #29's final review fixes onto the accessor branch so it is not based on a stale parent: the per-publish allocation is now documented in the write-path comment, the javadoc and the CHANGELOG, and the 0-before-first- publish caveat is stated as a heuristic rather than a proof.
System.nanoTime() is permitted to return 0, so a 0 stamp is not proof that nothing has been published -- a genuine publish can carry one too. Stated in the latestPublishNanos() javadoc, the latestValue() example, the topics guide and the CHANGELOG, along with which way the resulting over-reported age fails: it rejects a fresh value rather than admitting a stale one. latest() on this branch removes the question entirely, since one snapshot read needs no sentinel.
IamCoder18
force-pushed
the
perf/topic-lock-free
branch
from
October 2, 2026 02:56
b4dcf33 to
34f8ae8
Compare
Syncs the accessor branch with perf/topic-lock-free's final head, keeping latest() and its test. Applies the last three findings: - Drop the hard-coded 24-byte Latest size; object size is runtime-dependent and this library also targets FTC/Android VMs. - Scope the 'read accessors do not allocate' claim to latestValueOr(). latestValue() and latest() both wrap their result in an Optional, which they did before this branch. - Guard the CHANGELOG age snippet against the pre-publish 0 stamp, matching the javadoc and the topics guide. Tests 61/61, javadoc at the pre-existing 2-warning baseline.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
Topic.latest(), which returns the latest value and its publish timestamp from a single snapshot read, so a staleness check cannot combine halves from two different publishes.This is the API follow-up to #29. #29 made the topic's internals safe and documented an ordering rule for composing the existing accessors. This removes the need for that rule.
The problem it solves
Checking staleness today takes two calls:
Both accessors are individually correct. But a publish can land between them:
Reading the timestamp first makes that safe in one direction — the age can only be over-reported, never under-reported — but over-reporting by a full publish interval still makes you reject a fresh value as stale. On a 100 Hz sensor with a 50 ms budget that is 20% of your margin, spent for nothing.
How often this happens depends on how long the two reads take. Normally they are nanoseconds apart and the odds are negligible. It stops being negligible when the reader is preempted between the two calls — a GC pause or scheduler preemption — because the window becomes milliseconds. This project deliberately configures a small young gen (
-Xmn16m,-Xmx256m), so young GCs are part of the deployment, not a hypothetical. On a Control Hub also running the driver station and telemetry, a reader landing in a GC pause between two adjacent reads is not exotic.The API
Topic.Latestbecomes public with three accessors:value()publishNanos()System.nanoTime()at which it was recordedageNanos()publishNanos()— the age of this valueageNanos()is computed from the object's own stamp, so it also removes the pre-publish0sentinel that the two-call form has to guard by hand.latestPublishNanos()still returns0before the first publish, so that guard remains necessary for anyone composing by hand — it is now documented as such in the javadoc, CHANGELOG and topics guide rather than left implicit.Additive only. No existing signature or behaviour changes. Callers who never check an age keep using
latestValueOr— identical single volatile read, and allocation-free wherelatest()is not.It allocates.
Optional.ofNullablewraps every non-empty result, solatest()costs one small object per call. That matters in a periodic loop on the small young gen this project configures, so the javadoc says so and points atlatestValueOrfor value-only reads. An earlier draft of this description claimedlatest()"allocates nothing"; that was wrong and is corrected here.Why the ordering rule disappears
#29's rule was "sample the timestamp first, because two calls can straddle." With one read there is no straddle to guard, so:
latestValue()javadoc now says "preferlatest()" rather than teaching a rulelatest(), so the rule has no remaining subjectThat is the part I think matters most: it is not that the rule became easier to remember, it is that there is no longer anything to remember.
Tests
Suite 61 tests, 0 failures. Two of these assertions were wrong when first written and are worth noting, since they are the same error twice: I required a strictly positive age, and used a fixed
Thread.sleep(2)to guarantee clock movement.System.nanoTime()may return the same tick for adjacent reads, so both could fail on a coarse-resolution clock without a production bug.ageNanos() == 0is legitimate; the fixed sleep now waits on the clock with a bounded loop, reusing the pattern from the existing test.New
latestReturnsValueAndTimestampFromOneConsistentPublish: empty before the first publish; agrees with both single-field accessors; a published stamp is never the0sentinel;ageNanos()is in range and never exceeds a delta measured later from the same stamp (a smaller delta would mean the age came from somewhere other than that stamp); a later publish replaces both halves together and reports a younger age.The concurrency test's reader switched to
latest(). Its old cross-check between "latest()empty" and "latestPublishNanos()nonzero" had to be deleted — a publish can legitimately land between those two reads, so a nonzero stamp there says nothing about consistency. I found this the hard way: the assertion fired on the first run after the switch.Verification
gradle test→ 61 tests, 0 failures, from cleantaskset -c 0,1)gradle javadoc→ no new warningsOpen questions for the maintainer
latest()earn public API space, or should it stay internal? It is the honest fix for the straddle, but it is also a new type in the public surface with a name (Latest) that could collide with user vocabulary.latestValue()andlatestPublishNanos()be deprecated? They are still correct and still the right choice when you only want one. I have deliberately not deprecated anything — that is a maintainer decision, and it would break the "purely additive" property of this PR.Optional<Topic.Latest<T>>allocates one small wrapper per call, which is a real cost in a periodic loop on a small young gen; a nullable-returninglatestOrNull()would be allocation-free. I choseOptionalto matchlatestValue()and make the empty case impossible to ignore, but if hot-path staleness checks matter more than the ergonomics, the allocation-free variant is the better shape and I would rather you chose it than have me guess. The cost is documented on the accessor and in the guide either way.Summary by Sourcery
Provide an atomic latest-value snapshot API for reliable staleness checks without changing existing accessors.
New Features:
Topic.latest()API that returns the latest value and publish timestamp from one consistent snapshot, withLatestaccessors for value, timestamp, and age.Bug Fixes:
Enhancements:
Documentation:
Tests: