perf: tail-only f32 re-encode on streaming append - #314
Conversation
Every append tick re-encoded 100% of every shipped geometry column (O(N) f64->f32 pass + N-sized alloc per column per tick) even though the sticky ship offset keeps the encoding prefix-stable and the client only uploads the tail. Cache the whole-column f32 encoding on Column (encoded_f32: offset/scale/ base-pointer keyed, capacity-doubling buffer mirroring Column.append) and serve identity whole-column ships from it, encoding only the appended rows. Invalidation is conservative: offset/scale changes, shrinks, or external rebinding re-encode fully into a fresh buffer so previously shipped split-payload views are never overwritten; append re-keys the cache across its own growth-buffer migrations. Filtered/decimated/masked ships bypass the cache unchanged. - +4 B/point resident RAM per shipped column, itemized in memory_report() as encode_cache_bytes (per column and totaled) - pinned by tests/test_streaming.py::test_append_reencodes_only_the_tail plus byte-identity, shipped-buffer aliasing, and memory-report tests - new CodSpeed benchmark test_stream_scatter_append_direct (direct tier pinned via density=False): 495us -> 205us per 1k-row append at 100k pts; 8 direct traces x 199k pts: 8.96ms -> 0.64ms per tick - spec: design-dossier §27 memory rules corollary, wire-protocol §4 kernel-side tail-only note, benchmarks results streaming_updates row
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughStreaming append payloads now cache whole-column f32 encodings, incrementally encode appended rows, invalidate caches when required, report cache memory, and validate prefix stability through tests and benchmarks. ChangesStreaming encode cache
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Figure.append
participant _PayloadWriter.ship
participant Column
participant kernels.encode_f32
Figure.append->>_PayloadWriter.ship: ship whole-column values
_PayloadWriter.ship->>Column: request cached f32 encoding
Column-->>_PayloadWriter.ship: return cached prefix and encoded tail
_PayloadWriter.ship->>kernels.encode_f32: encode fallback or appended rows
kernels.encode_f32-->>Figure.append: return payload buffers and metadata
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Performance Changes
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/xy/columns.py (1)
332-353: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle external rebinding before re-keying the cache.
If
_growalready exists,valuesis rebound, and a cache is warmed for the rebound array, Line 349 matchesold_ptrand re-keys that cache to the stale_growbuffer. The next payload combines the rebound encoded prefix with stale canonical values. Detect thatvaluesno longer aliases_grow, rebuild growth storage from current values, and invalidate the cache.🤖 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 `@python/xy/columns.py` around lines 332 - 353, Update the growth logic around the cache handling in the column append method to detect when an existing _grow buffer no longer aliases self.values before re-keying _enc_cache. Rebuild _grow from the current values, preserve the required capacity and data, and invalidate the cache when external rebinding is detected; only re-key a cache after confirming the growth buffer remains the active backing storage.
🧹 Nitpick comments (1)
tests/test_streaming.py (1)
439-453: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the replacement payload is correct after recentering.
This only proves
bufs1was not mutated. A fresh buffer containing stale pre-recenter bytes would still pass. Retain the finalmsg, buffersand compare each final column buffer withkernels.encode_f32(col.values, offset, scale).🤖 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 `@tests/test_streaming.py` around lines 439 - 453, The test test_shipped_buffers_survive_later_appends_and_recenter must also validate the replacement payload after recentering, not only preservation of bufs1. Retain the final msg and buffers from the last append, obtain each column’s values and encoding metadata from the final payload, and assert every final buffer equals kernels.encode_f32(col.values, offset, scale).
🤖 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 `@python/xy/columns.py`:
- Around line 299-305: Update the cache-building logic around the full and tail
encode paths in python/xy/columns.py lines 299-305 to use the established
finite-value plus validity representation instead of storing raw potentially
non-finite f32 output; apply this consistently to both buf[:n] and buf[n_old:n]
updates. In python/xy/_payload.py lines 92-100, use the cache only when it
provides equivalent validity semantics to the normal geometry encoder; otherwise
preserve the existing safe encoding path.
In `@spec/design-dossier.md`:
- Around line 997-1003: The capacity-cost documentation needs to distinguish
allocated f32 cache capacity from current point count and tail work. In
spec/design-dossier.md:997-1003, update the whole-column cache description to
state 4 B per allocated f32 capacity slot and identify encode_cache_bytes as the
actual resident allocation, including minimum allocation and doubling behavior.
In spec/design/wire-protocol.md:271-278, state that tail encoding is O(appended
rows) while buffer maintenance, including resize copies, is amortized.
---
Outside diff comments:
In `@python/xy/columns.py`:
- Around line 332-353: Update the growth logic around the cache handling in the
column append method to detect when an existing _grow buffer no longer aliases
self.values before re-keying _enc_cache. Rebuild _grow from the current values,
preserve the required capacity and data, and invalidate the cache when external
rebinding is detected; only re-key a cache after confirming the growth buffer
remains the active backing storage.
---
Nitpick comments:
In `@tests/test_streaming.py`:
- Around line 439-453: The test
test_shipped_buffers_survive_later_appends_and_recenter must also validate the
replacement payload after recentering, not only preservation of bufs1. Retain
the final msg and buffers from the last append, obtain each column’s values and
encoding metadata from the final payload, and assert every final buffer equals
kernels.encode_f32(col.values, offset, scale).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ce503fd-3ecc-4a8a-b1fe-72acd7dcb24d
📒 Files selected for processing (7)
benchmarks/test_codspeed_kernels.pypython/xy/_payload.pypython/xy/columns.pyspec/benchmarks/results.mdspec/design-dossier.mdspec/design/wire-protocol.mdtests/test_streaming.py
| if n > n_old: | ||
| buf[n_old:n] = kernels.encode_f32(self.values[n_old:], offset, scale) | ||
| self._enc_cache = (offset, scale, n, ptr, buf) | ||
| return buf[:n] | ||
| buf = np.empty(max(n, 1024), dtype=np.float32) | ||
| buf[:n] = kernels.encode_f32(self.values, offset, scale) | ||
| self._enc_cache = (offset, scale, n, ptr, buf) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep cached geometry finite before it reaches the wire. The cache stores raw f32 output and the payload path ships it directly, allowing NaNs from canonical columns into vertex buffers.
python/xy/columns.py#L299-L305: encode invalid values using the established finite-value plus validity representation for full and tail cache updates.python/xy/_payload.py#L92-L100: use the cache only after it has equivalent validity semantics to the normal geometry encoder; otherwise retain the safe path.
📍 Affects 2 files
python/xy/columns.py#L299-L305(this comment)python/xy/_payload.py#L92-L100
🤖 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 `@python/xy/columns.py` around lines 299 - 305, Update the cache-building logic
around the full and tail encode paths in python/xy/columns.py lines 299-305 to
use the established finite-value plus validity representation instead of storing
raw potentially non-finite f32 output; apply this consistently to both buf[:n]
and buf[n_old:n] updates. In python/xy/_payload.py lines 92-100, use the cache
only when it provides equivalent validity semantics to the normal geometry
encoder; otherwise preserve the existing safe encoding path.
Source: Coding guidelines
| Second corollary: every whole-column geometry ship keeps the encoded f32 result as | ||
| a per-column cache (`Column.encoded_f32`) so a streaming append re-encodes only the | ||
| appended tail (§5) — +4 B/point of resident RAM per shipped column (even when the | ||
| canonical column is memmapped), itemized per column and totaled as | ||
| `encode_cache_bytes`. It is a rebuildable derived cache under rule 1: any | ||
| offset/scale change or values rebinding drops it for a full re-encode into a fresh | ||
| buffer, never overwriting bytes a previously shipped split payload still borrows. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document capacity-doubling costs accurately. The f32 cache has a minimum allocation and doubles on growth, so memory and CPU costs are amortized rather than fixed per current point/per tick.
spec/design-dossier.md#L997-L1003: describe 4 B per allocated f32 capacity slot and refer toencode_cache_bytesfor actual resident allocation.spec/design/wire-protocol.md#L271-L278: state that tail encoding is O(appended rows), while buffer maintenance is amortized due to resize copies.
📍 Affects 2 files
spec/design-dossier.md#L997-L1003(this comment)spec/design/wire-protocol.md#L271-L278
🤖 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 `@spec/design-dossier.md` around lines 997 - 1003, The capacity-cost
documentation needs to distinguish allocated f32 cache capacity from current
point count and tail work. In spec/design-dossier.md:997-1003, update the
whole-column cache description to state 4 B per allocated f32 capacity slot and
identify encode_cache_bytes as the actual resident allocation, including minimum
allocation and doubling behavior. In spec/design/wire-protocol.md:271-278, state
that tail encoding is O(appended rows) while buffer maintenance, including
resize copies, is amortized.
Source: Coding guidelines
CodSpeed flagged 12-33% regressions on first-build benchmarks: the cold encode path allocated a slack cache buffer and memcpy'd the encode output into it — an extra N-f32 alloc + copy per geometry column on every fresh build. Adopt the exact-size encode output itself as the cache buffer instead, making the cold path identical in cost to the pre-cache code; the doubling slack starts on the first append (which always grows once).
Alek99
left a comment
Alek99
left a comment
There was a problem hiding this comment.
I found three issues in the current head (1c5d68a):
-
High — rebinding can corrupt cached geometry (
python/xy/columns.py:353-355). When_growexists andvalueswas externally rebound, a cache warmed on the rebound array satisfiescache[3] == old_ptr.append()then restores the stale_growprefix and re-keys the rebound encoding to that pointer. I reproduced a cached payload whose prefix differed from a fresh encode ofcol.values. Please confirmvaluesstill aliases_grow; otherwise rebuild growth storage from the current values and invalidate the cache. -
Medium —
resident_array_bytesomits the encode cache (python/xy/_figure.py:1876-1881).encode_cache_bytesis itemized but is not included in the combined resident total. For a 256-point scatter, the report returned 4096 canonical-capacity bytes, 2048 encode-cache bytes, and only 4096 resident-array bytes. -
Medium — the memory/complexity specification counts live points instead of allocated capacity (
spec/design-dossier.md:997-1003,spec/design/wire-protocol.md:272-278). The f32 buffer doubles capacity, so 257 live entries can retain 512 slots—nearly 8 B/live point, not +4 B/point. The per-tick O(appended rows) claim should also be described as amortized because a growth event copies the prefix.
Validation: tests/test_streaming.py passed (28 tests), and Ruff check/format passed. The broader figure/scatter run had 238 passes and one unrelated local Chromium screenshot failure.
|
This seems like a non-trivial amount of extra memory usage to maintain the cache. At 4-8 B / point, we're talking hundreds of megabytes in large plots. |
3 participants
What
Streaming appends re-encoded 100% of every shipped geometry column per tick — an O(N) f64→f32 native pass plus an N-sized allocation per column — even though the sticky ship offset (
Column.suggest_offset) keeps the encoding prefix-stable and the browser client already uploads only the tail (bufferSubData). This is item 1 of the 2026-07 Abseil perf audit.This PR adds the kernel-side twin of the client's tail upload: a per-column whole-column f32 encode cache (
Column.encoded_f32) served by an identity fast path in_PayloadWriter.ship, so each tick encodes only the appended rows.How
(offset, scale, values base pointer), held in a capacity-doubling f32 buffer mirroringColumn.append's growth buffer.Column.appendre-keys the cache across its own growth-buffer migrations (prefix preserved byte-for-byte) and drops it on external rebinding.values is not col.values) bypass the cache unchanged.memory_report()per column and totaled asencode_cache_bytes(§27: if it isn't in the report it isn't real).Numbers
test_stream_scatter_append_direct(direct tier pinned viadensity=False): 495µs → 205µs per 1k-row append at 100k points.test_stream_line_appendis expected to move less: decimated lines ship M4 views that bypass the cache by design.Tests
test_append_reencodes_only_the_tail— spies onkernels.encode_f32; fails on main (4097-row full re-encode per 1-row append), passes here.test_append_cached_encode_matches_full_reencode— cached bytes are identical to a from-scratch encode.test_shipped_buffers_survive_later_appends_and_recenter— shipped zero-copy views stay byte-stable across tail extensions and cache invalidation.test_memory_report_itemizes_encode_cache_bytes.test_append_keeps_offsets_sticky_and_prefixes_byte_identicaletc.) passes unchanged;scripts/append_stream_smoke.pypasses end-to-end.Spec
spec/design-dossier.md§27 memory rules: encode-cache corollary.spec/design/wire-protocol.md§4: kernel-side tail-only re-encode note.spec/benchmarks/results.md:streaming_updatesrow now inventories the CodSpeed streaming benchmarks.Summary by CodeRabbit
Performance
Monitoring
Documentation
Tests