Skip to content

Commit

Permalink
lib,src: clean up ArrayBufferAllocator
Browse files Browse the repository at this point in the history 
Remove the direct dependency on node::Environment (which is per-context)
from node::ArrayBufferAllocator (which is per-isolate.)

Contexts that want to toggle the zero fill flag, now do so through a
field that is owned by ArrayBufferAllocator.  Better, still not great.

PR-URL: #7082
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  • Loading branch information
@bnoordhuis
bnoordhuis committed Jun 1, 2016
1 parent 334ef4f commit 27e84dd
Show file tree
Hide file tree
Showing 7 changed files with 39 additions and 91 deletions.
18 changes: 8 additions & 10 deletions lib/buffer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,16 @@ Buffer.prototype.swap32 = function swap32() {
return swap32n.apply(this);
};

const flags = bindingObj.flags;
const kNoZeroFill = 0;
// |binding.zeroFill| can be undefined when running inside an isolate where we
// do not own the ArrayBuffer allocator. Zero fill is always on in that case.

This comment has been minimized.

Sign in to view

Copy link
@Trott

Trott Sep 26, 2016

Member

@bnoordhuis If we wanted to run a test that exercised the right hand side of the || below, how would we do that? Like, what does it mean to "not own the ArrayBuffer allocator"? Is this a test that can be performed in JS-land or not so much? /cc @oogz

This comment has been minimized.

Sign in to view

Copy link
@bnoordhuis

bnoordhuis Sep 27, 2016

Author Member

You could do it through a cctest or add-on test that creates an isolate with a custom v8::ArrayBuffer::Allocator and a node context with node::CreateEnvironment().

This comment has been minimized.

Sign in to view

Copy link
@bengl

bengl Mar 1, 2017

Member

I'm full of questions: Is the || [0] even necessary? Isn't it safe to assume that lib/buffer.js is always running with node's ArrayBuffer::Allocator? Is there a (realistic) environment is using node::CreateEnvironment() without node's allocator? Are such environments actually supported? Should they be?

This comment has been minimized.

Sign in to view

Copy link
@bnoordhuis

bnoordhuis Mar 2, 2017

Author Member

node::CreateEnvironment() was added by popular demand from people embedding node so yes, I would say it is in use and no, they won't be using node's allocator.
const zeroFill = bindingObj.zeroFill || [0];

function createBuffer(size, noZeroFill) {
flags[kNoZeroFill] = noZeroFill ? 1 : 0;
try {
const ui8 = new Uint8Array(size);
Object.setPrototypeOf(ui8, Buffer.prototype);
return ui8;
} finally {
flags[kNoZeroFill] = 0;
}
if (noZeroFill)
zeroFill[0] = 0; // Reset by the runtime.
const ui8 = new Uint8Array(size);
Object.setPrototypeOf(ui8, Buffer.prototype);
return ui8;
}

function createPool() {
Expand Down
3 changes: 2 additions & 1 deletion src/debug-agent.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,8 @@ void Agent::WorkerRun() {
Isolate::Scope isolate_scope(isolate);

HandleScope handle_scope(isolate);
IsolateData isolate_data(isolate, &child_loop_);
IsolateData isolate_data(isolate, &child_loop_,
array_buffer_allocator.zero_fill_field());
Local<Context> context = Context::New(isolate);

Context::Scope context_scope(context);
Expand Down
36 changes: 8 additions & 28 deletions src/env-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ namespace node {
//
// One byte because our strings are ASCII and we can safely skip V8's UTF-8
// decoding step. It's a one-time cost, but why pay it when you don't have to?
inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop)
inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop,
uint32_t* zero_fill_field)
:
#define V(PropertyName, StringValue) \
PropertyName ## _( \
Expand All @@ -48,12 +49,17 @@ inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop)
sizeof(StringValue) - 1).ToLocalChecked()),
PER_ISOLATE_STRING_PROPERTIES(V)
#undef V
isolate_(isolate), event_loop_(event_loop) {}
isolate_(isolate), event_loop_(event_loop),
zero_fill_field_(zero_fill_field) {}

inline uv_loop_t* IsolateData::event_loop() const {
return event_loop_;
}

inline uint32_t* IsolateData::zero_fill_field() const {
return zero_fill_field_;
}

