Release the legend-visible row cache, and put it in the report - #324
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
638f61d to
4977a69
Compare
`_legend_visible_rows` memoizes the canonical rows outside the hidden-category set, keyed on (hidden set, column length). The cache is worth having — density_view runs per pan and zoom step, and the O(N) code scan must not repeat while the predicate holds — but it was never dropped, never invalidated and never reported. One legend click on a 4M-row three-category trace left 20.4 MB resident for the figure's lifetime; at 50M rows with one of eight categories hidden it is ~350 MB, and `memory_report()` claimed none of it. Two changes, both narrow: - `_legend_visible_rows` clears the cache on its `pred is None` early return. That branch is exactly the state in which nothing can consume the array — no category is hidden — so dropping it is provably free, and it is the state a user reaches by un-hiding the last hidden series. - `memory_report()` gains `legend_vis_cache_bytes` and folds it into `resident_array_bytes`, alongside `pyramid_bytes` and `bin_color_bytes`. Design dossier §27: if a memory number isn't in the report, it isn't real. Deliberately not done here: invalidating on append. The cache only exists for a trace with categorical color codes, and `append_data` rejects categorical channels outright, so that line would be unreachable today — and its key check already refuses a stale entry on length change. Nor is the dtype narrowed to uint32: NumPy fancy-indexing upcasts non-intp index arrays internally, which would trade this retention for a per-use temporary of the same size. Three new tests cover the release, the report line, and the resident total.
Dropping it in `_legend_visible_rows` only helps if something calls that again, and it is the cache's only reader. Un-hiding the last category left the index resident until some later `density_view` happened to run — and a legend click restoring every series is a perfectly ordinary last interaction, after which nothing ran. A 200k-row trace held 1,065,040 bytes indefinitely, reported but never released. Clear it in `legend_toggle` when the hidden set becomes empty. The reader still clears it too, for the paths that reach unmasked without a toggle (a color encoding replaced under a stale mask). The test covering this asked for another density view before checking, which is exactly the thing that used to do the releasing, so it passed either way; it now asserts the release before any further view, alongside a partial un-hide that must re-key rather than drop, and the trace-level `hidden` switch that must not touch this cache at all.
a79cc54 to
38188c0
Compare
1 participant
A retained allocation that §27 did not know about. From the second memory audit
pass (follow-up to #309); sibling PRs carry the encode paths (#320), the Python
paths (#321), the Rust core (#322), and the selection kernel (#323).
The leak
_legend_visible_rowsmemoizes the canonical rows outside the hidden-categoryset, keyed on
(hidden set, column length). The cache is worth having —density_viewruns per pan and zoom step, and the O(N) code scan must not repeatwhile the predicate is unchanged.
But the array was never dropped, never invalidated, and never reported. One
legend click leaves one
np.intpper visible row on the trace for the figure'slifetime:
And
memory_report()claimed none of it, which is the part that makes it a specproblem rather than just a size problem.
The fix, in two narrow pieces
Release it when nothing is hidden.
_legend_visible_rowsclears the cache onits
pred is Noneearly return. That branch is exactly the state in whichnothing can consume the array — no category is hidden — so dropping it is
provably free, and it is the state a user reaches by un-hiding the last hidden
series. After un-hiding, the 20.4 MB above becomes 0.
Report it.
memory_report()gainslegend_vis_cache_bytesand folds it intoresident_array_bytes, alongsidepyramid_bytesandbin_color_bytes. Designdossier §27: if a memory number isn't in the report, it isn't real.
What this deliberately does not do
categorical color codes (
_hidden_predicatereturns None withoutcolor_ch.codes), andappend_datarejects categorical channels outright, sosuch a line would be unreachable today. Its key check already refuses a stale
entry on length change.
fancy-indexing upcasts non-
intpindex arrays internally, so it would tradethis retention for a per-use temporary of the same size — worse on both CPU and
peak.
Verification
Three new tests: the release when nothing is hidden (with the report line
tracking it while held), the report line itself, and its inclusion in
resident_array_bytes.2414 tests pass. A 97-artifact output fingerprint sweep is identical to main.
Perf
The dropped branch does no work that any consumer needed, and the re-scan it
could cause only happens if a user re-hides a category after un-hiding all of
them — which pays one O(N) code scan, the same one the first hide paid.
Interleaved A/B against main: neutral, no region more than 5% worse.