Repository navigation
Conversation
A four-field mappings segment that came after a five-field segment got the name of that earlier segment. Both decoders stored the running name index once any segment had a name. Each row now stores the name index of its own fifth field, or -1. The parse-wide has_names flag has no reader after that, so it is gone from the kernel state and from the scalar loop. The batched path of the kernel now prefetches its output. Its speed on output that is not in the cache depended on how the function happened to compile.
StatusReproduction, on main (3f3a9ff) and on Bun 1.4.2: const { SourceMap } = require("node:module");
const map = new SourceMap({ version: 3, sources: ["a.ts"], names: ["x", "y"], mappings: "AAAAA,CAAC,CAAC;" });
console.log([0, 1, 2].map(column => map.findEntry(0, column).name));
CIBuild 137050 finished. No failure touches this diff.
|
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Parser state layout src/highway/lib.rs, src/jsc/bindings/highway_sourcemap.cpp |
The Rust and C++ parser state layouts no longer include has_names; their size checks specify nine i32 values. |
Per-segment name parsing src/sourcemap/Mapping.rs, src/jsc/bindings/highway_sourcemap.cpp, test/js/node/module/sourcemap-simd.test.ts, test/js/node/module/sourcemap.test.js |
Scalar and SIMD parsing assign names to segments with a fifth field and return no name for four-field segments. The SIMD batch path prefetches output columns 64 rows ahead. Tests cover segment boundaries, lookup results, fixture mappings, SIMD paths, and truncated name-field VLQs. |
Possibly related PRs
- oven-sh/bun#47380: Changes VLQ decoding for long fields in the same SIMD/scalar mapping decoder path.
Suggested reviewers: alii, jarred-sumner
Merge Risk: 
2b63f
-
The change prevents unnamed source-map segments from inheriting earlier names. No actionable merge-blocking risk is established by the supplied evidence; merge after normal checks pass.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: assigning a name to a segment only when the segment contains a name field. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description is complete and directly addresses the required information. It explains what the PR changes, the reported problem, design decisions, limitations, performance impact, and verification … Full details: Description check
-
Explanation
The description is complete and directly addresses the required information. It explains what the PR changes, the reported problem, design decisions, limitations, performance impact, and verification results. It does not use the exact template headings, but it includes equivalent content and detailed test evidence.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked: the new hwy::Prefetch calls can form addresses up to 64 rows past the end of the output buffer, but prefetch hints never fault and no store past os_end was added; the kSt* renumbering in highway_sourcemap.cpp matches the 9-field Rust ParseMappingsState on both sides (size and offsetof asserts updated together); and the unconditional name_idx accumulate in the general loop is sound because d_name is zero-initialized and only assigned when field_count == 5.
Extended reasoning...
The change touches the SIMD source-map kernel (C++), its Rust state mirror, and the scalar parser in src/sourcemap/Mapping.rs, plus tests; it touches no auth, crypto, or injection surface. The one inline finding concerns 6-field segment handling that the PR reformats but does not change in behavior. The note records the kernel-level concerns I ruled out from reading the diff so a human reviewer can focus elsewhere.
Problem
SourceMap.findEntry()andfindOrigin()(node:module) give a four-fieldmappingssegment an earlier segment's name: with names["x", "y"]and mappingsAAAAA,IAAI;,findEntry(0, 4).nameis"x". Node.js v26.3.0 answersundefined. No user reported it.src/sourcemap/Mapping.rs:715,src/jsc/bindings/highway_sourcemap.cpp:604,:766,:965.Fix
has_namesthen has no reader and is deleted.test/js/node/module/sourcemap.test.js,sourcemap-simd.test.ts(15 and 3 tests fail on main). Alsotest/js/bun/sourcemap/.Background
mappingssegment has 1, 4 or 5 VLQ fields. The fifth is a delta on a running name index.Downsides
names[nameIndex](SourceMap.findEntry() returns a stale name for a final mapping segment that has no name index nodejs/node#63795): 451 of 594 named maps undertest/node_modules.mappingssegment that has no source #47734).Notes
Who it reaches
new SourceMap(payload)keeps names (src/runtime/api/JSSourceMap.rs:154has the oneallow_names: true). So it reaches a caller that builds aSourceMapfrom a map with names and reads.name. Stack traces do not read names.bun compile --minify --sourcemap=externalwrites a name only for a site inside a named function (src/aot/report/obfuscation_map.rs).docs/compiler/debugging.mdxsays that any source map library recovers the names. On main, Bun's ownSourceMapnames two anonymous frames of a 14-line programtraverseArrayandmap. Node.js and this PR give them no name.find test/node_modules -name '*.map' -type flists 8,006 files, 594 with a named segment. An independent decoder checkedfindEntry().nameat 5,929,755 segments. Main names 1,544,702 four-field segments. Node.js v26.3.0 names 451, each the unterminated last segment of a map. This PR names none, with the kernel and with the scalar decoder. All 625,603 five-field segments have their own name in all three.Node.js and the end of the string
Node.js sets
nameper segment, only when a fifth field follows: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/source_map/source_map.js#L304-L308. At the end of the stringpeek()returns'', which is not a separator, so Node.js reads fields that are not there. nodejs/node#63795 reports that. Its fix PRs nodejs/node#63801 and nodejs/node#66264 are open.["x", "y"]AAAAA,IAAI;xAAAAA,CAACxxAAAAA,CAAC,xxAAAAxAAAAA,C,CAAC;xxtest/js/node/test/fixtures/test426expectsmappedName: nullfor each four-field segment ofbasicMapping. Main fails 7 of its 12 checks. A new test runs that fixture test.Nonearm of thematchinmapping::parse.mappingssegment that has no source #47734 changes that.findOrigintakes 0-based arguments in Bun and 1-based arguments in Node.js. This PR does not change that. The newfindOrigintest uses a map with two equal lines, so it holds for both.The batched path and the prefetch
new SourceMapover 200,000 batched segments (oBAAC,) took 13% to 28% longer on an AVX-512 host. The loop drops each map before it builds the next. It was +7% at 16,000 segments and equal at 2,000 and at 2,000,000. Instructions per row fell (95.1 to 92.5).memsetcall measured the same. A harness calls the kernel on output that is not in the cache. There main runs 26.1 cycles per row and the name rule alone 33.2. Main with only the general loop's name store changed runs 28.8 to 33.1, and this input does not reach that loop.memcpyafter two loads of staging arrays stall on store forwarding. How early the output lines reach the cache depended on how the function compiled. Demand fills from DRAM over 200 constructs: 180,000 to 290,000 on main, 830,000 to 1,330,000 with the name rule alone, 43,000 to 47,000 with the prefetch.hwy::Prefetchon its four output columns, 64 rows ahead.hwy::Prefetchcompiles to nothing without SSE2. No ARM machine was available to measure that path.new SourceMap, wall ns per row, release builds of main 3f3a9ff and of this PR, AMD EPYC with AVX-512, median of 5 interleaved rounds:oBAAC,)CAAC,)Another run of the 2,000-segment row (8 rounds) measured +1.3%.
Kernel cycles per row in the harness, 30 cells (5 shapes, block 16/32/64, name column on/off), median of 4 rounds, this PR against main:
Measurements
new SourceMapinstructions per row, main to PR, SIMD / scalar-only: uniform4 51.18 to 49.35 / 155.06 to 151.02, batch4 95.29 to 93.61 / 165.90 to 161.91, mixed5 200.00 to 200.27 / 233.89 to 223.58, lines4 142.58 to 140.83 / 159.51 to 155.74, all5 145.13 to 144.72 / 190.48 to 183.30. Run-to-run noise is 0.15%.estimateShallowMemoryUsageOf): name-lessnew SourceMap, 20 rows: 1,048 to 1,048 bytes. 200,000 rows: 5,734,496 to 5,734,496 bytes.ParseMappingsImpl36,661 to 36,018 bytes (8 ISA copies),mapping::parse5,424 to 5,342. Release binary 140,474,656 to 140,474,656 bytes.ParseMappingsImpl, per ISA copy: main 3 / 4, PR 3 / 5, plus 4prefetcht0. The batched path fills the name column withmemset.bench/snippets/source-map-parse.mjs, 9.5 MB map: main 11.34 ms, PR 11.20 ms (median of 6 interleaved rounds, spread 8%).scripts/verify-baseline-staticpasses on the release build.Design notes
name_idx = WrapAdd(name_idx, d_name)has no condition on purpose. Underif (field_count == 5)the kernel is above main in 13 of the 36 instruction cells (up to +4.3%, AVX2 copy).allow_namesmeasured +7% to +12% time for a name-less map with the SIMD decoder off, because each row is 24 bytes and not 20.hn::StoreNfor the batched name store is above main in 11 of the 12 NEON instruction cells. A prefill of the name column with -1 adds a pass over the column to each construct.Tests
sourcemap-simd.test.tspinned the carried name in two places (entries[81]andentries[139]were"n0"). They arenullnow, as in Node.js.ParseMappingsState. The kernel indexes that state by position and Rust by field name.test/js/bun/sourcemap/,test/js/node/module/module-sourcemap.test.js,test/cli/install/bun-pm-diff.test.ts,test/js/bun/util/inspect-error.test.js,test/bake/dev/sourcemap.test.ts,test/bake/dev/server-sourcemap.test.ts.Other open PRs in these files
mappingssegment that has no source #47734 (one-field segments) and sourcemap: decode a VLQ written with more than 8 digits #47380 (longer VLQs) merge with this branch without a conflict.sourcemap-simd.test.ts. Its row model carries the name and needs the new rule.[human-review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file