Skip to content

Commit

Permalink
src: remove custom tracking for SharedArrayBuffers
Browse files Browse the repository at this point in the history
Remove custom tracking for `SharedArrayBuffer`s and their allocators
and instead let V8 do the tracking of both. This is required starting
in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s
differs from what was performed previously (i.e. it is no longer
easily possible for one Isolate to release an `ArrayBuffer` and another
to accept it into its own allocator), and the alternative would
have been adapting the `SharedArrayBuffer` tracking logic to also
apply to regular `ArrayBuffer` instances.

Refs: nodejs#30044

PR-URL: nodejs#30020
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
  • Loading branch information
addaleax authored and targos committed Nov 8, 2019
1 parent 2707efd commit 2bdeb88
Show file tree
Hide file tree
Showing 13 changed files with 58 additions and 335 deletions.
2 changes: 0 additions & 2 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,6 @@
'src/node_zlib.cc',
'src/pipe_wrap.cc',
'src/process_wrap.cc',
'src/sharedarraybuffer_metadata.cc',
'src/signal_wrap.cc',
'src/spawn_sync.cc',
'src/stream_base.cc',
Expand Down Expand Up @@ -642,7 +641,6 @@
'src/pipe_wrap.h',
'src/req_wrap.h',
'src/req_wrap-inl.h',
'src/sharedarraybuffer_metadata.h',
'src/spawn_sync.h',
'src/stream_base.h',
'src/stream_base-inl.h',
Expand Down
8 changes: 8 additions & 0 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,14 @@ Isolate* NewIsolate(ArrayBufferAllocator* allocator,
return NewIsolate(&params, event_loop, platform);
}

Isolate* NewIsolate(std::shared_ptr<ArrayBufferAllocator> allocator,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform) {
Isolate::CreateParams params;
if (allocator) params.array_buffer_allocator_shared = allocator;
return NewIsolate(&params, event_loop, platform);
}

