Skip to content

Commit 5ba8aab

Browse files
eliau2005v8-scoped@luci-project-accounts.iam.gserviceaccount.com
eliau2005
authored and
[email protected]
committed
Reland "[parser] Create debug magic comment values as source substrings"
This is a reland of commit 8dcf14f 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. > > [email protected], [email protected] > > Bug: 566467883 > Refs: nodejs/node#64155 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8472768 > Reviewed-by: Leszek Swirski <[email protected]> > Reviewed-by: Patrick Thier <[email protected]> > Cr-Commit-Position: refs/heads/main@{#110612} [email protected], [email protected], [email protected] Bug: 566467883 Change-Id: I76eec1fdaf71526afdd881d0d4a2b117c32d3ba8 Refs: nodejs/node#64155 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8579167 Reviewed-by: Patrick Thier <[email protected]> Commit-Queue: Patrick Thier <[email protected]> Reviewed-by: Toon Verwaest <[email protected]> Cr-Commit-Position: refs/heads/main@{#111132}
1 parent 05afdb1 commit 5ba8aab

17 files changed

Lines changed: 831 additions & 120 deletions

‎src/codegen/compiler.cc‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2043,6 +2043,9 @@ void BackgroundCompileTask::Run(
20432043
parser.ParseOnBackground(isolate, &info, script_, start_position_,
20442044
end_position_, function_literal_id_);
20452045
parser.UpdateStatistics(script_, &use_counts_, &total_preparse_skipped_);
2046+
if (toplevel_script_compilation) {
2047+
debug_magic_comment_locations_ = parser.debug_magic_comment_locations();
2048+
}
20462049

20472050
TRACE_EVENT(TRACE_DISABLED_BY_DEFAULT("v8.compile"),
20482051
"V8.CompileCodeBackground");
@@ -2786,8 +2789,14 @@ MaybeHandle<SharedFunctionInfo> BackgroundCompileTask::FinalizeScript(
27862789
{
27872790
DisallowGarbageCollection no_gc;
27882791
SetScriptFieldsFromDetails(isolate, *script, script_details, &no_gc);
2789-
LOG(isolate, ScriptDetails(*script));
27902792
}
2793+
// After SetScriptFieldsFromDetails, so that a source map URL provided
2794+
// through the API takes precedence. The locations come from the streamed
2795+
// source; the embedder is expected to pass the same source here, but an
2796+
// out-of-bounds location is ignored.
2797+
Parser::SetDebugMagicCommentFields(isolate, script, source,
2798+
debug_magic_comment_locations_);
2799+
LOG(isolate, ScriptDetails(*script));
27912800
}
27922801

27932802
ReportStatistics(isolate);

‎src/codegen/compiler.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include "src/objects/debug-objects.h"
2525
#include "src/parsing/parse-info.h"
2626
#include "src/parsing/pending-compilation-error-handler.h"
27+
#include "src/parsing/scanner.h" // For Scanner::DebugMagicCommentLocations.
2728
#include "src/snapshot/code-serializer.h"
2829
#include "src/utils/allocation.h"
2930
#include "src/zone/zone.h"
@@ -653,6 +654,9 @@ class V8_EXPORT_PRIVATE BackgroundCompileTask {
653654
DeferredFinalizationJobDataList jobs_to_retry_finalization_on_main_thread_;
654655
base::SmallVector<v8::Isolate::UseCounterFeature, 8> use_counts_;
655656
int total_preparse_skipped_ = 0;
657+
// The source is not available on the background thread, so the debug magic
658+
// comment values are created from these locations when finalizing.
659+
Scanner::DebugMagicCommentLocations debug_magic_comment_locations_;
656660

657661
// Single function data for top-level function compilation.
658662
MaybeIndirectHandle<SharedFunctionInfo> input_shared_info_;

‎src/parsing/literal-buffer.cc‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -6,29 +6,11 @@
66

77
#include "src/base/strings.h"
88
#include "src/base/unique-array.h"
9-
#include "src/execution/isolate.h"
10-
#include "src/execution/local-isolate.h"
11-
#include "src/heap/factory.h"
12-
#include "src/objects/objects.h"
13-
#include "src/objects/string.h"
149
#include "src/utils/memcopy.h"
1510

1611
namespace v8 {
1712
namespace internal {
1813

19-
template <typename IsolateT>
20-
DirectHandle<String> LiteralBuffer::Internalize(IsolateT* isolate) const {
21-
if (is_one_byte()) {
22-
return isolate->factory()->InternalizeString(one_byte_literal());
23-
}
24-
return isolate->factory()->InternalizeString(two_byte_literal());
25-
}
26-
27-
template DirectHandle<String> LiteralBuffer::Internalize(
28-
Isolate* isolate) const;
29-
template DirectHandle<String> LiteralBuffer::Internalize(
30-
LocalIsolate* isolate) const;
31-
3214
size_t LiteralBuffer::NewCapacity(size_t min_capacity) {
3315
return min_capacity < (kMaxGrowth / (kGrowthFactor - 1))
3416
? min_capacity * kGrowthFactor

‎src/parsing/literal-buffer.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,6 @@ class LiteralBuffer final {
9999
is_one_byte_ = true;
100100
}
101101

102-
template <typename IsolateT>
103-
DirectHandle<String> Internalize(IsolateT* isolate) const;
104-
105102
private:
106103
static constexpr size_t kInitialCapacity = 256;
107104
static constexpr size_t kGrowthFactor = 4;

‎src/parsing/parser.cc‎

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "src/common/message-template.h"
2121
#include "src/common/scoped-modification.h"
2222
#include "src/compiler-dispatcher/lazy-compile-dispatcher.h"
23+
#include "src/heap/factory-inl.h"
2324
#include "src/heap/local-heap-inl.h"
2425
#include "src/heap/parked-scope.h"
2526
#include "src/logging/counters.h"
@@ -3590,32 +3591,50 @@ void Parser::InsertSloppyBlockFunctionVarBindings(DeclarationScope* scope) {
35903591
// ----------------------------------------------------------------------------
35913592
// Parser support
35923593

3593-
template <typename IsolateT>
3594-
void Parser::HandleDebugMagicComments(IsolateT* isolate,
3594+
void Parser::HandleDebugMagicComments(Isolate* isolate,
35953595
DirectHandle<Script> script) {
3596-
DirectHandle<String> source_url = scanner_.SourceUrl(isolate);
3597-
if (!source_url.is_null()) {
3596+
// On the main thread, the scanned source is the script source.
3597+
DirectHandle<String> source(Cast<String>(script->source()), isolate);
3598+
SetDebugMagicCommentFields(isolate, script, source,
3599+
scanner_.debug_magic_comment_locations());
3600+
}
3601+
3602+
// static
3603+
void Parser::SetDebugMagicCommentFields(
3604+
Isolate* isolate, DirectHandle<Script> script, DirectHandle<String> source,
3605+
const Scanner::DebugMagicCommentLocations& locations) {
3606+
source = String::Flatten(isolate, source);
3607+
auto is_valid = [&](Scanner::Location location) {
3608+
return location.IsValid() && location.length() != 0 &&
3609+
static_cast<uint32_t>(location.end_pos) <= source->length();
3610+
};
3611+
if (is_valid(locations.source_url)) {
3612+
// The source URL is used as the script name (e.g. in stack traces,
3613+
// profiles and heap snapshots), so it is a sequential copy rather than a
3614+
// substring of the source.
3615+
DirectHandle<String> source_url = isolate->factory()->NewCopiedSubstring(
3616+
source, locations.source_url.beg_pos, locations.source_url.length());
35983617
script->set_source_url(*source_url);
35993618
}
3600-
DirectHandle<String> source_mapping_url = scanner_.SourceMappingUrl(isolate);
36013619
// The API can provide a source map URL and the API should take precedence.
36023620
// Let's make sure we do not override the API with the magic comment.
3603-
if (!source_mapping_url.is_null() &&
3604-
IsUndefined(script->source_mapping_url())) {
3621+
if (IsUndefined(script->source_mapping_url()) &&
3622+
is_valid(locations.source_mapping_url)) {
3623+
// The source map URL can be arbitrarily large (e.g. an inline source map),
3624+
// so it is a substring of the source rather than a copy.
3625+
DirectHandle<String> source_mapping_url = isolate->factory()->NewSubString(
3626+
source, locations.source_mapping_url.beg_pos,
3627+
locations.source_mapping_url.end_pos);
36053628
script->set_source_mapping_url(*source_mapping_url);
36063629
}
3607-
3608-
DirectHandle<String> debug_id = scanner_.DebugId(isolate);
3609-
if (!debug_id.is_null()) {
3630+
if (is_valid(locations.debug_id)) {
3631+
// Debug IDs are short, so they are copied like the source URL.
3632+
DirectHandle<String> debug_id = isolate->factory()->NewCopiedSubstring(
3633+
source, locations.debug_id.beg_pos, locations.debug_id.length());
36103634
script->set_debug_id(*debug_id);
36113635
}
36123636
}
36133637

3614-
template void Parser::HandleDebugMagicComments(Isolate* isolate,
3615-
DirectHandle<Script> script);
3616-
template void Parser::HandleDebugMagicComments(LocalIsolate* isolate,
3617-
DirectHandle<Script> script);
3618-
36193638
void Parser::UpdateStatistics(Isolate* isolate, DirectHandle<Script> script) {
36203639
CHECK_NOT_NULL(isolate);
36213640

@@ -3720,9 +3739,6 @@ void Parser::ParseOnBackground(LocalIsolate* isolate, ParseInfo* info,
37203739
MaybeProcessSourceRanges(info, result, stack_limit_);
37213740
});
37223741
// We need to unpark by now though, to be able to internalize.
3723-
if (flags().is_toplevel()) {
3724-
HandleDebugMagicComments(isolate, script);
3725-
}
37263742
if (result == nullptr) return;
37273743
PostProcessParseResult(isolate, info, result);
37283744
}

‎src/parsing/parser.h‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,8 +170,19 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
170170
DirectHandle<Script> script,
171171
base::SmallVector<v8::Isolate::UseCounterFeature, 8>* use_counters,
172172
int* preparse_skipped);
173-
template <typename IsolateT>
174-
void HandleDebugMagicComments(IsolateT* isolate, DirectHandle<Script> script);
173+
void HandleDebugMagicComments(Isolate* isolate, DirectHandle<Script> script);
174+
const Scanner::DebugMagicCommentLocations& debug_magic_comment_locations()
175+
const {
176+
return scanner_.debug_magic_comment_locations();
177+
}
178+
// Sets the debug magic comment fields of {script} to the values in {source}
179+
// at the given locations: the source map URL is a substring of {source}, the
180+
// source URL and debug ID are sequential copies. Locations that are empty,
181+
// invalid or out of bounds are ignored.
182+
static void SetDebugMagicCommentFields(
183+
Isolate* isolate, DirectHandle<Script> script,
184+
DirectHandle<String> source,
185+
const Scanner::DebugMagicCommentLocations& locations);
175186

176187
private:
177188
friend class ParserBase<Parser>;

‎src/parsing/scanner.cc‎

Lines changed: 24 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -314,12 +314,14 @@ void Scanner::TryToParseMagicComment(base::uc32 hash_or_at_sign) {
314314
}
315315
if (!name.is_one_byte()) return;
316316
base::Vector<const uint8_t> name_literal = name.one_byte_literal();
317-
LiteralBuffer* value = nullptr;
318-
LiteralBuffer per_function_compile_hints_value;
317+
// The debug magic comment values are recorded as source locations, so that
318+
// they can be created as substrings of the source instead of being copied.
319+
Location* value_location = nullptr;
320+
bool is_compile_hints = false;
319321
if (name_literal == base::StaticOneByteVector("sourceURL")) {
320-
value = &source_url_;
322+
value_location = &debug_magic_comment_locations_.source_url;
321323
} else if (name_literal == base::StaticOneByteVector("sourceMappingURL")) {
322-
value = &source_mapping_url_;
324+
value_location = &debug_magic_comment_locations_.source_mapping_url;
323325
DCHECK(hash_or_at_sign == '#' || hash_or_at_sign == '@');
324326
saw_source_mapping_url_magic_comment_at_sign_ = hash_or_at_sign == '@';
325327
} else if (!saw_non_comment_ &&
@@ -330,30 +332,33 @@ void Scanner::TryToParseMagicComment(base::uc32 hash_or_at_sign) {
330332
} else if (name_literal ==
331333
base::StaticOneByteVector("functionsCalledOnLoad") &&
332334
hash_or_at_sign == '#') {
333-
value = &per_function_compile_hints_value;
335+
is_compile_hints = true;
334336
} else if (name_literal == base::StaticOneByteVector("debugId") &&
335337
hash_or_at_sign == '#') {
336-
value = &debug_id_;
338+
value_location = &debug_magic_comment_locations_.debug_id;
337339
} else {
338340
return;
339341
}
340342
if (c0_ != '=') {
341343
return;
342344
}
343-
value->Start();
344-
if (!ScanMagicCommentValue([value](base::uc32 c) { value->AddChar(c); })
345-
.IsValid()) {
346-
value->Start();
347-
}
348-
if (value == &per_function_compile_hints_value &&
349-
per_function_compile_hints_value.is_one_byte()) {
350-
base::Vector<const uint8_t> value_literal =
351-
per_function_compile_hints_value.one_byte_literal();
352-
per_function_compile_hint_positions_.clear();
353-
per_function_compile_hint_positions_idx_ = 0;
354-
ProcessPerFunctionCompileHints(value_literal, source_pos(),
355-
per_function_compile_hint_positions_);
345+
if (!is_compile_hints) {
346+
DCHECK_NOT_NULL(value_location);
347+
*value_location = ScanMagicCommentValue([](base::uc32) {});
348+
return;
356349
}
350+
LiteralBuffer value;
351+
value.Start();
352+
Location value_location_in_source =
353+
ScanMagicCommentValue([&value](base::uc32 c) { value.AddChar(c); });
354+
// A value followed by trailing garbage is treated as empty, which clears
355+
// the previously recorded compile hints.
356+
if (!value_location_in_source.IsValid()) value.Start();
357+
if (!value.is_one_byte()) return;
358+
per_function_compile_hint_positions_.clear();
359+
per_function_compile_hint_positions_idx_ = 0;
360+
ProcessPerFunctionCompileHints(value.one_byte_literal(), source_pos(),
361+
per_function_compile_hint_positions_);
357362
}
358363

359364
bool Scanner::HasPerFunctionCompileHint(int position) {
@@ -695,43 +700,6 @@ Token::Value Scanner::ScanTemplateSpan() {
695700
return result;
696701
}
697702

698-
template <typename IsolateT>
699-
DirectHandle<String> Scanner::SourceUrl(IsolateT* isolate) const {
700-
DirectHandle<String> tmp;
701-
if (source_url_.length() > 0) {
702-
tmp = source_url_.Internalize(isolate);
703-
}
704-
return tmp;
705-
}
706-
707-
template DirectHandle<String> Scanner::SourceUrl(Isolate* isolate) const;
708-
template DirectHandle<String> Scanner::SourceUrl(LocalIsolate* isolate) const;
709-
710-
template <typename IsolateT>
711-
DirectHandle<String> Scanner::SourceMappingUrl(IsolateT* isolate) const {
712-
DirectHandle<String> tmp;
713-
if (source_mapping_url_.length() > 0) {
714-
tmp = source_mapping_url_.Internalize(isolate);
715-
}
716-
return tmp;
717-
}
718-
719-
template DirectHandle<String> Scanner::SourceMappingUrl(Isolate* isolate) const;
720-
template DirectHandle<String> Scanner::SourceMappingUrl(
721-
LocalIsolate* isolate) const;
722-
723-
template <typename IsolateT>
724-
DirectHandle<String> Scanner::DebugId(IsolateT* isolate) const {
725-
DirectHandle<String> tmp;
726-
if (debug_id_.length() > 0) {
727-
tmp = debug_id_.Internalize(isolate);
728-
}
729-
return tmp;
730-
}
731-
732-
template DirectHandle<String> Scanner::DebugId(Isolate* isolate) const;
733-
template DirectHandle<String> Scanner::DebugId(LocalIsolate* isolate) const;
734-
735703
bool Scanner::ScanDigitsWithNumericSeparators(bool (*predicate)(base::uc32 ch),
736704
bool is_check_first_digit) {
737705
// we must have at least one digit after 'x'/'b'/'o'

‎src/parsing/scanner.h‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -464,12 +464,18 @@ class V8_EXPORT_PRIVATE Scanner {
464464
return ScanTemplateSpan();
465465
}
466466

467-
template <typename IsolateT>
468-
DirectHandle<String> SourceUrl(IsolateT* isolate) const;
469-
template <typename IsolateT>
470-
DirectHandle<String> SourceMappingUrl(IsolateT* isolate) const;
471-
template <typename IsolateT>
472-
DirectHandle<String> DebugId(IsolateT* isolate) const;
467+
// Source locations of the values of the debug magic comments. A location is
468+
// invalid if there was no such comment, or if the last one was malformed; it
469+
// is empty if the last one had an empty value. In both cases there is no
470+
// value.
471+
struct DebugMagicCommentLocations {
472+
Location source_url = Location::invalid();
473+
Location source_mapping_url = Location::invalid();
474+
Location debug_id = Location::invalid();
475+
};
476+
const DebugMagicCommentLocations& debug_magic_comment_locations() const {
477+
return debug_magic_comment_locations_;
478+
}
473479

474480
bool SawSourceMappingUrlMagicCommentAtSign() const {
475481
return saw_source_mapping_url_magic_comment_at_sign_;
@@ -813,10 +819,7 @@ class V8_EXPORT_PRIVATE Scanner {
813819
// Whether this scanner encountered an HTML comment.
814820
bool found_html_comment_;
815821

816-
// Values parsed from magic comments.
817-
LiteralBuffer source_url_;
818-
LiteralBuffer source_mapping_url_;
819-
LiteralBuffer debug_id_;
822+
DebugMagicCommentLocations debug_magic_comment_locations_;
820823
bool saw_source_mapping_url_magic_comment_at_sign_ = false;
821824
bool saw_magic_comment_compile_hints_all_ = false;
822825
bool saw_non_comment_ = false;

‎src/snapshot/code-serializer.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,12 @@ class CodeSerializer : public Serializer {
243243
OptimizeBackReferences();
244244
Pad();
245245
}
246+
// Slices usually refer to the script source (e.g. debug magic comment
247+
// values), which is an attached object: off-thread deserialization attaches
248+
// a placeholder instead of the source.
249+
bool SerializeSlicedStringsAsSequentialStrings() const override {
250+
return true;
251+
}
246252

247253
DISALLOW_GARBAGE_COLLECTION(no_gc_)
248254
SourceHash source_hash_;

0 commit comments

Comments
 (0)