Cut 9 ms of cold start from static export at 10M points - #284
Merged
Conversation
The docs "Snapshot at 10 million points" (0.0232 s) is ~71% first-call overhead, not data work: a cold static PNG at 10M measures 27.4 ms here, while repeating the same two calls in-process costs 7.8 ms. Two non-engine costs dominate, and neither is visible to CodSpeed — its largest scatter is 1M, and warm_lazy_modules deliberately warms lazy imports before every measured region. Vendor xml.sax.saxutils.escape into _svg.py. Importing it pulls urllib.request -> http.client -> ssl, socket and all of email: 35+ modules for a function whose body is three str.replace calls. Nothing else in xy needs that tree. Skip the density point-sample overlay on raster export. density["sample"] has one writer and zero Python readers -- _raster._emit_grid draws density traces from the grid alone -- so at 10M it ran a SplitMix predicate over all 10M rows and gathered two 8,222-row columns that no pixel consumed. The public build_payload keeps shipping it; the browser client draws it. Measured, interleaved cold A/B at 10M over 6 rounds: to_png 14.8 -> 5.7 ms, cold total 28.4 -> 19.2 ms (-32%). Output is unchanged: 72 artifacts (PNG at scale 1 and 2, SVG, HTML, wire blob, wire spec) across 12 figure configurations -- density, direct, f32, clipped, continuous colour, NaN-injected, log axes, categorical at 2.2M, line -- all hash-identical to main. Full suite 2283 passed; ty reports the same 13 pre-existing diagnostics.
📝 WalkthroughWalkthroughRaster exports now omit sampled density point overlays while wire payloads retain them. SVG export uses a local XML escaping implementation, with documentation and regression tests covering payload bytes, rendering determinism, escaping compatibility, and static-export imports. ChangesRaster density overlay handling
Static SVG escaping
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
tests/test_svg_escape.py (1)
64-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the cold-import assertion precise.
NumPy is imported before
xy, so the test can falsely attribute NumPy’s imports toxy. It also omitsxml.sax.saxutils, allowing the removed import to regress without failing this guard. Snapshotsys.modulesafter NumPy import, compare the post-export delta, and includexml.sax.saxutilsin the checked set.Suggested test adjustment
- "import sys; import numpy as np; import xy\n" + "import sys; import numpy as np\n" + "before_xy = set(sys.modules)\n" + "import xy\n" ... - "leaked = {'urllib.request', 'ssl', 'http.client', 'email', 'socket'} & set(sys.modules)\n" + "leaked = {'xml.sax.saxutils', 'urllib.request', 'ssl', 'http.client', 'email', 'socket'} & (set(sys.modules) - before_xy)\n"🤖 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_svg_escape.py` around lines 64 - 82, Update test_svg_export_does_not_import_urllib_or_ssl so the subprocess snapshots sys.modules immediately after importing NumPy, then computes only the modules newly loaded by importing xy and exporting SVG. Add xml.sax.saxutils to the forbidden module set alongside the existing network-stack modules, and assert against the post-snapshot delta.
🤖 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/_payload.py`:
- Around line 1131-1134: Record a stable omission reason in the density
specification when the point overlay raster path has no sample, updating the
branch around _density_sample_spec and preserving any existing rows_exceed_u32
reason. Add or update the raster density overlay test to assert the new reason
field while keeping normal sample emission unchanged.
---
Nitpick comments:
In `@tests/test_svg_escape.py`:
- Around line 64-82: Update test_svg_export_does_not_import_urllib_or_ssl so the
subprocess snapshots sys.modules immediately after importing NumPy, then
computes only the modules newly loaded by importing xy and exporting SVG. Add
xml.sax.saxutils to the forbidden module set alongside the existing
network-stack modules, and assert against the post-snapshot delta.
🪄 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: 691add10-2578-4136-bc19-02488182fdfd
📒 Files selected for processing (6)
python/xy/_payload.pypython/xy/_svg.pyspec/api/export.mdspec/design-dossier.mdtests/test_raster_density_overlay.pytests/test_svg_escape.py
Comment on lines
+1131
to
+1134
| if pw.point_overlay: | ||
| sample = self._density_sample_spec(t, sel, visible, xr, yr, pw, sample_sel=sample_sel) | ||
| if sample is not None: | ||
| density["sample"] = sample |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Record raster overlay omission in the emitted density spec.
This branch drops density["sample"] without adding an omission reason. Unlike the existing rows_exceed_u32 path, consumers and diagnostics cannot distinguish the intentional raster-export omission from an absent or broken sample. Add a stable spec field and assert it in tests/test_raster_density_overlay.py while preserving any existing oversized-row reason.
As per coding guidelines, every decimation or tier decision must be recorded in the specification rather than made silently.
🤖 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/_payload.py` around lines 1131 - 1134, Record a stable omission
reason in the density specification when the point overlay raster path has no
sample, updating the branch around _density_sample_spec and preserving any
existing rows_exceed_u32 reason. Add or update the raster density overlay test
to assert the new reason field while keeping normal sample emission unchanged.
Source: Coding guidelines
Merging this PR will not alter performance
Comparing 
Footnotes
|
Alek99
added a commit
that referenced
this pull request
Jul 25, 2026
Review on #284: dropping the point-sample overlay on the raster path left no trace in the spec, which contradicts the §28 rule that no decimation decision is silent. The oversized branch already records overlay_omitted = "rows_exceed_u32"; this adds "static_raster" for the new case, guarded so it cannot mask the more fundamental u32 reason when both apply. Tests pin the field on the raster payload and its absence on the wire payload, so the raster-only flag can never leak to the client. Also tighten the import guard: it now measures modules imported after NumPy, so NumPy's own tree is never attributed to xy, and it checks for xml.sax.saxutils directly instead of only the urllib/ssl/email fallout that import happens to drag in today. A future slimmer saxutils would otherwise let the removed import creep back without failing the test named for it. overlay_omitted is written only into the private raster spec, which no renderer reads: all 72 artifact hashes across 12 figure configurations are unchanged. Full suite 2283 passed.
masenf
pushed a commit
that referenced
this pull request
Jul 27, 2026
Review on #284: dropping the point-sample overlay on the raster path left no trace in the spec, which contradicts the §28 rule that no decimation decision is silent. The oversized branch already records overlay_omitted = "rows_exceed_u32"; this adds "static_raster" for the new case, guarded so it cannot mask the more fundamental u32 reason when both apply. Tests pin the field on the raster payload and its absence on the wire payload, so the raster-only flag can never leak to the client. Also tighten the import guard: it now measures modules imported after NumPy, so NumPy's own tree is never attributed to xy, and it checks for xml.sax.saxutils directly instead of only the urllib/ssl/email fallout that import happens to drag in today. A future slimmer saxutils would otherwise let the removed import creep back without failing the test named for it. overlay_omitted is written only into the private raster spec, which no renderer reads: all 72 artifact hashes across 12 figure configurations are unchanged. Full suite 2283 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The docs headline Snapshot at 10 million points publishes 0.0232 s for the static CPU PNG. Profiling that exact measured region shows ~71% of it is first-call overhead, not data work:
figure()to_png()The real 10M kernel work is under 8 ms and already memory-bound at 88–105 GB/s. Two non-engine costs dominate the rest, and this PR removes both.
Neither is visible to CodSpeed, by construction: its largest scatter is
LARGE_N = 1_000_000, and the session-autousewarm_lazy_modulesfixture deliberately warms lazy imports before every measured region (it was added for good reason — it killed a phantom 26% regression — but it means the dominant term of the published number cannot be observed there).What changed
1. Vendor
xml.sax.saxutils.escapeinto_svg.py. Importing it pullsurllib.request→http.client→ssl,socketand the wholeemailpackage — 35+ modules for a function whose body is threestr.replacecalls. Nothing else in xy needs that tree. All 46 call sites are untouched.2. Raster export skips the density point-sample overlay.
density["sample"]has exactly one writer and zero Python readers —_raster._emit_griddraws density traces from the grid alone. At 10M that meant running a SplitMix predicate over all 10M rows and gathering two 8,222-row columns that no pixel consumes._build_raster_payloadnow passespoint_overlay=False; the publicbuild_payload/build_payload_splitstill ship the overlay because the browser client (js/src/50_chartview.ts) draws it.Measured
Interleaved cold-process A/B at N=10M, 6 rounds, alternating order:
to_pngProof of no functionality loss
& < > " 'plus non-BMP characters in its title soescapeis genuinely exercised.ruff checkandruff format --checkclean;tyreports the same 13 pre-existing diagnostics as main.tests/test_svg_escape.py— differential fuzz of the vendoredescapeagainst the stdlib (exhaustive over all strings of length ≤3 on a hostile alphabet, plus 20k random strings × 5 entity dicts), and a fresh-subprocess assertion that a static SVG export leavesurllib.request/ssl/http.client/email/socketunimported. The subprocess matters:import pytestalone loadsemail, which would mask the regression in-process.tests/test_raster_density_overlay.py— pins both halves of the contract (wire payload keepssample, raster payload drops it), asserts the raster grid bytes equal the wire grid bytes, and covers the compact-categorical and clipped-domain branches plus the direct tier.spec/api/export.md, and the dossier'sDENSITY_SAMPLE_TARGETrow and §33 import budget.Reviewer notes
urllib— a web server,requests, orbenchmarks/_launch_interactive.pyitself — it collapses to ~1.2 ms. It is a real win for cold CLI/script/serverless exports, which is exactly what the static benchmark measures, but a long-lived app will not see the full number.to_png/to_jpeg/to_webp/savefigof any scatter past the density threshold.to_svgpays the same dead-overlay cost (~1.4 ms) because it goes through the publicbuild_payload. Left as a follow-up — it needs its own private payload path, which is a wider change than this one.benchmarks/launch_baselines/, recorded on an M5 Pro. They need regenerating on that reference machine, not on the machine these numbers came from.docs-app-codespellpre-commit hook could not run locally (it cannot installcodespellin this sandbox); it fails identically on an unchangedmaintree, so it is environmental. Changed files were spell-checked directly.Summary by CodeRabbit
Performance
Compatibility
Documentation
Tests