Repository navigation
Conversation
A segment of `mappings` with one field says that the generated code from its column on comes from no source. The two decoders dropped such a segment. A lookup in that code then found the mapped segment before it and named a wrong original position. The scalar decoder and the SIMD kernel now keep a row for the segment. The row has -1 as its source index, original line and original column. `mapping::List::find` and `ParsedSourceMap::find_mapping` return `None` for a position in such a row. A stack frame there keeps its position in the file that was loaded. `SourceMap.prototype.findEntry` returns the generated position of the segment and nothing else, as Node.js does, and `findOrigin` returns an empty object. Code coverage does not count a function in such code as a function of the file. `for_each_mapping` skips the row. `write_vlqs` writes it as one field. A one-field segment that ends the string is read as Node.js reads it: as a mapped segment with zero deltas. In a `SourceMap` object of a map that has no sources, that segment has no source. `InternalSourceMap::from_vlq` still skips a one-field segment. Its input is a map that Bun wrote, and Bun writes none.
The decoder read a one-field segment with no delimiter after it as Node.js v26.13.0 does: as a mapped segment with zero deltas. In Node.js that is a read past the end of the string (nodejs/node#66264). ECMA-426 and its test suite give the segment no source. `mapping::parse` now keeps the same row for every one-field segment, and `write_vlqs` writes no `;` after one that ends the list. Tests: - The rows for the end of the string have their own table. - The three cases of the test of nodejs/node#66264 and the single-field case of the specification tests are added. - A stack frame in a segment that ends the string keeps its position in the loaded file. - The SIMD tests read the name of a one-field row that lies between segments with names. - The coverage test has a mapped segment after the one-field segment, and its map names the loaded file as the source.
The row of a one-field segment was appended in the loop of each decoder. That changed the code of the loops for every other segment: - `mapping::parse` had two calls of `List::append`, so rustc no longer inlined the one of a mapped segment: 41 more instructions for each row. - The SIMD kernel ran 2 more instructions for each segment of its general loop. The scalar decoder now appends the row in `append_without_source`, a cold function that does not go through `List::append`. The kernel stores it in `StoreRowWithoutSource`, which is not inlined. Measured on a release build, `new SourceMap()` of the map of next@16.4.3 `app-page.runtime.prod.js` (162,975 segments, 477 with one field): 26,552,948 instructions on main, 26,300,300 now. The kernel (AVX-512 target) runs 411,943 fewer, `mapping::parse` 148,673 more (3 for each segment that the scalar decoder reads).
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 1 minute. View limit details
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. |
Collaborator
Author
Collaborator
Author
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
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 traced every consumer of the new -1 rows: the remaining find_mapping callers (SavedSourceMap, VirtualMachine, CodeCoverage, sourcemap/lib.rs) all go through List::find, which now filters on has_source; the one reader of the raw columns outside ParsedSourceMap (pm_diff_normalize.rs) already skips rows with a negative original line; and the SIMD kernel's new os >= os_end bail covers the extra row before any of og/oo/os/on is written.
Extended reasoning...
The change makes both the scalar and SIMD mappings decoders retain a sentinel row (source_index -1, Ordinal::INVALID original) for one-field VLQ segments and threads that through find/find_segment, for_each_mapping, the re-encoder and node:module SourceMap.findEntry; it touches no auth, crypto or injection surface. The one confirmed inline finding (sort cost on reverse-ordered one-field segments) is the only open issue; the sentinel-consumer audit found no reader that treats -1 as an index.
This was referenced Nov 2, 2026
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.
Related to #2125
Problem
mappingssegment marks generated code with no source (ECMA-426). Both decoders drop it, so a lookup there finds the segment before it.mappings: "AAAA,I,IAAQ",findEntry(0, 5)namesa.ts0:0 (Node.js v26.13.0: the generated position only). A// @bunframe there printsat glue (orig.ts:11:5).onLoadreturned #46148 lists it under Limits.Fix
mapping::parseand the SIMD kernel keep a row for it, with -1 as source index, line and column.find_mappingreturnsNonethere: the frame stays in the loaded file,findOriginreturns{}, coverage counts no function.findEntryreturns the generated position only.test/js/node/module/sourcemap.test.jsand three more files (24 fail on main). 10,323,998 lookups on 103 maps equal Node.js with that fix. Self-reviewed: 30 concerns raised, 28 addressed (see notes).Background
mapping::Listholds the decoded rows fornew SourceMap(),// @bunfiles with a sidecar map, coverage and the dev server.;AAAA;A.Downsides
find_mapping: +5 instructions per lookup. Scalar decoder: +3 per segment (+1.7%). Binary: up to +16 KB. A 20 or 24 B row per one-field segment.Notes
Who reaches this on main
module.findSourceMap()is off on main. Two paths read a map that another tool wrote:new SourceMap(payload), and a// @bunfile whosesourceMappingURLmap comes from another tool. Bun's own writer puts four fields in each segment (append_mapping_to_buffer), so no map that Bun writes has such a segment..mapfiles, 103 have a one-field segment (23,805 of 81,892,954 segments). 68 of the 103 belong to two versions ofnext(next@16.4.3: 38 maps, next@15.5.28: 30). 78 of the 103 end with one. Example:next@16.4.3/dist/compiled/next-server/app-page.runtime.prod.js.maphas 477 in 162,975 segments.--enable-source-mapsis on. Runtime plugins: positions go through the inline source map of whatonLoadreturned #46148 and bundler: compose source maps through an input file's sourceMappingURL map #45638 read the maps of other tools through the same decoder.Lookups on the 103 maps
findEntryat the first column of each segment and at the column after it: 10,323,998 lookups for each runtime. A;appended tomappingsmakes Node.js v26.13.0 read the last segment as nodejs/node#66264 does, and changes nothing else.;appended{}The 1,996 are lookups at a column where a mapped segment starts.
Differs from Node.js v26.13.0, on purpose
#parseMaptestsisSeparator(peek()), which is false at the end of the string, so it reads the segment as a mapped one with zero deltas: https://github.com/nodejs/node/blob/v26.13.0/lib/internal/source_map/source_map.js#L290-L308mappingSemanticsSingleFieldSegment(AAAC,E) expects none, and it is now a test here. sourcemaps: don't map a trailing generated-only segment nodejs/node#66264 (open) changes Node.js to the same answer. Its three test cases are a test here too.})(),module.exports=i})();, and main and Node.js v26.13.0 name the last mapped source for it.JSSourceMap, not the decoder. In the decoder it makes a frame of a// @bunfile with the map;AAAA;Aname the original file where main names the loaded file.Still different from Node.js, not changed here
nameof the five-field one. Node.js gives it none. sourcemap: give a segment a name only when it has a name field #47796 changes that.findEntrylooks on the line of the position only. Node.js answers with the last segment of an earlier line. A test here pins the current answer. The order of the keys of an entry, and the arguments and keys offindOrigin, differ too. node:module: SourceMap.findEntry finds the segment Node.js finds, findOrigin is one-based #47803 changes these. It is stacked on this PR and revives node:module: align SourceMap.findOrigin/findEntry/payload with Node.js #35123.InternalSourceMap::from_vlq(the map in abun build --compileexecutable) still skips a one-field segment. Its input is what the bundler writes, which has none today. bundler: fix source map names, closing-brace mappings, null segments, and URL escaping #40834 makes the bundler write them. The one of the two that lands second makesfrom_vlqkeep the row.sections) are not read.Order against open PRs
validMappingLargeVLQexpectsfindEntry(0, 1)to be{}for a map whose one segment has one field. With this PR it is the generated position. Both PRs change the comment of the one-field arm of the kernel.onLoadreturned #46148: its coverage rule reads a sidecar map only when the one source of the map is the file itself. The coverage test here has such a map, so it should hold in either order. The line about one-field segments under its Limits can go.sourcemap-simd.test.ts. That expectation changes.mappings.find, which now returnsNonein code with no source.Mapping.rsandhighway_sourcemap.cpp.Costs
Release builds of main and of this branch, with no ASLR, a fixed allocator seed and JSC's concurrent JIT and GC off. The rows of the Next.js map count the instructions of one
new SourceMap()between two markers. The othernew SourceMap()rows areinstructions:uof a run with five constructions minus a run with none, divided by five. Repeated runs differ by less than 500 instructions.bun, stripped, linux-x64, built herenew SourceMap()of the Next.js map above (974,001 B ofmappings)mapping::parse(the 48,628 segments after the kernel stops at a 12-byte segment)new SourceMap(), 160,000 four-field segmentsnew SourceMap(), 160,000 four- and five-field segmentsfindOrigin, per lookup (2,000,000 lookups)findEntry, per lookup (2,000,000 lookups)mapping::parse, a second call ofList::appendin the loop made rustc stop inlining the first (41 more instructions per row). The row of a one-field segment is appended by a cold function that does not go throughList::append.MultiArrayList::sort. Main reaches it with four-field segments. Node.js v26.13.0 takes 9.6 ms and 17.2 ms for the two inputs. collections: MultiArrayList stable sort is O(n log n), not insertion sort #34525 replaced the sort and was closed by the stale sweep with no review. No map of the 103 has a negative column delta.Tests
test/js/node/module/sourcemap.test.jstest/js/node/module/sourcemap-simd.test.tstest/js/bun/util/inspect-error.test.jstest/cli/test/coverage.test.tsAlso run with the debug build:
internal-sourcemap-roundtrip,internal-sourcemap,compile-sourcemap-internal,bun-build-compile-sourcemap,external-sourcemap-stack-leak,module-sourcemap,negative-vlq-mapping,source-positions,bake/dev/sourcemap,bake/dev/server-sourcemap,inspect-inline-sourcemap,node-module-module.Self-review
30 concerns. The ones that changed the code:
for_each_mapping. It has one now, and its map names the loaded file as the source.Not taken:
test.todorows with the values of Node.js for thenameof a four-field segment. The change that fixes the name adds them.FNFandFNHonly). TheDAline is what shows that a one-field segment starts no line of its own.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/test/coverage.test.ts