From 27e84ddd4e10cdb40d6e89d872277abe16830eb0 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 31 May 2016 20:58:31 +0200 Subject: [PATCH] lib,src: clean up ArrayBufferAllocator 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: https://github.com/nodejs/node/pull/7082 Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- lib/buffer.js | 18 ++++++++---------- src/debug-agent.cc | 3 ++- src/env-inl.h | 36 ++++++++---------------------------- src/env.h | 30 +++++------------------------- src/node.cc | 17 ++++++----------- src/node_buffer.cc | 20 ++++++++------------ src/node_internals.h | 6 ++---- 7 files changed, 39 insertions(+), 91 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index 2472390086057c..54096aa99410c1 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -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. +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() { diff --git a/src/debug-agent.cc b/src/debug-agent.cc index a8797a5bcf4887..e5dc346dc6a354 100644 --- a/src/debug-agent.cc +++ b/src/debug-agent.cc @@ -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::New(isolate); Context::Scope context_scope(context); diff --git a/src/env-inl.h b/src/env-inl.h index 9836f14da42fbc..008e5103b16c6e 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -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 ## _( \ @@ -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; } @@ -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 context) { Environment* env = new Environment(isolate_data, context); @@ -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_; } diff --git a/src/env.h b/src/env.h index a6aea815fcd75a..d252e5db6ce264 100644 --- a/src/env.h +++ b/src/env.h @@ -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) @@ -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); }; @@ -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); @@ -497,7 +479,6 @@ 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); @@ -505,6 +486,7 @@ class Environment { 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); @@ -602,7 +584,6 @@ class Environment { private: inline Environment(IsolateData* isolate_data, v8::Local context); inline ~Environment(); - inline IsolateData* isolate_data() const; v8::Isolate* const isolate_; IsolateData* const isolate_data_; @@ -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_; diff --git a/src/node.cc b/src/node.cc index 2f64e36e5935e6..3c508183c922d8 100644 --- a/src/node.cc +++ b/src/node.cc @@ -971,11 +971,9 @@ Local 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); } @@ -4363,8 +4361,8 @@ Environment* CreateEnvironment(IsolateData* isolate_data, static void StartNodeInstance(void* arg) { NodeInstanceData* instance_data = static_cast(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 @@ -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::New(isolate); Context::Scope context_scope(context); Environment* env = CreateEnvironment(&isolate_data, @@ -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); @@ -4464,7 +4461,6 @@ static void StartNodeInstance(void* arg) { __lsan_do_leak_check(); #endif - array_buffer_allocator->set_env(nullptr); env->Dispose(); env = nullptr; } @@ -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) { diff --git a/src/node_buffer.cc b/src/node_buffer.cc index bb77387e8f87b7..bb94263ccd5b64 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -1224,18 +1224,14 @@ void SetupBufferJS(const FunctionCallbackInfo& args) { env->SetMethod(proto, "copy", Copy); - CHECK(args[1]->IsObject()); - Local bObj = args[1].As(); - - uint32_t* const fields = env->array_buffer_allocator_info()->fields(); - uint32_t const fields_count = - env->array_buffer_allocator_info()->fields_count(); - - Local 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(); + 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()); + } } diff --git a/src/node_internals.h b/src/node_internals.h index 64134d9ab8d9b7..56606613ddfeb9 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -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