Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: implement minimal v8 snapshot integration #27321

Closed
wants to merge 10 commits into from
Closed

src: implement minimal v8 snapshot integration #27321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
076c31f
src: allow creating NodeMainInstance that does not own the isolate
joyeecheung Apr 19, 2019
e354795
src: implement IsolateData serialization and deserialization
joyeecheung Apr 18, 2019
de02be6
tools: implement node_mksnapshot
joyeecheung Apr 18, 2019
52c5c05
src: enable snapshot with per-isolate data
joyeecheung Apr 18, 2019
8248be6
src: enable context snapshot after running per-context scripts
joyeecheung Apr 20, 2019
3055cb1
fixup! tools: implement node_mksnapshot
joyeecheung Apr 21, 2019
2ac1d81
src: use std::vector<size_t> instead of IndexArray
joyeecheung Apr 21, 2019
1e0210b
fixup! src: enable context snapshot after running per-context scripts
joyeecheung Apr 22, 2019
0324fe3
fixup! src: allow creating NodeMainInstance that does not own the iso…
joyeecheung Apr 22, 2019
90b5d8d
fixup! tools: implement node_mksnapshot
joyeecheung Apr 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,8 @@ LINT_CPP_FILES = $(filter-out $(LINT_CPP_EXCLUDE), $(wildcard \
tools/icu/*.h \
tools/code_cache/*.cc \
tools/code_cache/*.h \
tools/snapshot/*.cc \
tools/snapshot/*.h \
))

# Code blocks don't have newline at the end,
Expand Down
71 changes: 70 additions & 1 deletion node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@
'deps/acorn/acorn/dist/acorn.js',
'deps/acorn/acorn-walk/dist/walk.js',
],
'node_mksnapshot_exec': '<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)node_mksnapshot<(EXECUTABLE_SUFFIX)',
'mkcodecache_exec': '<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)mkcodecache<(EXECUTABLE_SUFFIX)',
'conditions': [
[ 'node_shared=="true"', {
Expand Down Expand Up @@ -430,6 +431,31 @@
'src/node_code_cache_stub.cc'
],
}],
['want_separate_host_toolset==0', {
'dependencies': [
'node_mksnapshot',
],
'actions': [
{
'action_name': 'node_mksnapshot',
'process_outputs_as_sources': 1,
'inputs': [
'<(node_mksnapshot_exec)',
],
'outputs': [
'<(SHARED_INTERMEDIATE_DIR)/node_snapshot.cc',
],
'action': [
'<@(_inputs)',
'<@(_outputs)',
],
},
],
}, {
'sources': [
'src/node_snapshot_stub.cc'
],
}],
],
}, # node_core_target_name
{
Expand Down Expand Up @@ -1039,6 +1065,7 @@
'defines': [ 'NODE_WANT_INTERNALS=1' ],

'sources': [
'src/node_snapshot_stub.cc',
'src/node_code_cache_stub.cc',
'test/cctest/node_test_fixture.cc',
'test/cctest/test_aliased_buffer.cc',
Expand Down Expand Up @@ -1129,6 +1156,7 @@
'NODE_WANT_INTERNALS=1'
],
'sources': [
'src/node_snapshot_stub.cc',
'src/node_code_cache_stub.cc',
'tools/code_cache/mkcodecache.cc',
'tools/code_cache/cache_builder.cc',
Expand All @@ -1154,7 +1182,48 @@
}],
],
}, # mkcodecache
], # end targets
{
'target_name': 'node_mksnapshot',
'type': 'executable',

'dependencies': [
'<(node_lib_target_name)',
'deps/histogram/histogram.gyp:histogram',
],

'includes': [
'node.gypi'
],

'include_dirs': [
'src',
'tools/msvs/genfiles',
'deps/v8/include',
'deps/cares/include',
'deps/uv/include',
],

'defines': [ 'NODE_WANT_INTERNALS=1' ],

'sources': [
'src/node_snapshot_stub.cc',
'src/node_code_cache_stub.cc',
'tools/snapshot/node_mksnapshot.cc',
'tools/snapshot/snapshot_builder.cc',
joyeecheung marked this conversation as resolved.
Show resolved Hide resolved
'tools/snapshot/snapshot_builder.h',
],

'conditions': [
[ 'node_report=="true"', {
'conditions': [
['OS=="win"', {
'libraries': [ 'Ws2_32' ],
}],
],
}],
],
}, # node_mksnapshot
], # end targets

'conditions': [
['OS=="aix" and node_shared=="true"', {
Expand Down
36 changes: 26 additions & 10 deletions src/api/environment.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,17 +183,33 @@ void SetIsolateCreateParamsForNode(Isolate::CreateParams* params) {
#endif
}

void SetIsolateUpForNode(v8::Isolate* isolate, IsolateSettingCategories cat) {
switch (cat) {
case IsolateSettingCategories::kErrorHandlers:
isolate->AddMessageListenerWithErrorLevel(
OnMessage,
Isolate::MessageErrorLevel::kMessageError |
Isolate::MessageErrorLevel::kMessageWarning);
isolate->SetAbortOnUncaughtExceptionCallback(
ShouldAbortOnUncaughtException);
isolate->SetFatalErrorHandler(OnFatalError);
break;
case IsolateSettingCategories::kMisc:
isolate->SetMicrotasksPolicy(MicrotasksPolicy::kExplicit);
isolate->SetAllowWasmCodeGenerationCallback(
AllowWasmCodeGenerationCallback);
isolate->SetPromiseRejectCallback(task_queue::PromiseRejectCallback);
v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);
break;
default:
UNREACHABLE();
break;
}
}

void SetIsolateUpForNode(v8::Isolate* isolate) {
isolate->AddMessageListenerWithErrorLevel(
OnMessage,
Isolate::MessageErrorLevel::kMessageError |
Isolate::MessageErrorLevel::kMessageWarning);
isolate->SetAbortOnUncaughtExceptionCallback(ShouldAbortOnUncaughtException);
isolate->SetMicrotasksPolicy(MicrotasksPolicy::kExplicit);
isolate->SetFatalErrorHandler(OnFatalError);
isolate->SetAllowWasmCodeGenerationCallback(AllowWasmCodeGenerationCallback);
isolate->SetPromiseRejectCallback(task_queue::PromiseRejectCallback);
v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);
SetIsolateUpForNode(isolate, IsolateSettingCategories::kErrorHandlers);
SetIsolateUpForNode(isolate, IsolateSettingCategories::kMisc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of a bad API (although it's not a public API so it matters less) in that order might be important but that's easy to get wrong for callers. A single call where you pass in an options bitmask would be more robust.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the APIs that are called here...I don't think the order actually matters for now? The only dangerous bit is that you could call with one category twice, but that's also true before this change.

I think the current categories is not good as final for sure, but I don't really know the use cases for this other than this patch, so we might as well polish it as we add more use cases (I tried to organize the setup a bit better in another cctest attempt before but that didn't really go anywhere).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the APIs that are called here...I don't think the order actually matters for now?

No, but as a caller I don't know that unless I (closely) read the function's source. It would just be a bit easier all around if I could just say "do this and that" without having to worry about order. Declarative vs. imperative if you will.

Copy link
Member Author

@joyeecheung joyeecheung Apr 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but as a caller I don't know that unless I (closely) read the function's source. It would just be a bit easier all around if I could just say "do this and that" without having to worry about order. Declarative vs. imperative if you will.

This is more like a temporary DRY thing here, when we figure out other use cases we could just tweak it for them, but it’s probably too early to spend time designing it. In the initial prototype I actually copy pasted this into SetIsolateUpForSnapshot() but it looked worse..

}

Isolate* NewIsolate(ArrayBufferAllocator* allocator, uv_loop_t* event_loop) {
Expand Down
146 changes: 103 additions & 43 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ using v8::NewStringType;
using v8::Number;
using v8::Object;
using v8::Private;
using v8::SnapshotCreator;
using v8::StackTrace;
using v8::String;
using v8::Symbol;
Expand All @@ -49,22 +50,57 @@ int const Environment::kNodeContextTag = 0x6e6f64;
void* const Environment::kNodeContextTagPtr = const_cast<void*>(
static_cast<const void*>(&Environment::kNodeContextTag));

IsolateData::IsolateData(Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform,
ArrayBufferAllocator* node_allocator)
: isolate_(isolate),
event_loop_(event_loop),
allocator_(isolate->GetArrayBufferAllocator()),
node_allocator_(node_allocator == nullptr ?
nullptr : node_allocator->GetImpl()),
uses_node_allocator_(allocator_ == node_allocator_),
platform_(platform) {
CHECK_NOT_NULL(allocator_);
std::vector<size_t> IsolateData::Serialize(SnapshotCreator* creator) {
Isolate* isolate = creator->GetIsolate();
std::vector<size_t> indexes;
HandleScope handle_scope(isolate);
// XXX(joyeecheung): technically speaking, the indexes here should be
// consecutive and we could just return a range instead of an array,
// but that's not part of the V8 API contract so we use an array
// just to be safe.

#define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
#define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
#define VS(PropertyName, StringValue) V(v8::String, PropertyName)
#define V(TypeName, PropertyName) \
indexes.push_back(creator->AddData(PropertyName##_.Get(isolate)));
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
PER_ISOLATE_SYMBOL_PROPERTIES(VY)
PER_ISOLATE_STRING_PROPERTIES(VS)
#undef V
#undef VY
#undef VS
#undef VP

options_.reset(
new PerIsolateOptions(*(per_process::cli_options->per_isolate)));
return indexes;
}

void IsolateData::DeserializeProperties(const std::vector<size_t>* indexes) {
size_t i = 0;
HandleScope handle_scope(isolate_);

#define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
#define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
#define VS(PropertyName, StringValue) V(v8::String, PropertyName)
#define V(TypeName, PropertyName) \
do { \
MaybeLocal<TypeName> field = \
isolate_->GetDataFromSnapshotOnce<TypeName>((*indexes)[i++]); \
if (field.IsEmpty()) { \
fprintf(stderr, "Failed to deserialize " #PropertyName "\n"); \
} \
PropertyName##_.Set(isolate_, field.ToLocalChecked()); \
} while (0);
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
PER_ISOLATE_SYMBOL_PROPERTIES(VY)
PER_ISOLATE_STRING_PROPERTIES(VS)
#undef V
#undef VY
#undef VS
#undef VP
}

void IsolateData::CreateProperties() {
// Create string and private symbol properties as internalized one byte
// strings after the platform is properly initialized.
//
Expand All @@ -76,44 +112,68 @@ IsolateData::IsolateData(Isolate* isolate,
// One byte because our strings are ASCII and we can safely skip V8's UTF-8
// decoding step.

HandleScope handle_scope(isolate);
HandleScope handle_scope(isolate_);

#define V(PropertyName, StringValue) \
PropertyName ## _.Set( \
isolate, \
Private::New( \
isolate, \
String::NewFromOneByte( \
isolate, \
reinterpret_cast<const uint8_t*>(StringValue), \
NewStringType::kInternalized, \
sizeof(StringValue) - 1).ToLocalChecked()));
#define V(PropertyName, StringValue) \
PropertyName##_.Set( \
isolate_, \
Private::New(isolate_, \
String::NewFromOneByte( \
isolate_, \
reinterpret_cast<const uint8_t*>(StringValue), \
NewStringType::kInternalized, \
sizeof(StringValue) - 1) \
.ToLocalChecked()));
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V)
#undef V
#define V(PropertyName, StringValue) \
PropertyName ## _.Set( \
isolate, \
Symbol::New( \
isolate, \
String::NewFromOneByte( \
isolate, \
reinterpret_cast<const uint8_t*>(StringValue), \
NewStringType::kInternalized, \
sizeof(StringValue) - 1).ToLocalChecked()));
#define V(PropertyName, StringValue) \
PropertyName##_.Set( \
isolate_, \
Symbol::New(isolate_, \
String::NewFromOneByte( \
isolate_, \
reinterpret_cast<const uint8_t*>(StringValue), \
NewStringType::kInternalized, \
sizeof(StringValue) - 1) \
.ToLocalChecked()));
PER_ISOLATE_SYMBOL_PROPERTIES(V)
#undef V
#define V(PropertyName, StringValue) \
PropertyName ## _.Set( \
isolate, \
String::NewFromOneByte( \
isolate, \
reinterpret_cast<const uint8_t*>(StringValue), \
NewStringType::kInternalized, \
sizeof(StringValue) - 1).ToLocalChecked());
#define V(PropertyName, StringValue) \
PropertyName##_.Set( \
isolate_, \
String::NewFromOneByte(isolate_, \
reinterpret_cast<const uint8_t*>(StringValue), \
NewStringType::kInternalized, \
sizeof(StringValue) - 1) \
.ToLocalChecked());
PER_ISOLATE_STRING_PROPERTIES(V)
#undef V
}

IsolateData::IsolateData(Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform,
ArrayBufferAllocator* node_allocator,
const std::vector<size_t>* indexes)
: isolate_(isolate),
event_loop_(event_loop),
allocator_(isolate->GetArrayBufferAllocator()),
node_allocator_(node_allocator == nullptr ? nullptr
: node_allocator->GetImpl()),
uses_node_allocator_(allocator_ == node_allocator_),
platform_(platform) {
CHECK_NOT_NULL(allocator_);

options_.reset(
new PerIsolateOptions(*(per_process::cli_options->per_isolate)));

if (indexes == nullptr) {
CreateProperties();
} else {
DeserializeProperties(indexes);
}
}

void IsolateData::MemoryInfo(MemoryTracker* tracker) const {
#define V(PropertyName, StringValue) \
tracker->TrackField(#PropertyName, PropertyName(isolate()));
Expand Down
8 changes: 7 additions & 1 deletion src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "node.h"
#include "node_binding.h"
#include "node_http2_state.h"
#include "node_main_instance.h"
#include "node_options.h"
#include "req_wrap.h"
#include "util.h"
Expand Down Expand Up @@ -418,10 +419,12 @@ class IsolateData : public MemoryRetainer {
IsolateData(v8::Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform = nullptr,
ArrayBufferAllocator* node_allocator = nullptr);
ArrayBufferAllocator* node_allocator = nullptr,
const std::vector<size_t>* indexes = nullptr);
SET_MEMORY_INFO_NAME(IsolateData);
SET_SELF_SIZE(IsolateData);
void MemoryInfo(MemoryTracker* tracker) const override;
std::vector<size_t> Serialize(v8::SnapshotCreator* creator);

inline uv_loop_t* event_loop() const;
inline MultiIsolatePlatform* platform() const;
Expand Down Expand Up @@ -451,6 +454,9 @@ class IsolateData : public MemoryRetainer {
IsolateData& operator=(const IsolateData&) = delete;

private:
void DeserializeProperties(const std::vector<size_t>* indexes);
void CreateProperties();

#define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
#define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
#define VS(PropertyName, StringValue) V(v8::String, PropertyName)
Expand Down
21 changes: 19 additions & 2 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,8 +885,25 @@ int Start(int argc, char** argv) {
}

{
NodeMainInstance main_instance(
uv_default_loop(), result.args, result.exec_args);
Isolate::CreateParams params;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not move this bit into the NodeMainInstance::main_instance overload? Sine 2 out of 3 params are anyway calculated using NodeMainInstance:: static methods, and the third is {nullptr};
i.e. something like:

    NodeMainInstance main_instance({nullptr},
                                   uv_default_loop(),
                                   per_process::v8_platform.Platform(),
                                   result.args,
                                   result.exec_args);
...
    NodeMainInstance main_instance(const std::vector<intptr_t> external_references&&,
                                   uv_loop_t* event_loop,
                                   MultiIsolatePlatform* platform,
                                   const std::vector<std::string>& args,
                                   const std::vector<std::string>& exec_args);
      Isolate::CreateParams params;
      v8::StartupData* blob = NodeMainInstance::GetEmbeddedSnapshotBlob();
      const NodeMainInstance::IndexArray* indexes =
          NodeMainInstance::GetIsolateDataIndexes();
      if (blob != nullptr) {
        params.external_references = external_references.data();
        params.snapshot_blob = blob;
      }
      ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about moving the GetEmbeddedSnapshotBlob() etc. out of NodeMainInstance at some point when we add support for other snapshots, e.g. workers, and maybe restructuring the class somehow so we could reuse it in the cctest, it would be easier to experiment if the deserialization can be switched off by the caller even when the binary is built with embedded snapshot.

// TODO(joyeecheung): collect external references and set it in
// params.external_references.
std::vector<intptr_t> external_references = {
reinterpret_cast<intptr_t>(nullptr)};
v8::StartupData* blob = NodeMainInstance::GetEmbeddedSnapshotBlob();
const std::vector<size_t>* indexes =
NodeMainInstance::GetIsolateDataIndexes();
if (blob != nullptr) {
params.external_references = external_references.data();
params.snapshot_blob = blob;
}

NodeMainInstance main_instance(&params,
uv_default_loop(),
per_process::v8_platform.Platform(),
result.args,
result.exec_args,
indexes);
result.exit_code = main_instance.Run();
}

Expand Down
Loading