Repository navigation
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
build
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66707 +/- ##
==========================================
- Coverage 90.67% 90.66% -0.02%
==========================================
Files 797 797
Lines 278791 278791
Branches 53616 53619 +3
==========================================
- Hits 252807 252775 -32
- Misses 16334 16375 +41
+ Partials 9650 9641 -9 🚀 New features to boost your workflow:
|
Contributor
Author
|
@richardlau @legendecas @joyeecheung could you take a look? This backports my V8 fix for #64155 (v8/v8@8dcf14f490c2). It didn't apply cleanly, so the first commit has three small adaptations for deps/v8, listed in the commit message: The A CI run, including V8 CI, would be appreciated. |
eliau2005
marked this pull request as draft
October 15, 2026 13:27
Contributor
Author
|
Heads-up: the upstream V8 change is being reverted (https://crrev.com/c/8574549) because |
Original commit message:
Reland "[parser] Create debug magic comment values as source substrings"
This is a reland of commit 8dcf14f490c20b23b90f9f4af56d52abdd933a0c
Changes since the original:
The original made every value with at least SlicedString::kMinLength
characters a SlicedString of the source, including usual source URLs
like //# sourceURL=inside-direct-eval.js. Heap snapshots name sliced
strings "(sliced string)" instead of by their contents, so DevTools'
heap snapshot ContextAnalyzer, which reads the name of the script's
source_url node, showed "(sliced string)" instead of the source URL.
The source URL is now always a sequential copy of its value, as
before the original (which internalized it), since it is used as the
script name (stack traces, profiles, heap snapshots). The debug ID is
also copied, since it is short. Only the sourceMappingURL value is a
substring of the source, which covers the case the original was about
(huge inline source maps). A huge sourceURL or debugId value is copied
onto the heap as before the original.
Added a regression test that a source URL longer than
SlicedString::kMinLength is sequential (script, streaming and direct
eval) and that heap snapshots name the source_url node by it, and
updated the existing tests accordingly.
Original change's description:
> [parser] Create debug magic comment values as source substrings
>
> The values of the //# sourceURL, sourceMappingURL and debugId magic
> comments were copied into LiteralBuffers while scanning and then
> internalized. An inline source map (sourceMappingURL=data:...) can be
> tens of MiB, so this allocated a string of that size on the heap in a
> non-throwing allocation, e.g. aborting a Node.js worker with a small
> old-generation limit even though the (external) source fits.
>
> The scanner now only records the source locations of these values, and
> the values are created as substrings of the script source:
> - On the main thread, when parsing finishes.
> - For background and streaming compiles, where the source is not
> available while parsing, the locations are kept in the
> BackgroundCompileTask and the substrings are created in
> FinalizeScript after the source is set. Locations outside of the
> source provided by the embedder are ignored.
>
> The semantics are unchanged: the last comment wins, trailing garbage or
> an empty value removes the value, and a source map URL provided through
> the API takes precedence.
>
> Since these values are now usually SlicedStrings of the script source,
> the code serializer serializes all sliced strings as sequential
> strings (one-byte if their contents are, even if their parent is
> two-byte). The script source is an attached object, and off-thread
> deserialization attaches a placeholder for it, so a serialized slice
> of it would not be valid.
>
> Consuming a code cache still materializes these values as sequential
> strings, e.g. on Node.js's NODE_COMPILE_CACHE path, so a huge cached
> value can still be large on the heap there. Keeping them as substrings
> of the source when deserializing requires a code cache format change
> and is left for a follow-up.
>
> This also removes the LiteralBuffer copies of the values.
>
> R=leszeks@chromium.org, verwaest@chromium.org
>
> Bug: 566467883
> Refs: nodejs#64155
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8472768
> Reviewed-by: Leszek Swirski <leszeks@chromium.org>
> Reviewed-by: Patrick Thier <pthier@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#110612}
R=leszeks@chromium.org, verwaest@chromium.org, pthier@chromium.org
Bug: 566467883
Change-Id: I76eec1fdaf71526afdd881d0d4a2b117c32d3ba8
Refs: nodejs#64155
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8579167
Reviewed-by: Patrick Thier <pthier@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/main@{#111132}
Adapted to deps/v8: this also includes the upstream refactor
b0f6ad031f03 ("[parser] Factor magic comment value scanning into a
helper"), which the reland is based on. src/base/unique-array.h does
not exist yet, so base::OwnedVector is used in the serializer, the
serializer keeps the 15.6 external string handling (without the later
SBXCHECK hardening), and the CodeSerializer override is placed for the
older class layout. The inspector test is not included because
deps/v8 does not ship test/inspector.
Refs: v8/v8@5ba8aab
Refs: v8/v8@b0f6ad0
Signed-off-by: eliau2005 <eliau.elkouby@gmail.com>
Signed-off-by: eliau2005 <eliau.elkouby@gmail.com>
eliau2005
force-pushed
the
deps-v8-backport-8dcf14f490c2
branch
from
October 29, 2026 21:43
8d57314 to
e846ccb
Compare
eliau2005
changed the title
eliau2005
marked this pull request as ready for review
October 29, 2026 21:43
Contributor
Author
|
Updated to the upstream reland (v8/v8@5ba8aab1ec1b), rebased on main (V8 15.6). The adaptations are described in the first commit message. Locally the new worker test and the source map, worker, compile cache and inspector tests pass. This is ready for review. @nodejs/v8-update @richardlau @legendecas @joyeecheung, could someone start a Jenkins CI and a V8 CI run? Thanks! |
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.
Backports the V8 fix for a fatal OOM in Parser::HandleDebugMagicComments:
a large inline source map (
//# sourceMappingURL=data:...) no longeraborts the process when loaded in a worker with a small heap limit, because
the source map URL is now created as a substring of the script source
instead of being copied and internalized.
This is the reland (v8/v8@5ba8aab1ec1b). The original (8dcf14f490c2) was
reverted upstream because
sourceURLvalues became SlicedStrings and showedup as "(sliced string)" in heap snapshots; in the reland only
sourceMappingURLis a substring, whilesourceURLanddebugIdarecopied again.
The backport also includes the upstream refactor b0f6ad031f03, which the
reland is based on, and needed small adaptations for V8 15.6 (no
src/base/unique-array.h, older serializer and CodeSerializer code); see the
first commit message.
Note: as the upstream commit says, consuming a code cache
(NODE_COMPILE_CACHE) still creates these values as full strings on the
heap.
Fixes: #64155
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/8579167
Refs: https://issues.chromium.org/issues/566467883