Skip to content

embedding: refactor NewIsolate() #26525

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
addaleax wants to merge 6 commits into from
Closed

embedding: refactor NewIsolate() #26525

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a24ee1
embedding: refactor public `ArrayBufferAllocator` API
addaleax Mar 8, 2019
8a0ff6c
embedding: make `NewIsolate()` API more flexible
addaleax Mar 8, 2019
ff05c1a
fixup! embedding: refactor public `ArrayBufferAllocator` API
addaleax Mar 8, 2019
0f26254
fixup! embedding: make `NewIsolate()` API more flexible
addaleax Mar 8, 2019
45e1f65
fixup! embedding: make `NewIsolate()` API more flexible
addaleax Mar 9, 2019
30d1e83
fixup! embedding: refactor public `ArrayBufferAllocator` API
addaleax Mar 13, 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
Prev Previous commit
Next Next commit
embedding: make NewIsolate() API more flexible 
Split the API up into its essential parts, namely setting up
the creation parameters for the Isolate, creating it, and performing
Node.js-specific customization afterwards.
  • Loading branch information
@addaleax
addaleax committed Mar 8, 2019
commit 8a0ff6cd324359225b160b5fb76b6b567f80e8fc
42 changes: 29 additions & 13 deletions src/api/environment.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,30 +164,25 @@ void FreeArrayBufferAllocator(ArrayBufferAllocator* allocator) {
delete allocator;
}

Isolate* NewIsolate(ArrayBufferAllocator* allocator, uv_loop_t* event_loop) {
Isolate::CreateParams params;
params.array_buffer_allocator = allocator;
void SetIsolateCreateParams(Isolate::CreateParams* params,
ArrayBufferAllocator* allocator) {
if (allocator != nullptr)
params->array_buffer_allocator = allocator;

double total_memory = uv_get_total_memory();
if (total_memory > 0) {
// V8 defaults to 700MB or 1.4GB on 32 and 64 bit platforms respectively.
// This default is based on browser use-cases. Tell V8 to configure the
// heap based on the actual physical memory.
params.constraints.ConfigureDefaults(total_memory, 0);
params->constraints.ConfigureDefaults(total_memory, 0);
}

#ifdef NODE_ENABLE_VTUNE_PROFILING
params.code_event_handler = vTune::GetVtuneCodeEventHandler();
params->code_event_handler = vTune::GetVtuneCodeEventHandler();
#endif
}

Isolate* isolate = Isolate::Allocate();
if (isolate == nullptr) return nullptr;

// Register the isolate on the platform before the isolate gets initialized,
// so that the isolate can access the platform during initialization.
per_process::v8_platform.Platform()->RegisterIsolate(isolate, event_loop);
Isolate::Initialize(isolate, params);

void SetIsolateUpForNode(v8::Isolate* isolate) {
isolate->AddMessageListenerWithErrorLevel(
OnMessage,
Isolate::MessageErrorLevel::kMessageError |
Expand All @@ -197,6 +192,27 @@ Isolate* NewIsolate(ArrayBufferAllocator* allocator, uv_loop_t* event_loop) {
isolate->SetFatalErrorHandler(OnFatalError);
isolate->SetAllowWasmCodeGenerationCallback(AllowWasmCodeGenerationCallback);
v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);
}

Isolate* NewIsolate(ArrayBufferAllocator* allocator, uv_loop_t* event_loop) {
return NewIsolate(allocator, event_loop, GetMainThreadMultiIsolatePlatform());
}

Isolate* NewIsolate(ArrayBufferAllocator* allocator,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform) {
Isolate::CreateParams params;
SetIsolateCreateParams(&params, allocator);

Isolate* isolate = Isolate::Allocate();
if (isolate == nullptr) return nullptr;

// 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);
Isolate::Initialize(isolate, params);

SetIsolateUpForNode(isolate);

return isolate;
}
Expand Down
15 changes: 15 additions & 0 deletions src/node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,24 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
virtual void UnregisterIsolate(v8::Isolate* isolate) = 0;
};

// Set up some Node.js-specific defaults for `params`, in particular
// the ArrayBuffer::Allocator if it is provided, memory limits, and
// possibly a code event handler.
NODE_EXTERN void SetIsolateCreateParams(v8::Isolate::CreateParams* params,
Comment thread
joyeecheung marked this conversation as resolved.
ArrayBufferAllocator* allocator
= nullptr);
// Set a number of callbacks for the `isolate`, in particular the Node.js
// uncaught exception listener.
NODE_EXTERN void SetIsolateUpForNode(v8::Isolate* isolate);
// Creates a new isolate with Node.js-specific settings.
// This is a convenience method equivalent to using SetIsolateCreateParams(),
// Isolate::Allocate(), MultiIsolatePlatform::RegisterIsolate(),
// Isolate::Initialize(), and SetIsolateUpForNow().
Comment thread
addaleax marked this conversation as resolved.
Outdated
NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator,
struct uv_loop_s* event_loop);
NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator,
struct uv_loop_s* event_loop,
MultiIsolatePlatform* platform);

Copy link
Copy Markdown
Member

@gireeshpunathil gireeshpunathil Mar 9, 2019

Choose a reason for hiding this comment

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

Does it make sense to have a NewIsolate constructor that takes only the event_loop , and builds one from the default allocator?

Copy link
Copy Markdown
Member Author

@addaleax addaleax Mar 9, 2019

Choose a reason for hiding this comment

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

How would we transfer ownership of the allocator in that case?

Copy link
Copy Markdown
Member

@gireeshpunathil gireeshpunathil Mar 9, 2019

Choose a reason for hiding this comment

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

like, say for example a class of embedder is not interested in creating or managing an allocator, so not interested in its ownership?

furthermore, do we have a guidance (or user data) that suggests the desired / recommended level of embedding node? With these items (v8, uv loop, allocator, isolate, env, inspector and probably more) that can be custom-created or delegated, what level of control we foresee to give to embedders, and what to keep within node?

Copy link
Copy Markdown
Member

@gireeshpunathil gireeshpunathil Mar 9, 2019

Choose a reason for hiding this comment

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

(I should admit that I don't have insight on this, and did not work with any embedding users that had reported issues or improvements in this area)

Copy link
Copy Markdown
Member Author

@addaleax addaleax Mar 9, 2019

Choose a reason for hiding this comment

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

like, say for example a class of embedder is not interested in creating or managing an allocator, so not interested in its ownership?

Yeah, I mean, I understand why you asked, I just don’t see how we could technically realize this without significant complexity (or maybe at all)? And the embedder would likely okay be okay with maintaining a reference to the allocator, because it has to do that for the Isolate already and can use the same code paths for that.

furthermore, do we have a guidance (or user data) that suggests the desired / recommended level of embedding node? With these items (v8, uv loop, allocator, isolate, env, inspector and probably more) that can be custom-created or delegated, what level of control we foresee to give to embedders, and what to keep within node?

Yeah, it’s a bit difficult to tell where to draw the line. I’d prefer to give embedders more control though if in doubt, because they might need it (for example, Electron hooks into Node’s internals a lot and it would be great to make that unnecessary).

Copy link
Copy Markdown
Member

@gireeshpunathil gireeshpunathil Mar 9, 2019

Choose a reason for hiding this comment

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

ok, thanks for the explanation, make sense to me!

// Creates a new context with Node.js-specific tweaks.
NODE_EXTERN v8::Local<v8::Context> NewContext(
Expand Down