inline Environment::AsyncHooks::AsyncHooks() {
for (int i = 0; i < kFieldsCount; i++) fields_[i] = 0;
}
Expand Down Expand Up @@ -128,27 +134,6 @@ inline void Environment::TickInfo::set_index(uint32_t value) {
fields_[kIndex] = value;
}

inline Environment::ArrayBufferAllocatorInfo::ArrayBufferAllocatorInfo() {
for (int i = 0; i < kFieldsCount; ++i)
fields_[i] = 0;
}

inline uint32_t* Environment::ArrayBufferAllocatorInfo::fields() {
return fields_;
}

inline int Environment::ArrayBufferAllocatorInfo::fields_count() const {
return kFieldsCount;
}

inline bool Environment::ArrayBufferAllocatorInfo::no_zero_fill() const {
return fields_[kNoZeroFill] != 0;
}

inline void Environment::ArrayBufferAllocatorInfo::reset_fill_flag() {
fields_[kNoZeroFill] = 0;
}

inline Environment* Environment::New(IsolateData* isolate_data,
v8::Local<v8::Context> context) {
Environment* env = new Environment(isolate_data, context);
Expand Down Expand Up @@ -318,11 +303,6 @@ inline Environment::TickInfo* Environment::tick_info() {
return &tick_info_;
}

inline Environment::ArrayBufferAllocatorInfo*
Environment::array_buffer_allocator_info() {
return &array_buffer_allocator_info_;
}

inline uint64_t Environment::timer_base() const {
return timer_base_;
}
Expand Down
30 changes: 5 additions & 25 deletions src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,8 +304,10 @@ RB_HEAD(ares_task_list, ares_task_t);

class IsolateData {
public:
inline IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop);
inline IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop,
uint32_t* zero_fill_field = nullptr);
inline uv_loop_t* event_loop() const;
inline uint32_t* zero_fill_field() const;

#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue)
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
Expand All @@ -330,6 +332,7 @@ class IsolateData {

v8::Isolate* const isolate_;
uv_loop_t* const event_loop_;
uint32_t* const zero_fill_field_;

DISALLOW_COPY_AND_ASSIGN(IsolateData);
};
Expand Down Expand Up @@ -414,27 +417,6 @@ class Environment {
DISALLOW_COPY_AND_ASSIGN(TickInfo);
};

class ArrayBufferAllocatorInfo {
public:
inline uint32_t* fields();
inline int fields_count() const;
inline bool no_zero_fill() const;
inline void reset_fill_flag();

private:
friend class Environment; // So we can call the constructor.
inline ArrayBufferAllocatorInfo();

enum Fields {
kNoZeroFill,
kFieldsCount
};

uint32_t fields_[kFieldsCount];

DISALLOW_COPY_AND_ASSIGN(ArrayBufferAllocatorInfo);
};

