Repository navigation
Conversation
Original commit message:
[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
Change-Id: I07d8d0c36f56c7aee40793b1db272657832ab36f
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>
Auto-Submit: eliau elkouby (אליהו אלקובי) <eliau.elkouby@gmail.com>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#110612}
Adapted to deps/v8: src/base/unique-array.h does not exist yet, so
base::OwnedVector is used in the serializer, and the CodeSerializer
override is placed for the older class layout.
Refs: v8/v8@8dcf14f
Assisted-by: a closed-source coding agent
Signed-off-by: eliau2005 <eliau.elkouby@gmail.com>
Assisted-by: a closed-source coding agent Signed-off-by: eliau2005 <eliau.elkouby@gmail.com>
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.47% 90.48% +0.01%
==========================================
Files 791 791
Lines 277245 277204 -41
Branches 53310 53318 +8
==========================================
- Hits 250834 250825 -9
+ Misses 16811 16792 -19
+ Partials 9600 9587 -13 🚀 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. |
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
2 participants
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:
the values of //# sourceURL, sourceMappingURL and debugId are now created
as substrings of the script source instead of being copied and
internalized, so a large inline source map no longer aborts the process
when loaded in a worker with a small heap limit.
The patch needed small adaptations for deps/v8 (no src/base/unique-array.h,
different CodeSerializer layout); 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/+/8472768
Refs: https://issues.chromium.org/issues/566467883