Skip to content

perf(concat): move owned sourcemaps in instead of borrow + recopy#368

Merged
Boshen merged 1 commit into
mainfrom
perf/concat-owned-move-in
Jun 20, 2026
Merged

perf(concat): move owned sourcemaps in instead of borrow + recopy#368
Boshen merged 1 commit into
mainfrom
perf/concat-owned-move-in

Conversation

@Boshen

@Boshen Boshen commented Jun 19, 2026

Copy link
Copy Markdown
Member

Summary

ConcatSourceMapBuilder::add_sourcemap stores Cow::Borrowed views into each input map, so a concatenated result borrows from its inputs. A consumer needing an owned ('static) result — e.g. rolldown joining per-module maps into one chunk map — then calls into_owned, deep-copying every name / source / sourcesContent string: one allocation per string, on a hot per-chunk path.

This adds owned-merge entry points that move the input's names / sources / source_contents straight in via SourceMap::into_parts (no string bytes copied), so the merged map is already owned and into_owned is unnecessary:

  • add_sourcemap_owned(SourceMap, u32) — owned counterpart of add_sourcemap.
  • from_owned_sourcemaps(..) — owned counterpart of from_sourcemaps.
  • add_sourcemap (borrowed) and add_sourcemap_owned (owned) share a private add_tokens helper, so the token translation / boundary-dedup / TokenChunk bookkeeping can't drift. add_tokens keeps the translate loop branchless (raw-id Token::translated, extend, post-loop prev-id scan) so it auto-vectorizes.
  • into_owned_sourcemap is kept for callers wanting a 'static result from borrowed inputs.

Existing signatures are untouched; a test asserts the owned path is byte-identical to the borrowed path.

Context

Builds on #370, which reverted the borrow-builder storage (#366) back to Cow — the storage owned-merge needs. Closely related to @hyf0's #365, which proposed the same owned-merge; this version shares the token helper between the borrowed/owned paths and adds the bulk from_owned_sourcemaps.

closes #365

@codspeed-hq

codspeed-hq Bot commented Jun 19, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 8.07%

⚡ 2 improved benchmarks
❌ 2 regressed benchmarks
✅ 12 untouched benchmarks
⏩ 7 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
lookup_table[real_small] 1.3 µs 1.4 µs -2.13%
lookup_table[real_medium] 1.5 µs 1.5 µs -1.93%
from_sourcemaps 305 µs 254.5 µs +19.82%
add_sourcemap_loop 322.7 µs 272.1 µs +18.63%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing perf/concat-owned-move-in (7ef4833) with main (5286161)

Open in CodSpeed

Footnotes

  1. 7 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.

@Boshen

Boshen commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

Update: #366 (borrow-builder storage) and #369 (its bench) were reverted in #370. This PR now applies additively on the restored Cow storage — no rebase-over-baseline needed.

@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: 5c04c20057

ℹ️ 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/concat_sourcemap_builder.rs
@Boshen
Boshen force-pushed the perf/concat-owned-move-in branch from 5c04c20 to 2496c22 Compare June 19, 2026 14:33
@Boshen
Boshen force-pushed the perf/concat-owned-move-in branch 2 times, most recently from 4fa83f2 to 6aa0e05 Compare June 19, 2026 15:41

@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: 6aa0e0542e

ℹ️ 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/concat_sourcemap_builder.rs Outdated
@Boshen
Boshen force-pushed the perf/concat-owned-move-in branch 2 times, most recently from 5baeb93 to 127c107 Compare June 20, 2026 03:59
`ConcatSourceMapBuilder::add_sourcemap` stores `Cow::Borrowed` views into each
input map, so a concatenated result borrows from its inputs. A consumer that
needs an owned (`'static`) result — e.g. rolldown joining per-module maps into
one chunk map — must then call `into_owned`, deep-copying every name / source /
`sourcesContent` string: one allocation per string, on a hot per-chunk path.

Add owned-merge entry points that take a `SourceMap` by value and **move** its
`names` / `sources` / `source_contents` storage straight in via
`SourceMap::into_parts` — no string bytes copied. The merged map is then already
owned, so the consumer can drop `into_owned` entirely:

* `add_sourcemap_owned(SourceMap<'a>, u32)` — owned counterpart of `add_sourcemap`.
* `from_owned_sourcemaps(..)` — owned counterpart of `from_sourcemaps`.
* `add_sourcemap` (borrowed) and `add_sourcemap_owned` (owned) share a private
  `add_tokens` helper, so the translation / boundary-dedup / `TokenChunk`
  bookkeeping can't drift between the two paths.
* `into_owned_sourcemap` is kept for callers wanting a `'static` result from
  borrowed inputs.

`add_tokens` translates ids on their raw representation (`Token::translated`, no
`Option` round-trip), bulk-`memcpy`s the first map when it needs no offset,
appends the rest via `extend`, and derives the next chunk's prev-id baseline with
a single backward scan — keeping the translate loop branchless so it
auto-vectorizes. The memcpy fast path is gated on all offsets being zero, so a
prior map contributing sources/names but no tokens still renumbers correctly.

Both paths pad `source_contents` with `None` to keep it index-aligned with
`sources` (input `sourcesContent` may be absent or shorter than `sources`).

Existing signatures are untouched. Tests assert the owned path produces
byte-identical output to the borrowed path, plus the two edge cases above.
@Boshen
Boshen force-pushed the perf/concat-owned-move-in branch from 127c107 to 7ef4833 Compare June 20, 2026 04:09
@Boshen
Boshen merged commit 021c3a8 into main Jun 20, 2026
8 checks passed
@Boshen
Boshen deleted the perf/concat-owned-move-in branch June 20, 2026 04:14
@oxc-guard oxc-guard Bot mentioned this pull request Jun 19, 2026
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