Polar audit round: GPU buffer leak, mobile chrome, wedge vertex cost, CodSpeed coverage - #380
Conversation
Nine reported failures across the polar surface and the responsive chrome it shares with every other chart. **P1 — repeated data updates leaked GPU buffers.** Trace teardown walked a hand-kept list of geometry buffer names, so every rebuilt trace (a state-driven update, an append that could not patch in place, an animated spec swap) orphaned its style, direct-RGBA colour, stroke, corner-radius, LOD-blend and dashed-line-length buffers. All three teardown paths now read one shared TRACE_GPU_BUFFERS list, pinned against the build paths by a test so a new channel cannot reintroduce the leak. **P1/P2 — mobile Wind Rose chrome.** The browser wraps a long title while layout measured one line, so a compact title lost ~10 px off the canvas top. Titles now wrap at one shared width in all three renderers and the browser caps the element at that same width; single-line titles are byte-identical. A polar legend now reserves a gutter beside the disc — 96 px on the loc's side, or a 64 px band beneath at compact widths — instead of covering the north-east sectors and the outer radial label. An authored anchor or four-tuple padding still wins. **P2 — wedge vertex cost.** Subdivision is span-proportional: clamp(ceil(96 * |span| / turn), 2, 96) over the *authored* angular width, in every renderer. Sagitta is quadratic in the per-segment angle, so this holds the flattening bound while a 16-sector wind-rose bar costs 14 vertices instead of 194; a full turn still uses 96. **P2 — inert polar axis options.** theta_axis/r_axis refuse minor_tick_values, minor_style, tick_label_min_gap, tick_label_anchor and the collision spellings of tick_label_strategy, each naming the control that does work. pyplot's projection="polar" drops the same keywords instead, because every matplotlib Axes carries an rcParam minor style it never authored; recorded in the compat spec. theta_axis(format=) now wins over the built-in degree text, and an r_axis(margin=) is honoured instead of discarded. **P2 — radial time axes.** A time radius autoranges from its data instead of epoch zero, which had squeezed every modern instant into a hairline ring at the rim. The signed-radius contract (a position, never a mirrored direction) is now normative in the spec and in the r_axis docstring. **P2 — compact colorbars.** They keep their two extreme tick labels and the scale title; only the interior ladder and the text-free minor ticks drop. Hiding everything left an unlabelled gradient. **P2 — redundant frames.** Data animations use the view animation's 80 ms label cadence instead of rebuilding the whole tick-label DOM every frame, and force one settled rebuild at the end. A DPR change coalesces into a single resize frame and rescales the per-instance widths and corner radii that are baked in device pixels. **P2 — pie_chart.** Listed in the generated chart-factory API reference with the other polar compositions. A zero-width bar is now legal and draws nothing, like line_width=0, so a 0% progress ring or an empty aggregated category no longer dies with "bar width must be positive".
|
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: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Merging this PR will not alter performance
Comparing Footnotes
|
`alek/polar-axes` went red at 7449360 (the squash of #380) after being green at a3e28bf, and all five failures are mine. **Two were tests pinning code I moved.** `test_tick_sides_bump_wire_protocol_and_client_in_lockstep` asserted the exact `00_header` import line, which grew a third name; it now checks that the client imports PROTOCOL rather than the spelling of the list. `test_triangle_mesh_resource_cleanup_deletes_every_coordinate_buffer` looked for the buffer names inside `_destroyTraceResources`, where they no longer live; it reads the shared `TRACE_GPU_BUFFERS` list and asserts the teardown uses it. (That second fix was already written, just not in the squash.) **Two were the polar legend gutter ellipsizing static labels.** At a flat 96 px the exporters' legend had 44 px of label room, so `series one` came out `series...` and `Organic 20 (33%)` came out `Orga...`. The gutter is now 22% of the canvas clamped to 120-200 px, which gives 68 px at the narrowest non-compact width and 146 px at the default export size — both failing labels fit. **One was the compact colorbar spending plot width.** Reserving a side gutter for the endpoint tick labels cost 36 px, and `test_narrow_fluid_resize_stays_painted_and_preserves_plot_space` guards exactly that (plot width >= 280, colorbar width 18). The endpoints now stack above and below the gradient instead of sitting beside it: centred on an 18 px bar they overflow ~4 px a side into the gap already reserved, so the reservation goes back to what it was and the fix is free. The rotated title joins the interior ladder in dropping — at phone width it has nowhere to go, and `box.title` already names the scale and its range. That test's "hide every tick and the title" assertion was the defect the audit reported, so it now states the new contract: the two extremes stay, everything else goes. Also reverts the DPR-change deferral. `render_smoke_nonumpy.py`'s `dprw` probe calls `_onDprChange()` and reads `dpr`/`canvas.width`/`chrome.width` on the very next line — a DPR change with no container resize has no later event to piggyback on — and routing it through `_queueResize` broke that contract. It also saved nothing: the ResizeObserver's queued pass already early-returns when width, height and dpr are all unchanged, and when the CSS size did change too, the second pass is doing real work at a new size. The dpr-baked width/radius rescale stays.
Stacked onto #370 (
alek/polar-axes) — merge here and the fixes land on that PR.Nine reported failures across the polar surface and the responsive chrome it shares with every other chart, plus the CodSpeed gap that let one of them ship.
P1 — repeated data updates leaked GPU buffers
_destroyTraceResourceswalked a hand-kept list of geometry buffer names, so every rebuilt trace — a state-driven data update, an append that could not patch in place, an animated spec swap — orphaned its style, direct-RGBA colour, stroke, corner-radius, LOD-blend and dashed-line-length buffers.All three teardown paths (trace, drill window, sample overlay) now read one shared
TRACE_GPU_BUFFERSlist in the leaf module.tests/test_trace_buffer_lifecycle.pypins that every<name>Buffield assigned anywhere injs/srcappears in it, so adding a channel buffer cannot silently reintroduce the leak.P1/P2 — mobile Wind Rose chrome
white-space: pre-line) while layout measured one line, so the first line went above the canvas. Titles now wrap at one shared width — the canvas minus the authored horizontal gutters, resolved before the title band because the measured left gutter depends on the plot height — in all three renderers, and the browser caps the title element at that same width so the DOM cannot wrap into more lines than were reserved. Single-line titles are byte-identical.upper rightlegend covered the north-east sectors and part of the outer radial label. A polar figure with a legend now reserves a gutter off the canvas edge before the disc is fitted —_POLAR_LEGEND_ROOM(96 px) on the sidelocnames, or_POLAR_LEGEND_BAND(64 px) beneath the disc at compact widths — and the legend places and bounds itself in that box. An authoredanchor(still resolved against the plot) or four-tuplepaddingreserves nothing.P2 — 50k polar bars fell off a cliff at 194 vertices per wedge
Subdivision is now span-proportional and recorded as a formula:
Sagitta is quadratic in the per-segment angle, so holding
span / nfixed holds the flattening bound: a 16-sector wind-rose sector needs six segments (14 vertices) rather than the full-turn worst case of 96 (194). A full turn still uses 96, so nothing gets coarser.Not the view-adaptive count §28 would need a recording for — the input is the authored angular width (the scalar
widthon the compact bar path, the widestx1 − x0in the trace on the four-edge path, measured once at build and cached), so zoom, resize and export cannot change it.P2 — polar axis options that shipped and were ignored
xy.theta_axis/xy.r_axisnow refuseminor_tick_values,minor_style,tick_label_min_gap,tick_label_anchorand the collision spellings oftick_label_strategy(auto/hide/rotate/stagger/preserve), each naming the control that does work.offandnonestay honoured.Refused on the documented surface rather than at payload build, deliberately:
xy.pyplot's polar projection assembles its axis from a property bag it does not own (every Axes carries an rcParamminor_style;minorticks_on()andtick_params(ha=)add more), so refusing there would turnprojection="polar"into an error over defaults nobody authored. The adapter drops them instead — what all three renderers already do with the values — and that drop is recorded inspec/matplotlib/compat.md.Two are now honoured instead:
theta_axis(format=".0f°")wins over the built-in degree/radian text (the angular branch used to run first and overwrite it), andr_axis(margin=)keeps the outer pad the centre-origin default otherwise drops.P2 — radial time axes
A time radius was pinned to the centre-origin default, i.e. epoch zero, so twelve consecutive days out of ~1.7e12 ms occupied 0.0006 % of the radius and read as a solid disc edge. A time radius now keeps its ordinary padded extent. Signed-radius behaviour (a position on a range that includes it, never a mirrored direction) is normative in the spec and stated in the
r_axisdocstring.P2 — compact colorbars were unlabelled
_positionColorbarhid every tick and the title, leaving a bare gradient. It now keeps the two extreme tick labels and the scale title, and reserves the room they need; only the interior ladder and the text-free minor ticks drop.P2 — redundant frames
P2 —
xy.pie_chartListed in the generated chart-factory inventory with the other polar compositions (it was the one missing). A zero-width bar is now legal and draws nothing, like
line_width=0, so the hand-rolled pie/gauge recipes no longer die at exactly 0 % with "bar width must be positive" — a message about the author's code from a value that came out of their data. Negative and non-finite widths are still refused.CodSpeed coverage for the polar surface
CodSpeed reported "103 untouched benchmarks" on #370 — for a change that rewrote wedge geometry in three renderers. That blind spot is why the 50k-bar cliff above was found by hand.
benchmarks/test_codspeed_polar.pyadds six rows: payload prep for a polar line / wind rose / pie (the three materially different validation and emit paths), SVG vs native-PNG export of the same rose (bracketing the arc-flattening term, so a regression back to a flat subdivision count is a step change rather than a bug report), and a polar heatmap's bounded inverse raster.The report's other item — 2 benchmarks skipped, baseline reused — is orphan rows in the dashboard, not skipped tests: the suite collects exactly 103 rows and 103 were measured, so those two have no code behind them and need archiving there by hand.
test_codspeed_row_count_matches_the_methodology_specnow gates the collected total againstspec/benchmarks/methodology.md§8 (which was itself one row stale), so a renamed or deleted benchmark fails CI instead of becoming a permanent "skipped" row.Not in this PR
docs/charts/pie-chart.mdanddocs/charts/radial-bar-chart.mdlive on #371's branch, not on this base, so the two doc-page items from the audit — addingxy.pie_chart()to the Pie prose, and the contrast failures in the Pie/Radial Bar examples (gray text at 2.63:1, adjacent purples at 1.33–1.43:1) — belong there. The underlying 0 %-recipe failure those pages hit is fixed here.Verification
Ruff check and format are clean across the worktree, and pre-commit runs nothing else outside
docs/(only codespell, and the sole docs change is a Python list).The sandbox has no network egress to PyPI, npm or crates.io, so
pytest,cargo testandnode js/build.mjscould not be executed here — the native core and the client bundle cannot be built. What was verified directly:js/src(33 literals, 0 failing);config.polar_bar_segmentsexercised standalone: 96 at a full turn, 6 at 22.5°, floor 2, and a constant per-segment angle across 360/180/90/45/22.5°;_textblock.wrap_lines/measure(max_width=)exercised standalone: the reported title wraps to two lines at a 380 px canvas (block height 33.6 px inside a 41.6 px reserved band), single-line titles keep the historical 30 px band, hard newlines are preserved, and an over-long word overflows on its own line;off/nonespellings that must survive;_NUMBER_SPEC(.0f°→90°);polar_wedge_pointscaller in the suite passes an explicitsteps=, so the new per-wedge default cannot move their assertions.CI is the first place the full suite runs.