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.

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

embedding: refactor NewIsolate() #26525

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
65 changes: 43 additions & 22 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ static void OnMessage(Local<Message> message, Local<Value> error) {
}
}

void* ArrayBufferAllocator::Allocate(size_t size) {
void* NodeArrayBufferAllocator::Allocate(size_t size) {
if (zero_fill_field_ || per_process::cli_options->zero_fill_all_buffers)
return UncheckedCalloc(size);
else
Expand All @@ -84,29 +84,29 @@ DebuggingArrayBufferAllocator::~DebuggingArrayBufferAllocator() {

void* DebuggingArrayBufferAllocator::Allocate(size_t size) {
Mutex::ScopedLock lock(mutex_);
void* data = ArrayBufferAllocator::Allocate(size);
void* data = NodeArrayBufferAllocator::Allocate(size);
RegisterPointerInternal(data, size);
return data;
}

void* DebuggingArrayBufferAllocator::AllocateUninitialized(size_t size) {
Mutex::ScopedLock lock(mutex_);
void* data = ArrayBufferAllocator::AllocateUninitialized(size);
void* data = NodeArrayBufferAllocator::AllocateUninitialized(size);
RegisterPointerInternal(data, size);
return data;
}

void DebuggingArrayBufferAllocator::Free(void* data, size_t size) {
Mutex::ScopedLock lock(mutex_);
UnregisterPointerInternal(data, size);
ArrayBufferAllocator::Free(data, size);
NodeArrayBufferAllocator::Free(data, size);
}

void* DebuggingArrayBufferAllocator::Reallocate(void* data,
size_t old_size,
size_t size) {
Mutex::ScopedLock lock(mutex_);
void* ret = ArrayBufferAllocator::Reallocate(data, old_size, size);
void* ret = NodeArrayBufferAllocator::Reallocate(data, old_size, size);
if (ret == nullptr) {
if (size == 0) // i.e. equivalent to free().
UnregisterPointerInternal(data, old_size);
Expand Down Expand Up @@ -149,41 +149,40 @@ void DebuggingArrayBufferAllocator::RegisterPointerInternal(void* data,
allocations_[data] = size;
}

ArrayBufferAllocator* CreateArrayBufferAllocator() {
if (per_process::cli_options->debug_arraybuffer_allocations)
return new DebuggingArrayBufferAllocator();
std::unique_ptr<ArrayBufferAllocator> ArrayBufferAllocator::Create(bool debug) {
if (debug || per_process::cli_options->debug_arraybuffer_allocations)
return std::make_unique<DebuggingArrayBufferAllocator>();
else
return new ArrayBufferAllocator();
return std::make_unique<NodeArrayBufferAllocator>();
}

ArrayBufferAllocator* CreateArrayBufferAllocator() {
return ArrayBufferAllocator::Create().release();
}

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 @@ -192,7 +191,29 @@ Isolate* NewIsolate(ArrayBufferAllocator* allocator, uv_loop_t* event_loop) {
isolate->SetMicrotasksPolicy(MicrotasksPolicy::kExplicit);
isolate->SetFatalErrorHandler(OnFatalError);
isolate->SetAllowWasmCodeGenerationCallback(AllowWasmCodeGenerationCallback);
isolate->SetPromiseRejectCallback(task_queue::PromiseRejectCallback);
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
2 changes: 1 addition & 1 deletion src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ inline v8::ArrayBuffer::Allocator* IsolateData::allocator() const {
return allocator_;
}

inline ArrayBufferAllocator* IsolateData::node_allocator() const {
inline NodeArrayBufferAllocator* IsolateData::node_allocator() const {
return node_allocator_;
}

Expand Down
7 changes: 2 additions & 5 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ IsolateData::IsolateData(Isolate* isolate,
: isolate_(isolate),
event_loop_(event_loop),
allocator_(isolate->GetArrayBufferAllocator()),
node_allocator_(node_allocator),
node_allocator_(node_allocator == nullptr ?
nullptr : node_allocator->GetImpl()),
uses_node_allocator_(allocator_ == node_allocator_),
platform_(platform) {
CHECK_NOT_NULL(allocator_);
Expand Down Expand Up @@ -257,10 +258,6 @@ Environment::Environment(IsolateData* isolate_data,
if (options_->no_force_async_hooks_checks) {
async_hooks_.no_force_checks();
}

// TODO(addaleax): the per-isolate state should not be controlled by
// a single Environment.
isolate()->SetPromiseRejectCallback(task_queue::PromiseRejectCallback);
}

CompileFnEntry::CompileFnEntry(Environment* env, uint32_t id)
Expand Down
4 changes: 2 additions & 2 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ class IsolateData {

inline bool uses_node_allocator() const;
inline v8::ArrayBuffer::Allocator* allocator() const;
inline ArrayBufferAllocator* node_allocator() const;
inline NodeArrayBufferAllocator* node_allocator() const;

#define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
#define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
Expand Down Expand Up @@ -442,7 +442,7 @@ class IsolateData {
v8::Isolate* const isolate_;
uv_loop_t* const event_loop_;
v8::ArrayBuffer::Allocator* const allocator_;
ArrayBufferAllocator* const node_allocator_;
NodeArrayBufferAllocator* const node_allocator_;
const bool uses_node_allocator_;
MultiIsolatePlatform* platform_;
std::shared_ptr<PerIsolateOptions> options_;
Expand Down
41 changes: 40 additions & 1 deletion src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@
#include "v8-platform.h" // NOLINT(build/include_order)
#include "node_version.h" // NODE_MODULE_VERSION

#include <memory>

#define NODE_MAKE_VERSION(major, minor, patch) \
((major) * 0x1000 + (minor) * 0x100 + (patch))

Expand Down Expand Up @@ -210,8 +212,30 @@ NODE_EXTERN void Init(int* argc,
int* exec_argc,
const char*** exec_argv);

class ArrayBufferAllocator;
class NodeArrayBufferAllocator;

// An ArrayBuffer::Allocator class with some Node.js-specific tweaks. If you do
// not have to use another allocator, using this class is recommended:
// - It supports Buffer.allocUnsafe() and Buffer.allocUnsafeSlow() with
// uninitialized memory.
// - It supports transferring, rather than copying, ArrayBuffers when using
// MessagePorts.
class NODE_EXTERN ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
public:
// If `always_debug` is true, create an ArrayBuffer::Allocator instance
// that performs additional integrity checks (e.g. make sure that only memory
// that was allocated by the it is also freed by it).
// This can also be set using the --debug-arraybuffer-allocations flag.
static std::unique_ptr<ArrayBufferAllocator> Create(
bool always_debug = false);

private:
virtual NodeArrayBufferAllocator* GetImpl() = 0;

friend class IsolateData;
};

// Legacy equivalents for ArrayBufferAllocator::Create().
NODE_EXTERN ArrayBufferAllocator* CreateArrayBufferAllocator();
NODE_EXTERN void FreeArrayBufferAllocator(ArrayBufferAllocator* allocator);

Expand All @@ -234,9 +258,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,
joyeecheung marked this conversation as resolved.
Show resolved Hide 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 SetIsolateUpForNode().
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
Member

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
Member Author

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
Member

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
Member

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
Member Author

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
Member

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
3 changes: 2 additions & 1 deletion src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,8 @@ void Initialize(Local<Object> target,

// It can be a nullptr when running inside an isolate where we
// do not own the ArrayBuffer allocator.
if (ArrayBufferAllocator* allocator = env->isolate_data()->node_allocator()) {
if (NodeArrayBufferAllocator* allocator =
env->isolate_data()->node_allocator()) {
uint32_t* zero_fill_field = allocator->zero_fill_field();
Local<ArrayBuffer> array_buffer = ArrayBuffer::New(
env->isolate(), zero_fill_field, sizeof(*zero_fill_field));
Expand Down
6 changes: 4 additions & 2 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ namespace task_queue {
void PromiseRejectCallback(v8::PromiseRejectMessage message);
} // namespace task_queue

class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
class NodeArrayBufferAllocator : public ArrayBufferAllocator {
public:
inline uint32_t* zero_fill_field() { return &zero_fill_field_; }

Expand All @@ -116,11 +116,13 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
virtual void RegisterPointer(void* data, size_t size) {}
virtual void UnregisterPointer(void* data, size_t size) {}

NodeArrayBufferAllocator* GetImpl() final { return this; }

private:
uint32_t zero_fill_field_ = 1; // Boolean but exposed as uint32 to JS land.
};

class DebuggingArrayBufferAllocator final : public ArrayBufferAllocator {
class DebuggingArrayBufferAllocator final : public NodeArrayBufferAllocator {
public:
~DebuggingArrayBufferAllocator() override;
void* Allocate(size_t size) override;
Expand Down