typedef void (*HandleCleanupCb)(Environment* env,
uv_handle_t* handle,
void* arg);
Expand Down Expand Up @@ -497,14 +479,14 @@ class Environment {
inline AsyncHooks* async_hooks();
inline DomainFlag* domain_flag();
inline TickInfo* tick_info();
inline ArrayBufferAllocatorInfo* array_buffer_allocator_info();
inline uint64_t timer_base() const;

static inline Environment* from_cares_timer_handle(uv_timer_t* handle);
inline uv_timer_t* cares_timer_handle();
inline ares_channel cares_channel();
inline ares_channel* cares_channel_ptr();
inline ares_task_list* cares_task_list();
inline IsolateData* isolate_data() const;

inline bool using_domains() const;
inline void set_using_domains(bool value);
Expand Down Expand Up @@ -602,7 +584,6 @@ class Environment {
private:
inline Environment(IsolateData* isolate_data, v8::Local<v8::Context> context);
inline ~Environment();
inline IsolateData* isolate_data() const;

v8::Isolate* const isolate_;
IsolateData* const isolate_data_;
Expand All @@ -613,7 +594,6 @@ class Environment {
AsyncHooks async_hooks_;
DomainFlag domain_flag_;
TickInfo tick_info_;
ArrayBufferAllocatorInfo array_buffer_allocator_info_;
const uint64_t timer_base_;
uv_timer_t cares_timer_handle_;
ares_channel cares_channel_;
Expand Down
17 changes: 6 additions & 11 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -971,11 +971,9 @@ Local<Value> WinapiErrnoException(Isolate* isolate,


void* ArrayBufferAllocator::Allocate(size_t size) {
if (env_ == nullptr ||
!env_->array_buffer_allocator_info()->no_zero_fill() ||
zero_fill_all_buffers)
if (zero_fill_field_ || zero_fill_all_buffers)
return calloc(size, 1);
env_->array_buffer_allocator_info()->reset_fill_flag();
zero_fill_field_ = 1;
return malloc(size);
}

Expand Down Expand Up @@ -4363,8 +4361,8 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
static void StartNodeInstance(void* arg) {
NodeInstanceData* instance_data = static_cast<NodeInstanceData*>(arg);
Isolate::CreateParams params;
ArrayBufferAllocator* array_buffer_allocator = new ArrayBufferAllocator();
params.array_buffer_allocator = array_buffer_allocator;
ArrayBufferAllocator array_buffer_allocator;
params.array_buffer_allocator = &array_buffer_allocator;
#ifdef NODE_ENABLE_VTUNE_PROFILING
params.code_event_handler = vTune::GetVtuneCodeEventHandler();
#endif
Expand All @@ -4385,7 +4383,8 @@ static void StartNodeInstance(void* arg) {
Locker locker(isolate);
Isolate::Scope isolate_scope(isolate);
HandleScope handle_scope(isolate);
IsolateData isolate_data(isolate, instance_data->event_loop());
IsolateData isolate_data(isolate, instance_data->event_loop(),
array_buffer_allocator.zero_fill_field());
Local<Context> context = Context::New(isolate);
Context::Scope context_scope(context);
Environment* env = CreateEnvironment(&isolate_data,
Expand All @@ -4395,8 +4394,6 @@ static void StartNodeInstance(void* arg) {
instance_data->exec_argc(),
instance_data->exec_argv());

array_buffer_allocator->set_env(env);

isolate->SetAbortOnUncaughtExceptionCallback(
ShouldAbortOnUncaughtException);

Expand Down Expand Up @@ -4464,7 +4461,6 @@ static void StartNodeInstance(void* arg) {
__lsan_do_leak_check();
#endif

array_buffer_allocator->set_env(nullptr);
env->Dispose();
env = nullptr;
}
Expand All @@ -4477,7 +4473,6 @@ static void StartNodeInstance(void* arg) {
CHECK_NE(isolate, nullptr);
isolate->Dispose();
isolate = nullptr;
delete array_buffer_allocator;
}

int Start(int argc, char** argv) {
Expand Down
20 changes: 8 additions & 12 deletions src/node_buffer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1224,18 +1224,14 @@ void SetupBufferJS(const FunctionCallbackInfo<Value>& args) {

env->SetMethod(proto, "copy", Copy);

CHECK(args[1]->IsObject());
Local<Object> bObj = args[1].As<Object>();

uint32_t* const fields = env->array_buffer_allocator_info()->fields();
uint32_t const fields_count =
env->array_buffer_allocator_info()->fields_count();

Local<ArrayBuffer> array_buffer =
ArrayBuffer::New(env->isolate(), fields, sizeof(*fields) * fields_count);

bObj->Set(String::NewFromUtf8(env->isolate(), "flags"),
Uint32Array::New(array_buffer, 0, fields_count));
if (auto zero_fill_field = env->isolate_data()->zero_fill_field()) {
CHECK(args[1]->IsObject());
auto binding_object = args[1].As<Object>();
auto array_buffer = ArrayBuffer::New(env->isolate(), zero_fill_field, 1);
auto name = FIXED_ONE_BYTE_STRING(env->isolate(), "zeroFill");
auto value = Uint32Array::New(array_buffer, 0, 1);
CHECK(binding_object->Set(env->context(), name, value).FromJust());
}
}


Expand Down
6 changes: 2 additions & 4 deletions src/node_internals.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,16 +204,14 @@ void ThrowUVException(v8::Isolate* isolate,

class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
public:
ArrayBufferAllocator() : env_(nullptr) { }

inline void set_env(Environment* env) { env_ = env; }
inline uint32_t* zero_fill_field() { return &zero_fill_field_; }

virtual void* Allocate(size_t size); // Defined in src/node.cc
virtual void* AllocateUninitialized(size_t size) { return malloc(size); }
virtual void Free(void* data, size_t) { free(data); }

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

// Clear any domain and/or uncaughtException handlers to force the error's
Expand Down

0 comments on commit 27e84dd

Please sign in to comment.