Cache the single-node ellipse arc per track - #138
Conversation
|
Claude finished @Babissimo's task in 3m 24s —— View job Code ReviewTasks
CI is green (991 passed). I was not able to execute the test suite myself in this sandbox (bash/pytest execution requires approval that isn't grantable here) — review below is static, informed by CI's passing result. SummaryThis is a well-scoped, well-documented change.
Observations (non-blocking)
Tests
Nice work — the design doc anticipated essentially every correctness question I'd otherwise raise (hex-collision safety, re-key duplication, cache growth bound). |
2ddf5da to
c30e66a
Compare
|
Claude finished @Babissimo's task in 3m 27s —— View job Code ReviewTasks
SummaryThe arc-caching core ( This sync added one new commit, Finding: flush-cadence change contradicts this PR's own design doc
That would be fine on its own merits, except this same PR ships a design doc and implementation plan that explicitly documented the opposite decision:
An earlier commit on this branch ( Two independent things worth reconciling before merge:
Neither point blocks on correctness grounds — the arc-cache logic itself is unaffected and still correct at any cadence per its design. This is a documentation/scope-consistency issue, not a bug. |
The flush build calls _build_single_node_arc for every aircraft every cycle, uncached, and that pure-Python scalar-trig recompute is what overruns the ~1s flush and pulses the map ellipses. Add a per-(hex,node) memo fingerprinted on the arc's non-static inputs (delay, lat, lon) so it rebuilds only when a new detection changes them. This commit adds the mechanism and its unit tests; wiring into the build follows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Route the per-track arc through _cached_single_node_arc and prune the cache each build to the (hex, node) keys actually seen, so an unchanged fleet does zero arc rebuilds and cache size stays bounded to the live fleet without a timer. The arc is still emitted every flush, so the frontend afterglow is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c30e66a to
11322d3
Compare
|
Claude finished @Babissimo's task in 2m 52s —— View job Code ReviewTasks
SummaryRe-reviewed at HEAD (
Correction to my prior review pass on this PRMy previous pass on this branch cited specific line numbers in Still-valid, non-blocking observationIndependent of the doc question above,
Neither point is a correctness issue — the arc-cache logic itself is correct at any flush cadence. |
The aircraft.json write + WebSocket broadcast loop runs at ~1 Hz. Replace its hardcoded sleep(1.0) with the AIRCRAFT_FLUSH_INTERVAL_S constant (which was dead, kept alive only by the vulture whitelist), correct the docstrings that still claimed 2 Hz, and drop the now-redundant whitelist entry (the constant is referenced now, not dead). Real RETINA nodes report detections at ~2 Hz, but the server flush stays a deliberate 1 Hz; the arc cache earlier in this branch removes the redundant per-flush arc recompute independently of that rate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
11322d3 to
6fb4ab3
Compare
|
Claude finished @Babissimo's task in 1m 39s —— View job Code ReviewTasks
I was not able to execute SummaryRe-reviewed at HEAD (
Previous finding: resolvedMy prior review pass flagged commit The latest commit, Remaining observations (non-blocking, already disclosed in the PR body)
Both match the PR's own "Deferred (non-blocking, tracked)" section — nothing new here. TestsCoverage is thorough and matches the cache's correctness matrix (hit/miss/jitter/None-delay/purity/eviction/behavioral). No gaps to flag. Nice work resolving the cadence issue — the arc-cache feature and the flush-loop cleanup are now cleanly separated in effect (cadence unchanged, only the dead-constant/docstring drift is fixed). |
jonnyspicer
left a comment
There was a problem hiding this comment.
lgtm, this method of testing is a bit funny, at some point we'll want some sort of observability on the frontend to detect slowdowns but not urgent
Summary
Adds a backend compute-cache that memoises
_build_single_node_arcper(hex, node_id)track, fingerprinted on(delay_us, lat, lon)(None-safe, tight epsilon) and evicted each flush to the set of currently-seen keys. This removes the redundant per-flush arc recompute that produced the ~4s map-ellipse pulse for single-node tracks.ClickUp: 86caxu6vc (subtask of umbrella 86cau8uu2)
Changes
b98b494— cache plus_cached_single_node_archelper, with unit tests6552887— wire the memo intobuild_combined_aircraft_json, evict per flush, integration tests6fb4ab3— drive the flush-loop cadence fromAIRCRAFT_FLUSH_INTERVAL_S(previously dead) instead of a hardcodedsleep(1.0), correct the docstrings to ~1 Hz, drop the now-redundant vulture whitelist entryThe flush/broadcast loop stays at 1 Hz. Real RETINA nodes report detections at ~2 Hz, but the server broadcast is deliberately throttled to 1 Hz; the cache removes the redundant per-flush recompute independently of either rate.
Benchmark
Single-threaded build-compute only (not full prod wall-clock), median
build_combined_aircraft_jsontime over a fixed 200-node synthetic fleet. The axis is the fleet--interval(seconds between fresh detections per track), which maps directly to cache hit rate:At the real ~2 Hz node cadence the detection changes every flush, so the cache is invalidated almost every flush and helps only idle/aging tracks (the 1 s row is a close proxy). The clear win is the 40 s test fleet (testmap), which is what this branch is for.
Testing
Full backend suite green: 991 passed, 81% coverage (the 1 Hz reconciliation changes only the interval constant's value and two docstrings, which no test references).
Deferred (non-blocking, tracked)
Follow-ups filed: 86cawjevd (realistic-cadence validation plus the GIL/compute fix), 86cawmutv (frontend coalesce), 86caxc0yg (radar-only to ADS-B re-key duplication / hex_id collisions).