IsolateData* CreateIsolateData(Isolate* isolate,
uv_loop_t* loop,
MultiIsolatePlatform* platform,
Expand Down
15 changes: 0 additions & 15 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1038,21 +1038,6 @@ char* Environment::Reallocate(char* data, size_t old_size, size_t size) {
return new_data;
}

void Environment::AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(
std::shared_ptr<v8::ArrayBuffer::Allocator> allocator) {
if (keep_alive_allocators_ == nullptr) {
MultiIsolatePlatform* platform = isolate_data()->platform();
CHECK_NOT_NULL(platform);

keep_alive_allocators_ = new ArrayBufferAllocatorList();
platform->AddIsolateFinishedCallback(isolate(), [](void* data) {
delete static_cast<ArrayBufferAllocatorList*>(data);
}, static_cast<void*>(keep_alive_allocators_));
}

keep_alive_allocators_->insert(allocator);
}

bool Environment::RunWeakRefCleanup() {
isolate()->ClearKeptObjects();

Expand Down
9 changes: 0 additions & 9 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ constexpr size_t kFsStatsBufferLength =
V(contextify_global_private_symbol, "node:contextify:global") \
V(decorated_private_symbol, "node:decorated") \
V(napi_wrapper, "node:napi:wrapper") \
V(sab_lifetimepartner_symbol, "node:sharedArrayBufferLifetimePartner") \

// Symbols are per-isolate primitives but Environment proxies them
// for the sake of convenience.
Expand Down Expand Up @@ -1250,10 +1249,6 @@ class Environment : public MemoryRetainer {

#endif // HAVE_INSPECTOR

// Only available if a MultiIsolatePlatform is in use.
void AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(
std::shared_ptr<v8::ArrayBuffer::Allocator>);

private:
template <typename Fn>
inline void CreateImmediate(Fn&& cb,
Expand Down Expand Up @@ -1433,10 +1428,6 @@ class Environment : public MemoryRetainer {
// Used by embedders to shutdown running Node instance.
AsyncRequest thread_stopper_;

typedef std::unordered_set<std::shared_ptr<v8::ArrayBuffer::Allocator>>
ArrayBufferAllocatorList;
ArrayBufferAllocatorList* keep_alive_allocators_ = nullptr;

template <typename T>
void ForEachBaseObject(T&& iterator);

Expand Down
16 changes: 16 additions & 0 deletions src/memory_tracker-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,16 @@ void MemoryTracker::TrackField(const char* edge_name,
TrackField(edge_name, value.get(), node_name);
}

template <typename T>
void MemoryTracker::TrackField(const char* edge_name,
const std::shared_ptr<T>& value,
const char* node_name) {
if (value.get() == nullptr) {
return;
}
TrackField(edge_name, value.get(), node_name);
}

template <typename T, typename Iterator>
void MemoryTracker::TrackField(const char* edge_name,
const T& value,
Expand Down Expand Up @@ -206,6 +216,12 @@ void MemoryTracker::TrackField(const char* edge_name,
TrackFieldWithSize(edge_name, value.size, "MallocedBuffer");
}

void MemoryTracker::TrackField(const char* edge_name,
const v8::BackingStore* value,
const char* node_name) {
TrackFieldWithSize(edge_name, value->ByteLength(), "BackingStore");
}

void MemoryTracker::TrackField(const char* name,
const uv_buf_t& value,
const char* node_name) {
Expand Down
7 changes: 7 additions & 0 deletions src/memory_tracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ class MemoryTracker {
inline void TrackField(const char* edge_name,
const std::unique_ptr<T>& value,
const char* node_name = nullptr);
template <typename T>
inline void TrackField(const char* edge_name,
const std::shared_ptr<T>& value,
const char* node_name = nullptr);

// For containers, the elements will be graphed as grandchildren nodes
// if the container is not empty.
Expand Down Expand Up @@ -197,6 +201,9 @@ class MemoryTracker {
inline void TrackField(const char* edge_name,
const MallocedBuffer<T>& value,
const char* node_name = nullptr);
inline void TrackField(const char* edge_name,
const v8::BackingStore* value,
const char* node_name = nullptr);
// We do not implement CleanupHookCallback as MemoryRetainer
// but instead specialize the method here to avoid the cost of
// virtual pointers.
Expand Down
4 changes: 4 additions & 0 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,10 @@ NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator,
NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator,
struct uv_loop_s* event_loop,
MultiIsolatePlatform* platform);
NODE_EXTERN v8::Isolate* NewIsolate(
std::shared_ptr<ArrayBufferAllocator> allocator,
struct uv_loop_s* event_loop,
MultiIsolatePlatform* platform);

// Creates a new context with Node.js-specific tweaks.
NODE_EXTERN v8::Local<v8::Context> NewContext(
Expand Down
65 changes: 16 additions & 49 deletions src/node_messaging.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
using node::contextify::ContextifyContext;
using v8::Array;
using v8::ArrayBuffer;
using v8::ArrayBufferCreationMode;
using v8::BackingStore;
using v8::Context;
using v8::EscapableHandleScope;
using v8::Exception;
Expand Down Expand Up @@ -123,10 +123,9 @@ MaybeLocal<Value> Message::Deserialize(Environment* env,
std::vector<Local<SharedArrayBuffer>> shared_array_buffers;
// Attach all transferred SharedArrayBuffers to their new Isolate.
for (uint32_t i = 0; i < shared_array_buffers_.size(); ++i) {
Local<SharedArrayBuffer> sab;
if (!shared_array_buffers_[i]->GetSharedArrayBuffer(env, context)
.ToLocal(&sab))
return MaybeLocal<Value>();
Local<SharedArrayBuffer> sab =
SharedArrayBuffer::New(env->isolate(),
std::move(shared_array_buffers_[i]));
shared_array_buffers.push_back(sab);
}
shared_array_buffers_.clear();
Expand All @@ -141,30 +140,12 @@ MaybeLocal<Value> Message::Deserialize(Environment* env,
delegate.deserializer = &deserializer;

// Attach all transferred ArrayBuffers to their new Isolate.
for (uint32_t i = 0; i < array_buffer_contents_.size(); ++i) {
if (!env->isolate_data()->uses_node_allocator()) {
// We don't use Node's allocator on the receiving side, so we have
// to create the ArrayBuffer from a copy of the memory.
AllocatedBuffer buf =
env->AllocateManaged(array_buffer_contents_[i].size);
memcpy(buf.data(),
array_buffer_contents_[i].data,
array_buffer_contents_[i].size);
deserializer.TransferArrayBuffer(i, buf.ToArrayBuffer());
continue;
}

env->isolate_data()->node_allocator()->RegisterPointer(
array_buffer_contents_[i].data, array_buffer_contents_[i].size);

for (uint32_t i = 0; i < array_buffers_.size(); ++i) {
Local<ArrayBuffer> ab =
ArrayBuffer::New(env->isolate(),
array_buffer_contents_[i].release(),
array_buffer_contents_[i].size,
ArrayBufferCreationMode::kInternalized);
ArrayBuffer::New(env->isolate(), std::move(array_buffers_[i]));
deserializer.TransferArrayBuffer(i, ab);
}
array_buffer_contents_.clear();
array_buffers_.clear();

if (deserializer.ReadHeader(context).IsNothing())
return MaybeLocal<Value>();
Expand All @@ -173,8 +154,8 @@ MaybeLocal<Value> Message::Deserialize(Environment* env,
}

void Message::AddSharedArrayBuffer(
const SharedArrayBufferMetadataReference& reference) {
shared_array_buffers_.push_back(reference);
std::shared_ptr<BackingStore> backing_store) {
shared_array_buffers_.emplace_back(std::move(backing_store));
}

void Message::AddMessagePort(std::unique_ptr<MessagePortData>&& data) {
Expand Down Expand Up @@ -249,16 +230,9 @@ class SerializerDelegate : public ValueSerializer::Delegate {
}
}

auto reference = SharedArrayBufferMetadata::ForSharedArrayBuffer(
env_,
context_,
shared_array_buffer);
if (!reference) {
return Nothing<uint32_t>();
}
seen_shared_array_buffers_.emplace_back(
Global<SharedArrayBuffer> { isolate, shared_array_buffer });
msg_->AddSharedArrayBuffer(reference);
msg_->AddSharedArrayBuffer(shared_array_buffer->GetBackingStore());
return Just(i);
}

Expand Down Expand Up @@ -386,18 +360,12 @@ Maybe<bool> Message::Serialize(Environment* env,
}

for (Local<ArrayBuffer> ab : array_buffers) {
// If serialization succeeded, we want to take ownership of
// (a.k.a. externalize) the underlying memory region and render
// it inaccessible in this Isolate.
ArrayBuffer::Contents contents = ab->Externalize();
// If serialization succeeded, we render it inaccessible in this Isolate.
std::shared_ptr<BackingStore> backing_store = ab->GetBackingStore();
ab->Externalize(backing_store);
ab->Detach();

CHECK(env->isolate_data()->uses_node_allocator());
env->isolate_data()->node_allocator()->UnregisterPointer(
contents.Data(), contents.ByteLength());

array_buffer_contents_.emplace_back(MallocedBuffer<char>{
static_cast<char*>(contents.Data()), contents.ByteLength()});
array_buffers_.emplace_back(std::move(backing_store));
}

delegate.Finish();
Expand All @@ -411,9 +379,8 @@ Maybe<bool> Message::Serialize(Environment* env,
}

void Message::MemoryInfo(MemoryTracker* tracker) const {
tracker->TrackField("array_buffer_contents", array_buffer_contents_);
tracker->TrackFieldWithSize("shared_array_buffers",
shared_array_buffers_.size() * sizeof(shared_array_buffers_[0]));
tracker->TrackField("array_buffers_", array_buffers_);
tracker->TrackField("shared_array_buffers", shared_array_buffers_);
tracker->TrackField("message_ports", message_ports_);
}

Expand Down
7 changes: 3 additions & 4 deletions src/node_messaging.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

#include "env.h"
#include "node_mutex.h"
#include "sharedarraybuffer_metadata.h"
#include <list>

namespace node {
Expand Down Expand Up @@ -52,7 +51,7 @@ class Message : public MemoryRetainer {

// Internal method of Message that is called when a new SharedArrayBuffer
// object is encountered in the incoming value's structure.
void AddSharedArrayBuffer(const SharedArrayBufferMetadataReference& ref);
void AddSharedArrayBuffer(std::shared_ptr<v8::BackingStore> backing_store);
// Internal method of Message that is called once serialization finishes
// and that transfers ownership of `data` to this message.
void AddMessagePort(std::unique_ptr<MessagePortData>&& data);
Expand All @@ -74,8 +73,8 @@ class Message : public MemoryRetainer {

private:
MallocedBuffer<char> main_message_buf_;
std::vector<MallocedBuffer<char>> array_buffer_contents_;
std::vector<SharedArrayBufferMetadataReference> shared_array_buffers_;
std::vector<std::shared_ptr<v8::BackingStore>> array_buffers_;
std::vector<std::shared_ptr<v8::BackingStore>> shared_array_buffers_;
std::vector<std::unique_ptr<MessagePortData>> message_ports_;
std::vector<v8::WasmModuleObject::TransferrableModule> wasm_modules_;

Expand Down
11 changes: 4 additions & 7 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ Worker::Worker(Environment* env,
per_isolate_opts_(per_isolate_opts),
exec_argv_(exec_argv),
platform_(env->isolate_data()->platform()),
array_buffer_allocator_(ArrayBufferAllocator::Create()),
start_profiler_idle_notifier_(env->profiler_idle_notifier_started()),
thread_id_(Environment::AllocateThreadId()),
env_vars_(env->env_vars()) {
Expand Down Expand Up @@ -95,10 +94,6 @@ bool Worker::is_stopped() const {
return stopped_;
}

std::shared_ptr<ArrayBufferAllocator> Worker::array_buffer_allocator() {
return array_buffer_allocator_;
}

void Worker::UpdateResourceConstraints(ResourceConstraints* constraints) {
constraints->set_stack_limit(reinterpret_cast<uint32_t*>(stack_base_));

Expand Down Expand Up @@ -138,9 +133,11 @@ class WorkerThreadData {
: w_(w) {
CHECK_EQ(uv_loop_init(&loop_), 0);

std::shared_ptr<ArrayBufferAllocator> allocator =
ArrayBufferAllocator::Create();
Isolate::CreateParams params;
SetIsolateCreateParamsForNode(&params);
params.array_buffer_allocator = w->array_buffer_allocator_.get();
params.array_buffer_allocator_shared = allocator;

w->UpdateResourceConstraints(&params.constraints);

Expand All @@ -164,7 +161,7 @@ class WorkerThreadData {
isolate_data_.reset(CreateIsolateData(isolate,
&loop_,
w_->platform_,
w->array_buffer_allocator_.get()));
allocator.get()));
CHECK(isolate_data_);
if (w_->per_isolate_opts_)
isolate_data_->set_options(std::move(w_->per_isolate_opts_));
Expand Down
2 changes: 0 additions & 2 deletions src/node_worker.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ class Worker : public AsyncWrap {
SET_SELF_SIZE(Worker)

bool is_stopped() const;
std::shared_ptr<ArrayBufferAllocator> array_buffer_allocator();

static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
static void CloneParentEnvVars(
Expand All @@ -72,7 +71,6 @@ class Worker : public AsyncWrap {
std::vector<std::string> argv_;

MultiIsolatePlatform* platform_;
std::shared_ptr<ArrayBufferAllocator> array_buffer_allocator_;
v8::Isolate* isolate_ = nullptr;
bool start_profiler_idle_notifier_;
uv_thread_t tid_;
Expand Down
Loading

0 comments on commit 2bdeb88

Please sign in to comment.