Conversation
robhogan
added this pull request to stack #1982
September 26, 2026 07:54
robhogan
force-pushed
the
pr1981
branch
2 times, most recently
from
September 26, 2026 08:39
9160f47 to
b25b719
Compare
There was a problem hiding this comment.
Copilot review overview
馃煝 Approval recommended
No unresolved review issues were identified, and regression coverage is included.
Review effort: Lite
Findings: None
What changed in this PR
Updates composeSourceMaps to preserve distinct mappings sharing a generated column.
Changes:
- Traces each mapping from its own original position.
- Avoids redundant lookups.
- Adds regression coverage for repeated generated columns.
| File | Description |
|---|---|
packages/鈥媘etro-source-map/鈥媠rc/鈥媍omposeSourceMaps.js |
Preserves and composes each mapping independently. |
packages/鈥媘etro-source-map/鈥媠rc/鈥媉_tests__/鈥媍omposeSourceMaps-test.js |
Verifies repeated-column mappings are retained. |
馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
robhogan
marked this pull request as ready for review
September 28, 2026 12:22
robhogan
force-pushed
the
pr1981
branch
3 times, most recently
from
September 29, 2026 15:43
ee369eb to
ba21973
Compare
`composeSourceMaps` iterates the mappings of the last map, and looked each one's generated position up again in that same map before tracing it back through the earlier maps. That second lookup is redundant, except where the last map has more than one mapping at the same generated column - which `hermesc` emits (15,689 in our benchmark app's Hermes map*). There, every mapping at the column resolved to the last one, and the generator collapsed the duplicates. This traces each mapping from its own original position instead, so the composed map keeps all of them. Lookups through Metro's `Consumer` are unchanged, because it resolves a column to its last mapping. Consumers that take the first mapping at a column, like `@jridgewell/trace-mapping`, now get the first mapping's own position rather than the last's. `composeSourceMaps` with a bundle map and its Hermes map is 8.1% faster than the previous diff (95% CI 7.6-8.7%) and 2.59x faster than `main` (2.57-2.61x; 878ms -> 805ms on our benchmark app*). The output gains 13,867 mappings (+0.8%) at repeated columns and is otherwise identical. Composition's memory is within 0.4% of the previous diff (+2MB*). | | Time (median) | vs previous | vs `main` | Composition memory* | vs previous | vs `main` | Peak RSS | |---|---|---|---|---|---|---|---| | `main` | 2,088ms (2,082 to 2,094) | | | 1,070MB (1,070 to 1,070) | | | 1,664MB | | Previous diff | 878ms (872 to 883) | | -58.0% (-58.1 to -57.8) | 908MB (908 to 909) | | -15.1% (-15.2 to -15.0) | 1,502MB | | This diff | 805ms (802 to 811) | -8.1% (-8.7 to -7.6) | -61.4% (-61.6 to -61.1) | 911MB (910 to 912) | +0.2% (+0.1 to +0.4) | -14.9% (-15.0 to -14.7) | 1,505MB | ## Changelog ``` - **[Fix]**: `composeSourceMaps` keeps every mapping at a generated column that the last map maps more than once ``` ## Test plan The new test fails on the previous diff (only the last position survives) and passes on this one. ``` yarn jest packages/metro-source-map packages/metro-symbolicate yarn flow check yarn lint yarn jest ``` \* Benchmark: Mattermost Mobile 2.45.0 (React Native 0.83.9, Metro 0.83.7), iOS release bundle, unminified: 7,086 sources, 52.6MB bundle, 82.4MB flat source map. Composed with its `hermesc -O -output-source-map` map (hermes-compiler 0.14.1; 10.7MB, 1.84M segments). Timings are `composeSourceMaps` alone, excluding JSON parsing, over 105 rounds on Node 22.13.1 on an M5 Pro; each round runs `main`, every diff in this stack and a memory baseline, in a shuffled order. Figures are medians with 95% bootstrap confidence intervals - for changes, of the per-round paired ratio. Composition memory is peak RSS less that of the same process parsing both maps without composing them (594MB).
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
composeSourceMapsiterates the mappings of the last map, and looked each one's generated position up again in that same map before tracing it back through the earlier maps:metro/packages/metro-source-map/src/composeSourceMaps.js
Lines 115 to 125 in b7c2055
That's redundant, except where the last map has more than one mapping at the same generated column, which
hermescemits (15.7k in our benchmark's Hermes map). There, every mapping at the column resolved to the last one and the duplicates were collapsed.This change
This traces each mapping from its own original position, so the composed map keeps all of them. Lookups through Metro's
Consumerare unchanged, since it resolves a column to its last mapping. Consumers that take the first mapping at a column, like@jridgewell/trace-mapping, now get that mapping's own position.composeSourceMapsis 8.1% faster than #1980 (95% CI 7.6-8.7%) and 2.59x faster thanmain(878ms -> 805ms on our benchmark app). The output gains 13,867 mappings (+0.8%) at repeated columns and is otherwise identical.Benchmark
AI-driven.
mainmainmainChangelog
Test plan
New test fails on #1980 (only the last position survives) and passes here.
Benchmark (AI-driven): Mattermost Mobile 2.45.0 (React Native 0.83.9, Metro 0.83.7), iOS release bundle, unminified: 7,086 sources, 52.6MB bundle, 82.4MB flat source map. Composed with its
hermesc -O -output-source-mapmap (hermes-compiler 0.14.1, 10.7MB, 1.84M segments). Timings arecomposeSourceMapsalone, excluding JSON parsing, over 105 rounds on Node 22.13.1 on an M5 Pro. Each round runsmain, every PR in this stack and a memory baseline, in a shuffled order. Figures are medians with 95% bootstrap confidence intervals, for changes of the per-round paired ratio. Composition memory is peak RSS less that of the same process parsing both maps without composing them (594MB).