Refactor console method decoration for snapshotting - #6874
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
a2bf6f9 to
e33db21
Compare
e33db21 to
3c8e057
Compare
Merging this PR will regress 1 benchmark
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | request[RegExpBenchmark] |
4.1 ms | 4.5 ms | -8.95% |
| ⚡ | bm_Promise_Fib10 |
20.1 µs | 18.3 µs | +10.22% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing dbezhetskov:live-snapshots-1 (3c8e057) with main (cd1f881)
Footnotes
-
129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
| {"error", LogLevel::ERROR, &consoleDecorator<1>}, | ||
| {"info", LogLevel::INFO, &consoleDecorator<2>}, | ||
| {"log", LogLevel::LOG, &consoleDecorator<3>}, | ||
| {"warn", LogLevel::WARN, &consoleDecorator<4>}, |
There was a problem hiding this comment.
Simply because it's a bit opaque here, either making the numbers here named constants or adding some comments here would be useful.
| // V2 (Local<Data>) embedder data slots. Read/written via | ||
| // SetEmbedderDataV2/GetEmbedderDataV2 — distinct API from the aligned-pointer | ||
| // slots above. Indices start past MAX_POINTER_SLOT so the two slot kinds never | ||
| // collide in the underlying embedder-data array. |
There was a problem hiding this comment.
Just so it's clear, some guidance about what belongs in the aligned-pointer slots vs. the context data slots here would be helpful (like a single quick sentence)
| // Save original methods into context so that GC can track them (so they are captured in a | ||
| // snapshot) and we can find them at a known location from the decorator. | ||
| for (size_t i = 0; i < kConsoleMethodsCount; ++i) { | ||
| auto methodStr = jsg::v8StrIntern(lock.v8Isolate, kConsoleMethods[i].name); |
There was a problem hiding this comment.
lock.strIntern(...) ... slightly less verbose
| auto methodStr = jsg::v8StrIntern(lock.v8Isolate, kConsoleMethods[i].name); | ||
| auto original = jsg::check(console->Get(context, methodStr)).As<v8::Function>(); | ||
| auto slot = static_cast<jsg::ContextDataSlot>( | ||
| static_cast<int>(jsg::ContextDataSlot::CONSOLE_ORIGINAL_DEBUG) + static_cast<int>(i)); |
There was a problem hiding this comment.
may as well make getting the jsg::ContextData enum value a utility method given the repeats.
jasnell
left a comment
jasnell
left a comment
There was a problem hiding this comment.
couple of doc nits but otherwise LGTM
3c8e057 to
29923cb
Compare
The current console decoration scheme breaks V8 snapshot creation in two ways: 1) The C++ lambdas used as decorators hold v8::Global handles to the original console methods, but V8 does not support serializing Global handles. 2) The decorator for each console method is created through wrapSimpleFunction, which allocates an opaque JS wrapper using the Wrappable path and binds that wrapper to the resulting V8 function via V8::Function::New. The JS wrapper, which holds the C++ lambda, belongs to the context. However, V8 snapshot forbids pointers from isolate-level snapshot objects to context-level data. In this case, the function/template machinery belongs to the isolate-level snapshot, while the wrapper belongs to the context-level snapshot. This patch addresses both issues: - We preserve the original methods in a private, context-level embedder-data slots instead of holding them in v8::Global handles. This establishes edges from the context to those JS objects, so they remain reachable and are automatically captured in the snapshot as part of the context graph. - We no longer use the Wrappable path for decoration. Instead, we create the V8 functions directly with V8::Function::New and use a plain C++ functor that retrieves the required state from the context-level embedder-data slot. This avoids creating JS wrappers that hold references to C++ state.
29923cb to
c6394cd
Compare
|
Thanks for the review, @jasnell ! |
3 participants
The current console decoration scheme breaks V8 snapshot creation in two ways:
The C++ lambdas used as decorators hold v8::Global handles to the
original console methods, but V8 does not support serializing Global
handles.
The decorator for each console method is created through
wrapSimpleFunction, which allocates an opaque JS wrapper using the
Wrappable path and binds that wrapper to the resulting V8 function
via V8::Function::New. The JS wrapper, which holds the C++ lambda,
belongs to the context. However, V8 snapshot forbids pointers from
isolate-level snapshot objects to context-level data. In this case,
the function/template machinery belongs to the isolate-level snapshot,
while the wrapper belongs to the context-level snapshot.
This patch addresses both issues:
We preserve the original methods in a private, context-level embedder-data slots instead of holding them in v8::Global handles. This establishes edges from the context to those JS objects, so they remain reachable and are automatically captured in the snapshot as part of the context graph.
We no longer use the Wrappable path for decoration. Instead, we create the V8 functions directly with V8::Function::New and use a plain C++ functor that retrieves the required state from the context-level embedder-data slot. This avoids creating JS wrappers that hold references to C++ state.