Skip to content

src: implement minimal v8 snapshot integration #27321

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

Closed
joyeecheung wants to merge 10 commits into from
Closed

src: implement minimal v8 snapshot integration #27321

Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
src: allow creating NodeMainInstance that does not own the isolate 
Allows instantiating a NodeMainInstance with an isolate
whose initialization and disposal are controlled by the caller.
  • Loading branch information
@joyeecheung
joyeecheung committed Apr 20, 2019
commit 076c31f3eff309df8f7e0129db7a8a1982b3e944
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);

@bnoordhuis bnoordhuis Apr 22, 2019

Copy link
Copy Markdown
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.

@joyeecheung joyeecheung Apr 22, 2019

Copy link
Copy Markdown
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).

@bnoordhuis bnoordhuis Apr 22, 2019

Copy link
Copy Markdown
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.

@joyeecheung joyeecheung Apr 22, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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
8 changes: 6 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,12 @@ int Start(int argc, char** argv) {
}

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

@refack refack Apr 20, 2019

Copy link
Copy Markdown
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;
      }
      ...

@joyeecheung joyeecheung Apr 20, 2019

Copy link
Copy Markdown
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.

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

Expand Down
3 changes: 3 additions & 0 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,9 @@ struct InitializationResult {
};
InitializationResult InitializeOncePerProcess(int argc, char** argv);
void TearDownOncePerProcess();
enum class IsolateSettingCategories { kErrorHandlers, kMisc };
void SetIsolateUpForNode(v8::Isolate* isolate, IsolateSettingCategories cat);
void SetIsolateCreateParamsForNode(v8::Isolate::CreateParams* params);

