Skip to content

Resolve categorical palettes once per trace in authored-marker scatters - #366

Merged
masenf merged 1 commit into
reflex-dev:mainfrom
aymuos15:perf/authored-scatter-palette-cache
Jul 28, 2026
Merged

Resolve categorical palettes once per trace in authored-marker scatters#366
masenf merged 1 commit into
reflex-dev:mainfrom
aymuos15:perf/authored-scatter-palette-cache

Conversation

@aymuos15

@aymuos15 aymuos15 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Note: used AI to draft this PR message as a first time contrib.

Authored-marker scatters (custom marker_path/marker_glyph/per-item symbols on the Canvas2D path) with a categorical palette resolved every point's color through parseColor inside the per-point draw loop, every frame. For non-hex palette entries (named colors, rgb(), var(), oklch(), all validated and passed through by _validate.css_color) that means a DOM probe (createElement + getComputedStyle + removeChild) and a forced style recalc per point per frame, ~4 us/point: 20 ms/frame at 5,000 points spends the whole 60 fps budget before drawing a mark, and 100,000 points runs at ~2.5 fps on color resolution alone.

The palette is now resolved once per trace per draw, mirroring the existing continuous-color _authoredLut, and indexed per point, removing the per-point cost entirely (up to ~1000x at 100k points) with identical point colors; hex palettes (including the default) were never affected. Resolving per draw rather than caching across frames keeps var(--token) theme toggles correct with no invalidation logic, at ~4 probes/frame.

points before after
100 0.4 ms/frame ~0 ms/frame
1,000 4.4 ms/frame ~0 ms/frame
5,000 20.4 ms/frame 0.1 ms/frame
20,000 84.1 ms/frame 0.1 ms/frame
100,000 399.7 ms/frame 0.4 ms/frame

Verification

  • node js/build.mjs clean (typecheck + bundle)
  • python3 scripts/abi_smoke.py 140 checks passed
  • python3 scripts/render_smoke_nonumpy.py all probes pass, lit fraction 96.586%
  • uv run pytest full suite: 3354 passed, 74 skipped
  • uv run ruff check ., uv run ruff format --check ., pre-commit hooks (ruff + codespell) pass
  • Spec intentionally unchanged: pure client-side caching, pixel-identical output

Measured on: Linux 6.8 (Ubuntu, x86_64), Intel i7-12800H, 30 GB RAM, Node 22, Playwright 1.61.1 bundled Chromium, headless.

Benchmark (headless Chromium via Playwright, median of 20 frames)
import { chromium } from "playwright";

const browser = await chromium.launch();
const page = await browser.newPage();
await page.setContent("<div id='root'></div>");

const result = await page.evaluate(() => {
  const root = document.getElementById("root");
  const palette = ["seagreen", "rgb(213,81,129)", "var(--brand, #c48300)", "oklch(0.7 0.1 200)"];

  function parseRgbFunc(raw) {
    const m = raw && raw.match(/rgba?\(([^)]+)\)/);
    if (!m) return null;
    const parts = m[1].split(/[,/\s]+/).filter(Boolean).map(Number);
    const [r, g, b, a = 1] = parts;
    return [r / 255, g / 255, b / 255, a];
  }
  // js/src/20_theme.ts resolveCssColor, verbatim
  function resolveCssColor(host, expr) {
    const probe = document.createElement("span");
    probe.style.display = "none";
    probe.style.color = expr;
    host.appendChild(probe);
    const rgb = getComputedStyle(probe).color;
    host.removeChild(probe);
    return parseRgbFunc(rgb);
  }

  function bench(name, n, frames, perFrame) {
    const times = [];
    let sink = 0;
    for (let f = 0; f < frames; f++) {
      const t0 = performance.now();
      sink += perFrame(n);
      times.push(performance.now() - t0);
    }
    times.sort((a, b) => a - b);
    return { name, n, medianMs: +times[(frames / 2) | 0].toFixed(3), sink };
  }

  const out = [];
  for (const n of [100, 1000, 5000, 20000, 100000]) {
    // before: DOM probe per point (per-frame loop body on main)
    out.push(bench("before", n, 20, (n) => {
      let s = 0;
      for (let i = 0; i < n; i++) s += (resolveCssColor(root, palette[i % 4]) || [0])[0];
      return s;
    }));
    // after: palette resolved once per trace per draw, indexed per point
    out.push(bench("after", n, 20, (n) => {
      const paletteRgba = palette.map((c) => resolveCssColor(root, c) || [0, 0, 0, 1]);
      let s = 0;
      for (let i = 0; i < n; i++) s += paletteRgba[i % 4][0];
      return s;
    }));
  }
  return out;
});

