Skip to content

perf(data-explorer): qualify SVG path construction - #4387

Closed
dieterolson wants to merge 2 commits into
mainfrom
bolt/optimize-svg-path-building-12382126696986403940
Closed

perf(data-explorer): qualify SVG path construction#4387
dieterolson wants to merge 2 commits into
mainfrom
bolt/optimize-svg-path-building-12382126696986403940

Conversation

@dieterolson

@dieterolson dieterolson commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

What

  • Build line, scatter-trendline, and spectrum SVG path text in one pass without retaining a separate command array.
  • Share the identical line/trendline builder, preserving DRY and non-finite gap semantics.
  • Add exact gap, empty-path, command-order, and separator contracts.
  • Remove code-restating optimization comments and retain the limitation in SPEC.md.

Evidence

On the repository-declared Node 24 runtime, all 384 P1AM frontend tests pass, changed-file ESLint is zero-warning, TypeScript passes, and the 1,589-module production build succeeds.

A post-warmup 30-sample local benchmark produced byte-identical old/new output. At 1,000/10,000/100,000 points, concatenation medians were 0.210/1.954/17.751 ms versus array/join 0.285/2.181/21.896 ms. At 100,000 points, median observed heap delta was 35.093 MiB versus 36.770 MiB. These are directional local measurements, not a cross-engine or GC-pause guarantee.

Release discipline

Human review is approved. Fresh exact-head checks and ordinary protected merge behavior remain required. This PR supersedes narrower duplicate #4386.

Replaced intermediate string array allocations (`[].push()` -> `.join(' ')`)
with single-pass string concatenation for generating SVG path `d` strings
in `ScatterPlot`, `LinePlot`, and `SpectrumPlot`. This prevents massive
array allocations during high-frequency chart re-renders.

Co-authored-by: dieterolson <198168927+dieterolson@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0877c38a3b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/p1am_control_system/frontend/src/components/data_explorer/plots/LinePlot.tsx Outdated
@d-sorgcodexagent d-sorgcodexagent Bot changed the title ⚡ Bolt: Optimize SVG path building allocations perf(data-explorer): qualify SVG path construction Aug 12, 2026
@d-sorgcodexagent

Copy link
Copy Markdown
Contributor

Addressed the actionable review thread at exact head 3a2fd162d15cddaf146f0b94ca10d1be927211a5: all three code-restating comments are removed. Line and scatter now share a DRY path builder; line/scatter/spectrum have explicit gap/empty/separator contracts. The PR description and spec no longer claim eliminated GC pauses and instead record bounded Node 24 measurements and limitations.

Local final-head evidence: 384/384 frontend tests, changed-file zero-warning ESLint, TypeScript, 1,589-module production build, docs governance, and diff hygiene pass.

@dieterolson

Copy link
Copy Markdown
Collaborator Author

Consolidated into #4408. Closing to reduce CI/CD runner load.

auto-merge was automatically disabled August 13, 2026 03:27

Pull request was closed

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.

1 participant