#if HAVE_INSPECTOR
namespace profiler {
Expand Down
67 changes: 54 additions & 13 deletions src/node_main_instance.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,30 +12,70 @@ using v8::Local;
using v8::Locker;
using v8::SealHandleScope;

NodeMainInstance::NodeMainInstance(uv_loop_t* event_loop,
NodeMainInstance::NodeMainInstance(Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform,
const std::vector<std::string>& args,
const std::vector<std::string>& exec_args)
: args_(args),
exec_args_(exec_args),
array_buffer_allocator_(nullptr),
isolate_(isolate),
platform_(platform),
isolate_data_(nullptr),
owns_isolate_(false) {
isolate_data_.reset(new IsolateData(isolate_, event_loop, platform, nullptr));
SetIsolateUpForNode(isolate_, IsolateSettingCategories::kMisc);
}

NodeMainInstance* NodeMainInstance::Create(
Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform,
const std::vector<std::string>& args,
const std::vector<std::string>& exec_args) {
return new NodeMainInstance(isolate, event_loop, platform, args, exec_args);
}

NodeMainInstance::NodeMainInstance(Isolate::CreateParams* params,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform,
const std::vector<std::string>& args,
const std::vector<std::string>& exec_args)
: args_(args),
exec_args_(exec_args),
array_buffer_allocator_(ArrayBufferAllocator::Create()),
isolate_(nullptr),
isolate_data_(nullptr) {
// TODO(joyeecheung): when we implement snapshot integration this needs to
// set params.external_references.
Isolate::CreateParams params;
params.array_buffer_allocator = array_buffer_allocator_.get();
isolate_ =
NewIsolate(&params, event_loop, per_process::v8_platform.Platform());
platform_(platform),
isolate_data_(nullptr),
owns_isolate_(true) {
params->array_buffer_allocator = array_buffer_allocator_.get();
isolate_ = Isolate::Allocate();
CHECK_NOT_NULL(isolate_);
isolate_data_.reset(CreateIsolateData(isolate_,
event_loop,
per_process::v8_platform.Platform(),
array_buffer_allocator_.get()));
// Register the isolate on the platform before the isolate gets initialized,
// so that the isolate can access the platform during initialization.
platform->RegisterIsolate(isolate_, event_loop);
SetIsolateCreateParamsForNode(params);
Isolate::Initialize(isolate_, *params);

isolate_data_.reset(new IsolateData(
isolate_, event_loop, platform, array_buffer_allocator_.get()));
SetIsolateUpForNode(isolate_, IsolateSettingCategories::kMisc);
SetIsolateUpForNode(isolate_, IsolateSettingCategories::kErrorHandlers);
}

void NodeMainInstance::Dispose() {
CHECK(!owns_isolate_);
platform_->DrainTasks(isolate_);
delete this;
}

NodeMainInstance::~NodeMainInstance() {
if (!owns_isolate_) {
return;
}
isolate_->Dispose();
per_process::v8_platform.Platform()->UnregisterIsolate(isolate_);
platform_->UnregisterIsolate(isolate_);
}

int NodeMainInstance::Run() {
Expand Down Expand Up @@ -120,6 +160,7 @@ std::unique_ptr<Environment> NodeMainInstance::CreateMainEnvironment(
}

Local<Context> context = NewContext(isolate_);
CHECK(!context.IsEmpty());
Context::Scope context_scope(context);

std::unique_ptr<Environment> env = std::make_unique<Environment>(
Expand Down
48 changes: 42 additions & 6 deletions src/node_main_instance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include <cinttypes>
Comment thread
joyeecheung marked this conversation as resolved.
Outdated
#include "node.h"
#include "util.h"
#include "uv.h"
Expand All @@ -14,29 +15,64 @@ namespace node {
// We may be able to create an abstract class to reuse some of the routines.
class NodeMainInstance {
public:
NodeMainInstance(const NodeMainInstance&) = delete;
NodeMainInstance& operator=(const NodeMainInstance&) = delete;
NodeMainInstance(NodeMainInstance&&) = delete;
NodeMainInstance& operator=(NodeMainInstance&&) = delete;
// To create a main instance that does not own the isoalte,
// The caller needs to do:
//
// Isolate* isolate = Isolate::Allocate();
// platform->RegisterIsolate(isolate, loop);
// isolate->Initialize(...);
// isolate->Enter();
// NodeMainInstance* main_instance =
// NodeMainInstance::Create(isolate, loop, args, exec_args);
//
// When tearing it down:
//
// main_instance->Cleanup(); // While the isolate is entered
// isolate->Exit();
// isolate->Dispose();
// platform->UnregisterIsolate(isolate);
//
// After calling Dispose() the main_instance is no longer accessible.
static NodeMainInstance* Create(v8::Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform,
const std::vector<std::string>& args,
const std::vector<std::string>& exec_args);
void Dispose();

NodeMainInstance(uv_loop_t* event_loop,
// Create a main instance that owns the isolate
NodeMainInstance(v8::Isolate::CreateParams* params,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform,
const std::vector<std::string>& args,
const std::vector<std::string>& exec_args);
~NodeMainInstance();

// Start running the Node.js instances, return the exit code when finished.
int Run();

private:
// TODO(joyeecheung): align this with the CreateEnvironment exposed in node.h
// and the environment creation routine in workers somehow.
std::unique_ptr<Environment> CreateMainEnvironment(int* exit_code);

private:
NodeMainInstance(const NodeMainInstance&) = delete;
NodeMainInstance& operator=(const NodeMainInstance&) = delete;
NodeMainInstance(NodeMainInstance&&) = delete;
NodeMainInstance& operator=(NodeMainInstance&&) = delete;

NodeMainInstance(v8::Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform,
const std::vector<std::string>& args,
const std::vector<std::string>& exec_args);
std::vector<std::string> args_;
std::vector<std::string> exec_args_;
std::unique_ptr<ArrayBufferAllocator> array_buffer_allocator_;
v8::Isolate* isolate_;
MultiIsolatePlatform* platform_;
std::unique_ptr<IsolateData> isolate_data_;
bool owns_isolate_ = false;
};

} // namespace node
Expand Down