Skip to content

refactor: apply borrow builder pattern to ConcatSourceMapBuilder#366

Merged
Boshen merged 1 commit into
mainfrom
refactor/concat-borrow-builder
Jun 19, 2026
Merged

refactor: apply borrow builder pattern to ConcatSourceMapBuilder#366
Boshen merged 1 commit into
mainfrom
refactor/concat-borrow-builder

Conversation

@Boshen

@Boshen Boshen commented Jun 19, 2026

Copy link
Copy Markdown
Member

Mirror the SourceMapBuilder change in #361 for ConcatSourceMapBuilder.

What

  • Store &'a str internally (names / sources / source_contents) instead of Cow<'a, str>, so concatenation does no string allocation and holds no owned variant during building.
  • add_sourcemap extends directly from the &'a str-yielding SourceMap getters, dropping the per-string Cow::Borrowed wrapping.
  • Defer the borrow-vs-own decision to the terminal methods:
    • into_sourcemap wraps in Cow::Borrowed (zero copy, as before).
    • new into_owned_sourcemap copies once into a 'static OwnedSourceMap, matching the SourceMapBuilder surface.

Compatibility

Backward compatible — fields are pub(crate) and existing method signatures are unchanged; only into_owned_sourcemap is added.

Verified by patching oxc and rolldown at this branch: both build clean and their sourcemap tests pass (incl. rolldown's source_joiner::test_concat_sourcemaps, the real ConcatSourceMapBuilder consumer).

Mirror the `SourceMapBuilder` change in #361: store `&'a str` internally
instead of `Cow<'a, str>` and defer the borrow-vs-own decision to the
terminal methods. Adds `into_owned_sourcemap` alongside `into_sourcemap`
to match the `SourceMapBuilder` surface.

The public API is backward compatible (fields are `pub(crate)`, existing
method signatures are unchanged).
@codspeed-hq

codspeed-hq Bot commented Jun 19, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 3.18%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 2 regressed benchmarks
✅ 14 untouched benchmarks
⏩ 5 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
from_sourcemaps 304.9 µs 317.9 µs -4.06%
add_sourcemap_loop 322.7 µs 330.3 µs -2.29%

Tip

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


Comparing refactor/concat-borrow-builder (7c349ae) with main (1bb86f5)

Open in CodSpeed

Footnotes

  1. 5 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 merged commit e55562d into main Jun 19, 2026
8 checks passed
@Boshen
Boshen deleted the refactor/concat-borrow-builder branch June 19, 2026 07:42
@oxc-guard oxc-guard Bot mentioned this pull request Jun 19, 2026
Boshen added a commit that referenced this pull request Jun 19, 2026
#366 switched `ConcatSourceMapBuilder` to borrowed `&'a str` storage, which is
optimal when inputs outlive the result. But its main consumer (rolldown's
`SourceJoiner`) feeds *owned* `SourceMap<'static>` maps and wants an owned map
back, so `into_owned_sourcemap` deep-copied every name/source/sourcesContent
string — and `sourcesContent` is the full module text.

Store entries as `Cow<'a, str>` again so the builder can absorb owned strings by
moving them, and add:

* `add_sourcemap_owned(SourceMap<'a>, u32)` / `from_owned_sourcemaps(..)` — consume
  owned maps and move their `Cow` entries in (no string bytes copied).
* `add_sourcemap` (borrowed) and `add_sourcemap_owned` now share an `add_tokens`
  helper; only how the strings get in differs.

`into_sourcemap` moves the accumulated vectors straight out (also cheaper than
#366's rebuild for the borrowed case); `into_owned_sourcemap` keeps moved-in owned
strings as-is and copies only borrowed ones.

The `concat/owned_maps` benchmark (added in #369 as the borrow baseline) is switched
to the move-in path here. From the investigation sweep (200 modules): move-in is flat
at ~32us regardless of `sourcesContent` size, while borrow + `into_owned_sourcemap`
grows linearly (37us @256b, 47us @2KB, 70us @8kb, 542us @32kb) — up to ~16x faster,
identical mappings output.
Boshen added a commit that referenced this pull request Jun 19, 2026
#366 switched `ConcatSourceMapBuilder` to borrowed `&'a str` storage, which is
optimal when inputs outlive the result. But its main consumer (rolldown's
`SourceJoiner`) feeds *owned* `SourceMap<'static>` maps and wants an owned map
back, so `into_owned_sourcemap` deep-copied every name/source/sourcesContent
string — and `sourcesContent` is the full module text.

Store entries as `Cow<'a, str>` again so the builder can absorb owned strings by
moving them, and add:

* `add_sourcemap_owned(SourceMap<'a>, u32)` / `from_owned_sourcemaps(..)` — consume
  owned maps and move their `Cow` entries in (no string bytes copied).
* `add_sourcemap` (borrowed) and `add_sourcemap_owned` now share an `add_tokens`
  helper; only how the strings get in differs.

`into_sourcemap` moves the accumulated vectors straight out (also cheaper than
#366's rebuild for the borrowed case); `into_owned_sourcemap` keeps moved-in owned
strings as-is and copies only borrowed ones.

The `concat` benchmark (added in #369 as `ConcatSourceMapBuilder::from_sourcemaps`,
the borrow baseline) is switched here to `ConcatSourceMapBuilder::from_owned_sourcemaps`.
From the investigation sweep (200 modules): move-in is flat at ~32us regardless of
`sourcesContent` size, while borrow + `into_owned_sourcemap` grows linearly
(37us @256b, 47us @2KB, 70us @8kb, 542us @32kb) — up to ~16x faster, identical
mappings output.
Boshen added a commit that referenced this pull request Jun 19, 2026
#366 switched `ConcatSourceMapBuilder` to borrowed `&'a str` storage, which is
optimal when inputs outlive the result. But its main consumer (rolldown's
`SourceJoiner`) feeds *owned* `SourceMap<'static>` maps and wants an owned map
back, so `into_owned_sourcemap` deep-copied every name/source/sourcesContent
string — and `sourcesContent` is the full module text.

Store entries as `Cow<'a, str>` again so the builder can absorb owned strings by
moving them, and add:

* `add_sourcemap_owned(SourceMap<'a>, u32)` / `from_owned_sourcemaps(..)` — consume
  owned maps and move their `Cow` entries in (no string bytes copied).
* `add_sourcemap` (borrowed) and `add_sourcemap_owned` now share an `add_tokens`
  helper; only how the strings get in differs.

`into_sourcemap` moves the accumulated vectors straight out (also cheaper than
#366's rebuild for the borrowed case); `into_owned_sourcemap` keeps moved-in owned
strings as-is and copies only borrowed ones.

The `concat` benchmark group covers all four entry points: `from_sourcemaps` /
`add_sourcemap` (borrow — the baseline added in #369) plus `from_owned_sourcemaps` /
`add_sourcemap_owned` (the move-in variants added here). From the investigation
sweep (200 modules): move-in is flat at ~32us regardless of `sourcesContent` size,
while borrow + `into_owned_sourcemap` grows linearly (37us @256b, 47us @2KB,
70us @8kb, 542us @32kb) — up to ~16x faster, identical mappings output.
Boshen added a commit that referenced this pull request Jun 19, 2026
#370)

* Revert "bench: add a representative concat sourcemap benchmark (#369)"

This reverts commit 8d72a6a.

* Revert "refactor: apply borrow builder pattern to ConcatSourceMapBuilder (#366)"

This reverts commit e55562d.
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