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 <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

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 {
+  if (noZeroFill)
+    zeroFill[0] = 0;  // Reset by the runtime.
   const ui8 = new Uint8Array(size);
   Object.setPrototypeOf(ui8, Buffer.prototype);
   return ui8;
-  } finally {
-    flags[kNoZeroFill] = 0;
-  }
 }
 
 function createPool() {

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 = Context::New(isolate);
 
     Context::Scope context_scope(context);

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<v8::Context> 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_;
 }

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<v8::Context> 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_;

src/node.cc

@@ -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);
 }
 
@@ -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
@@ -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,
@@ -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) {

src/node_buffer.cc

@@ -1224,18 +1224,14 @@ void SetupBufferJS(const FunctionCallbackInfo<Value>& args) {
 
   env->SetMethod(proto, "copy", Copy);
 
+  if (auto zero_fill_field = env->isolate_data()->zero_fill_field()) {
     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));
+    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());
+  }
 }
 
 

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