refactor(backpressure): reduce allocations in LatestValuesAccumulator#2661
refactor(backpressure): reduce allocations in LatestValuesAccumulator#2661NearlCrews wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 28 minutes and 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors LatestValuesAccumulator to centralize UNKNOWN_SOURCE, key accumulator entries by (context, path, source) and mutate them in-place, and rewrites buildFlushDeltas to group accumulated values by context and normalized $source before emitting backpressure deltas; adds a test for undefined timestamps. ChangesAccumulation and Flush Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
Thanks for picking this up @NearlCrews, the type tightening and the dead- A couple of things to revise before this is ready:
If you want to fold in one more allocation win while you're here: @tkurki happy to recommend merging once the JSDoc is back and the two |
NearlCrews
force-pushed
the
refactor/backpressure-tidy-accumulator
branch
from
5803f9f to
c75e74a
Compare
|
@msallin all four addressed in the latest commit:
16/16 in |
| expect(deltas[0].$backpressure.duration).to.equal(2000) | ||
| }) | ||
|
|
||
| it('should use the latest timestamp across items with same context+$source', function () { |
There was a problem hiding this comment.
I know this is not a change introduced by this PR, but the test and thus the current behavior is imho incorrect: the timestamp for an update is/should be the timestamp for all values in the update, NOT the timestamp of the freshest value in it.
So afai understand we are sending incorrect timestamps for some pathvalues, when they get grouped by context & $source. Am I reading things right here @dirkwa ?
There was a problem hiding this comment.
@tkurki yes, you're reading it right. Walking through a concrete case to make sure we're talking about the same thing:
Two frames arrive during backpressure from $source: gps:
- T=10.000:
{ timestamp: '10.000', values: [{path: 'navigation.speedOverGround', value: 5.0}] } - T=10.005:
{ timestamp: '10.005', values: [{path: 'navigation.headingTrue', value: 1.5}] }
accumulateLatestValue keys per context:path:$source, so each lands in its own accumulator entry with its original timestamp — correct so far.
buildFlushDeltas then regroups by context + $source only, merging both pathvalues into one Update and keeping the max timestamp. Result:
$source: 'gps',
timestamp: '10.005',
values: [
{ path: 'navigation.speedOverGround', value: 5.0 }, // actually sampled at 10.000
{ path: 'navigation.headingTrue', value: 1.5 } // actually sampled at 10.005
]
A downstream consumer is told speedOverGround=5.0 @ 10.005. Per the delta spec the update's timestamp applies to every pathvalue in it (no per-pathvalue timestamp field), so we are emitting incorrect timestamps for any pathvalue whose original frame timestamp differs from the newest one in the group.
This is pre-existing — the comparison is in the file before this diff; this PR just relocates it and unfortunately adds tests that lock the wrong behavior in.
Two possible fixes for the follow-up:
- Group by
context + $source + timestampso pathvalues that originally shared a timestamp ride in one update and the rest go in their own. Preserves spec semantics exactly. Slight cost: more updates per flush when a source emits paths at slightly different timestamps (common for N2K PGNs demuxed into multiple paths) — probably fine since backpressure flushes are infrequent by definition. - Group by
context + $source, droptimestamp(or set it to flush time). Honest "summarized snapshot" semantics, loses original-sample freshness.
I'd go with (1).
Proposal for this PR, to keep it as the scoped refactor it claims to be: I'll drop the two new tests that assert the max-timestamp behavior (should use the latest timestamp across items with same context+$source and the insertion-order variant) so we don't entrench the bug, keep the undefined-timestamp test (that one's about the dropped as Timestamp cast and is independent), and open a separate issue for the grouping fix. WDYT?
There was a problem hiding this comment.
@tkurki @dirkwa your reading of src/LatestValuesAccumulator.ts:113-119 matches mine: the merged update's timestamp is whichever of the merged items had the freshest one, and the spec says that timestamp applies to every pathvalue in the update. So yes, this emits incorrect timestamps for any merged pathvalue that originally arrived in an older frame. Pre-existing: the merge-and-max-timestamp logic was already there before this PR; this PR only restructures the loop.
Agree with option 1 (context + $source + timestamp grouping). The N2K-demuxed-into-multiple-paths case you mention usually shares a timestamp at source, so it'll still coalesce in practice. The extra-updates cost only shows up when timestamps genuinely differ, which is exactly when keeping them separate is the right thing.
Dropped the two new tests that locked in the max-timestamp behavior (should use the latest timestamp across items with same context+$source and the insertion-order variant) and kept the undefined-timestamp test, which is independent. Pushed as aeeeaaf4. Commit message updated to match.
Filed #2716 for the grouping fix.
NearlCrews
force-pushed
the
refactor/backpressure-tidy-accumulator
branch
from
c75e74a to
aeeeaaf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/LatestValuesAccumulator.js`:
- Around line 330-345: The new JavaScript test in LatestValuesAccumulator.js
must be converted to TypeScript: create a new file
LatestValuesAccumulator.test.ts (or move the block) and port the `it('should
propagate undefined timestamp when items have no timestamp'...)` test to
TypeScript syntax, updating imports/requires to use ES modules or TS imports
consistent with the test suite, ensure types for the accumulator Map entry (or
use `as any`/typed helpers) and keep the call to `buildFlushDeltas(accumulator,
1000)` and the same expectations (`deltas[0].updates[0].timestamp` ===
undefined); remove the .js test file after verifying the TS test runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07ea7a06-c18a-4aae-b29c-999960931058
📒 Files selected for processing (2)
src/LatestValuesAccumulator.tstest/LatestValuesAccumulator.js
| it('should propagate undefined timestamp when items have no timestamp', function () { | ||
| const accumulator = new Map() | ||
| accumulator.set('vessels.self:navigation.speedOverGround:gps', { | ||
| context: 'vessels.self', | ||
| path: 'navigation.speedOverGround', | ||
| value: 5.0, | ||
| $source: 'gps', | ||
| timestamp: undefined | ||
| }) | ||
|
|
||
| const deltas = buildFlushDeltas(accumulator, 1000) | ||
|
|
||
| expect(deltas.length).to.equal(1) | ||
| expect(deltas[0].updates.length).to.equal(1) | ||
| expect(deltas[0].updates[0].timestamp).to.equal(undefined) | ||
| }) |
There was a problem hiding this comment.
Move this new test case to TypeScript.
This PR adds new test logic in a .js file; per repo policy, new code should be added as TypeScript.
As per coding guidelines: **/*.{js,jsx}: “All new code must be written in TypeScript, not JavaScript”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/LatestValuesAccumulator.js` around lines 330 - 345, The new JavaScript
test in LatestValuesAccumulator.js must be converted to TypeScript: create a new
file LatestValuesAccumulator.test.ts (or move the block) and port the
`it('should propagate undefined timestamp when items have no timestamp'...)`
test to TypeScript syntax, updating imports/requires to use ES modules or TS
imports consistent with the test suite, ensure types for the accumulator Map
entry (or use `as any`/typed helpers) and keep the call to
`buildFlushDeltas(accumulator, 1000)` and the same expectations
(`deltas[0].updates[0].timestamp` === undefined); remove the .js test file after
verifying the TS test runs.
aeeeaaf to
7a7739c
Compare
|
Rebased on current master (7a7739c). CI is red on the Ready for review whenever master CI unblocks. |
|
Actionable comments posted: 0 |
|
Triage / direction. The allocation hoisting and the type tightening are good and I'm happy to take them. The blocker is the new timestamp tests: they lock in the Please split into two:
Once (1) is trimmed I'll recommend merge. |
LatestValuesAccumulator runs on the per-delta path while a client is
in backpressure mode. The accumulation loop and the flush group-by
both had small allocation/lookup inefficiencies that are easy to clean
up.
Changes:
- Hoist context, $source, sourceKey, and timestamp out of the
per-pathvalue inner loop in accumulateLatestValue (one read per
update instead of one read per pathvalue).
- accumulateLatestValue mutates the existing AccumulatedItem on
overwrite instead of allocating a fresh one. Common case during
sustained backpressure; saves one object per pathvalue per overwrite.
- Replace the double Map lookup (.has(k) + .get(k)) in
buildFlushDeltas with a single get-or-create pattern.
- Drop a tautological hasValues(update) check that runs immediately
after constructing the update with values: [].
- Tighten the inner Map type to Update & { values: PathValue[] } so
the initial { values: [] } is correctly typed and the per-pathvalue
push no longer needs the as Path / as any casts.
- Replace the value cast from any to Value (from @signalk/server-api).
- Add UNKNOWN_SOURCE = 'unknown' constant for the two places that
fell back when $source is missing.
- Drop a few WHAT-leaning inline comments. JSDoc on the two exported
functions is kept.
New test in test/LatestValuesAccumulator.js:
- buildFlushDeltas propagates undefined timestamp when items have no
timestamp (covers the dropped \`as Timestamp\` cast).
This is the LatestValuesAccumulator portion of the now-redundant
SignalK#2648 (the streambundle.ts portion was superseded by SignalK#2622, which
landed afterward).
Refs SignalK#2582.
|
Rebased onto master to clear the stale On the timestamp tests: the diff currently adds only one, So this already looks trimmed to the allocation hoisting, the get-or-create, and the type tightening. Do you want that one undefined-timestamp test dropped as well, or is it fine to keep since it does not lock in the grouping behavior? Happy to move it to the follow-up correctness PR if you would rather all timestamp tests live there. |
NearlCrews
force-pushed
the
refactor/backpressure-tidy-accumulator
branch
from
7a7739c to
8804ad9
Compare
4 participants
Why
LatestValuesAccumulatorruns on the per-delta path while a client is in backpressure mode. The accumulation loop and the flush group-by both had small allocation and lookup inefficiencies that are worth tidying up.This is the
LatestValuesAccumulator.tsportion of the now-redundant #2648 (thestreambundle.tsportion was superseded by #2622, which landed afterward). Reopened as a focused PR.What's in the diff
accumulateLatestValue:context,$source,sourceKey, andtimestampout of the per-pathvalue inner loop. One read per update instead of one read per pathvalue.buildFlushDeltas:.has(k)+.get(k)+.set(k, ...)) with a single get-or-create pattern.hasValues(update)check that runs immediately after constructing the update withvalues: [].Update & { values: PathValue[] }so the initial{ values: [] }is correctly typed and the per-pathvalue push no longer needsas Path/as anycasts.anytoValue(from@signalk/server-api).Cleanup:
UNKNOWN_SOURCE = 'unknown'module constant for the two places that fall back when$sourceis missing.Notes
test/LatestValuesAccumulator.jsandtest/BackpressureManager.tspass.Refs #2582.
Summary
This PR refactors LatestValuesAccumulator to reduce allocations and lookup inefficiencies on the per-delta path during client backpressure.
Changes in src/LatestValuesAccumulator.ts
@signalk/server-api.No behavioral change to per-delta accumulation or flushed-delta shape is introduced. No version bump.
Changes in test/LatestValuesAccumulator.js
Testing