perf(concat): move owned sourcemaps in instead of borrow + recopy#368
Conversation
Merging this PR will improve performance by 8.07%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
dbd6ef9 to
204fddf
Compare
204fddf to
5c04c20
Compare
There was a problem hiding this comment.
💡 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".
5c04c20 to
2496c22
Compare
4fa83f2 to
6aa0e05
Compare
There was a problem hiding this comment.
💡 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".
5baeb93 to
127c107
Compare
`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.
127c107 to
7ef4833
Compare
Summary
ConcatSourceMapBuilder::add_sourcemapstoresCow::Borrowedviews 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 callsinto_owned, deep-copying every name / source /sourcesContentstring: one allocation per string, on a hot per-chunk path.This adds owned-merge entry points that move the input's
names/sources/source_contentsstraight in viaSourceMap::into_parts(no string bytes copied), so the merged map is already owned andinto_ownedis unnecessary:add_sourcemap_owned(SourceMap, u32)— owned counterpart ofadd_sourcemap.from_owned_sourcemaps(..)— owned counterpart offrom_sourcemaps.add_sourcemap(borrowed) andadd_sourcemap_owned(owned) share a privateadd_tokenshelper, so the token translation / boundary-dedup /TokenChunkbookkeeping can't drift.add_tokenskeeps the translate loop branchless (raw-idToken::translated,extend, post-loop prev-id scan) so it auto-vectorizes.into_owned_sourcemapis kept for callers wanting a'staticresult 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 bulkfrom_owned_sourcemaps.closes #365