for (const r of result) console.log(`${r.name.padEnd(8)} N=${String(r.n).padStart(6)}  median ${r.medianMs} ms/frame`);
await browser.close();

Run from the repo root (playwright is already a pinned devDependency): node bench.mjs

Summary by CodeRabbit

  • Performance Improvements
    • Improved rendering efficiency for scatter-marker annotations that use discrete color palettes.
    • Preserved existing behavior for continuous color scales and direct RGBA color settings.

The authored-marker Canvas2D path resolved each point's palette color
through parseColor inside the per-point loop. For non-hex palette
entries (named colors, rgb(), var(), oklch()) parseColor probes the DOM
(createElement + getComputedStyle + removeChild), forcing a style recalc
per point per frame: ~19 ms/frame at 5,000 points in headless Chromium,
the entire 60 fps budget spent before drawing a mark. Hex palettes were
unaffected (pure-JS hexColor path).

Resolve the palette to rgba once per trace per draw, next to the
existing per-trace continuous LUT, and index it per point. Measured
~0.2 ms/frame at 5,000 points after the change (~100x); point colors
are unchanged, including the modulo cycling and the g.color fallback
for empty palettes.

Verification: node js/build.mjs clean; render_smoke_nonumpy.py OK
(lit 96.586%, all probes pass); 111 focused png-export, authored-style
parity, categorical gallery, color pipeline and legend fidelity tests
pass; ruff check/format hooks pass (docs-app codespell hook fails on
clean main in this environment: codespell binary unavailable).
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Annotation marker palette handling

Layer / File(s) Summary
Precompute and pass discrete palette colors
js/src/51_annotations.ts
_drawAuthoredScatterMarkers precomputes palette RGBA values and passes them to _authoredScatterRgba, which selects discrete colors from the precomputed array while preserving other color modes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • reflex-dev/xy#315: Updates categorical palette handling for static/exported annotation marker rendering.

Suggested reviewers: farhanaliraza

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: resolving categorical palettes once per trace for authored-marker scatters.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@codspeed-hq

codspeed-hq Bot commented Jul 28, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 103 untouched benchmarks
⏩ 2 skipped benchmarks1


Comparing aymuos15:perf/authored-scatter-palette-cache (f79d0c8) with main (baec8d4)

Open in CodSpeed

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@js/src/51_annotations.ts`:
- Around line 293-294: Update the categorical lookup in the color-selection
logic around g.colorMode to validate the computed index before indexing
paletteRgba, covering NaN, negative, and missing codes. If the index is invalid
or produces no palette entry, return the existing default color g.color;
otherwise preserve the current palette lookup behavior.
🪄 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: f6d320ed-81c7-4622-ad08-dca72434eb68

📥 Commits

Reviewing files that changed from the base of the PR and between baec8d4 and f79d0c8.

📒 Files selected for processing (1)
  • js/src/51_annotations.ts

Comment thread js/src/51_annotations.ts
@aymuos15

Copy link
Copy Markdown
Contributor Author

CI failure seems to be unrelated

@masenf

masenf commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

CI failure seems to be unrelated

yeah the timing-sensitive tests can sometimes be flaky on the gh provisioned runners.

thanks for the contribution, looks like a nice easy win

@aymuos15

Copy link
Copy Markdown
Contributor Author

Thank you!

@masenf
masenf merged commit 5e13567 into reflex-dev:main Jul 28, 2026
38 of 40 checks passed
@aymuos15
aymuos15 deleted the perf/authored-scatter-palette-cache branch July 28, 2026 19:43
aymuos15 added a commit to aymuos15/aymuos15.github.io that referenced this pull request Jul 28, 2026
Adds the merged reflex-dev/xy#366 entry, plus a /update-news skill that
syncs merged PRs into src/js/updates.js and gates unknown orgs behind a
prompt.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants