src: replace pushValueToArray with pure C++ API

benchmark/fs/bench-readdir.js

@@ -5,16 +5,19 @@ const fs = require('fs');
 const path = require('path');
 
 const bench = common.createBenchmark(main, {
-  n: [1e4],
+  n: [10],
+  dir: [ 'lib', 'test/parallel'],
+  withFileTypes: ['true', 'false']
 });
 
-
-function main({ n }) {
+function main({ n, dir, withFileTypes }) {
+  withFileTypes = withFileTypes === 'true';
   bench.start();
   (function r(cntr) {
     if (cntr-- <= 0)
       return bench.end(n);
-    fs.readdir(path.resolve(__dirname, '../../lib/'), function() {
+    const fullPath = path.resolve(__dirname, '../../', dir);
+    fs.readdir(fullPath, { withFileTypes }, function() {
       r(cntr);
     });
   }(n));

benchmark/fs/bench-readdirSync.js

@@ -5,14 +5,18 @@ const fs = require('fs');
 const path = require('path');
 
 const bench = common.createBenchmark(main, {
-  n: [1e4],
+  n: [10],
+  dir: [ 'lib', 'test/parallel'],
+  withFileTypes: ['true', 'false']
 });
 
 
-function main({ n }) {
+function main({ n, dir, withFileTypes }) {
+  withFileTypes = withFileTypes === 'true';
   bench.start();
   for (var i = 0; i < n; i++) {
-    fs.readdirSync(path.resolve(__dirname, '../../lib/'));
+    const fullPath = path.resolve(__dirname, '../../', dir);
+    fs.readdirSync(fullPath, { withFileTypes });
   }
   bench.end(n);
 }

common.gypi

@@ -32,7 +32,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.9',
+    'v8_embedder_string': '-node.10',
 
     # Enable disassembler for `--print-code` v8 options
     'v8_enable_disassembler': 1,

deps/v8/include/v8.h

@@ -3779,6 +3779,12 @@ class V8_EXPORT Array : public Object {
    */
   static Local<Array> New(Isolate* isolate, int length = 0);
 
+  /**
+   * Creates a JavaScript array out of a Local<Value> array in C++
+   * with a known length.
+   */
+  static Local<Array> New(Isolate* isolate, Local<Value>* elements,
+                          size_t length);
   V8_INLINE static Array* Cast(Value* obj);
  private:
   Array();

deps/v8/src/api.cc

@@ -6981,6 +6981,23 @@ Local<v8::Array> v8::Array::New(Isolate* isolate, int length) {
   return Utils::ToLocal(obj);
 }
 
+Local<v8::Array> v8::Array::New(Isolate* isolate, Local<Value>* elements,
+                                size_t length) {
+  i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  i::Factory* factory = i_isolate->factory();
+  LOG_API(i_isolate, Array, New);
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+  int len = static_cast<int>(length);
+
+  i::Handle<i::FixedArray> result = factory->NewFixedArray(len);
+  for (int i = 0; i < len; i++) {
+    i::Handle<i::Object> element = Utils::OpenHandle(*elements[i]);
+    result->set(i, *element);
+  }
+
+  return Utils::ToLocal(
+      factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS, len));
+}
 
 uint32_t v8::Array::Length() const {
   i::Handle<i::JSArray> obj = Utils::OpenHandle(this);

deps/v8/test/cctest/test-api.cc

@@ -5247,6 +5247,22 @@ THREADED_TEST(Array) {
   CHECK_EQ(27u, array->Length());
   array = v8::Array::New(context->GetIsolate(), -27);
   CHECK_EQ(0u, array->Length());
+
+  std::vector<Local<Value>> vector = {v8_num(1), v8_num(2), v8_num(3)};
+  array = v8::Array::New(context->GetIsolate(), vector.data(), vector.size());
+  CHECK_EQ(vector.size(), array->Length());
+  CHECK_EQ(1, arr->Get(context.local(), 0)
+                  .ToLocalChecked()
+                  ->Int32Value(context.local())
+                  .FromJust());
+  CHECK_EQ(2, arr->Get(context.local(), 1)
+                  .ToLocalChecked()
+                  ->Int32Value(context.local())
+                  .FromJust());
+  CHECK_EQ(3, arr->Get(context.local(), 2)
+                  .ToLocalChecked()
+                  ->Int32Value(context.local())
+                  .FromJust());
 }
 
 

src/node_file.cc

@@ -571,10 +571,7 @@ void AfterScanDir(uv_fs_t* req) {
   Environment* env = req_wrap->env();
   Local<Value> error;
   int r;
-  Local<Array> names = Array::New(env->isolate(), 0);
-  Local<Function> fn = env->push_values_to_array_function();
-  Local<Value> name_argv[NODE_PUSH_VAL_TO_ARRAY_MAX];
-  size_t name_idx = 0;
+  std::vector<Local<Value>> name_v;
 
   for (int i = 0; ; i++) {
     uv_dirent_t ent;
@@ -596,24 +593,10 @@ void AfterScanDir(uv_fs_t* req) {
     if (filename.IsEmpty())
       return req_wrap->Reject(error);
 
-    name_argv[name_idx++] = filename.ToLocalChecked();
-
-    if (name_idx >= arraysize(name_argv)) {
-      MaybeLocal<Value> ret = fn->Call(env->context(), names, name_idx,
-                                       name_argv);
-      if (ret.IsEmpty()) {
-        return;
-      }
-      name_idx = 0;
-    }
-  }
-
-  if (name_idx > 0) {
-    fn->Call(env->context(), names, name_idx, name_argv)
-      .ToLocalChecked();
+    name_v.push_back(filename.ToLocalChecked());
   }
 
-  req_wrap->Resolve(names);
+  req_wrap->Resolve(Array::New(env->isolate(), name_v.data(), name_v.size()));
 }
 
 void AfterScanDirWithTypes(uv_fs_t* req) {
@@ -628,13 +611,9 @@ void AfterScanDirWithTypes(uv_fs_t* req) {
   Isolate* isolate = env->isolate();
   Local<Value> error;
   int r;
-  Local<Array> names = Array::New(isolate, 0);
-  Local<Function> fn = env->push_values_to_array_function();
-  Local<Value> name_argv[NODE_PUSH_VAL_TO_ARRAY_MAX];
-  size_t name_idx = 0;
-  Local<Value> types = Array::New(isolate, 0);
-  Local<Value> type_argv[NODE_PUSH_VAL_TO_ARRAY_MAX];
-  size_t type_idx = 0;
+
+  std::vector<Local<Value>> name_v;
+  std::vector<Local<Value>> type_v;
 
   for (int i = 0; ; i++) {
     uv_dirent_t ent;
@@ -656,48 +635,13 @@ void AfterScanDirWithTypes(uv_fs_t* req) {
     if (filename.IsEmpty())
       return req_wrap->Reject(error);
 
-    name_argv[name_idx++] = filename.ToLocalChecked();
-
-    if (name_idx >= arraysize(name_argv)) {
-      MaybeLocal<Value> ret = fn->Call(env->context(), names, name_idx,
-                                       name_argv);
-      if (ret.IsEmpty()) {
-        return;
-      }
-      name_idx = 0;
-    }
-
-    type_argv[type_idx++] = Integer::New(isolate, ent.type);
-
-    if (type_idx >= arraysize(type_argv)) {
-      MaybeLocal<Value> ret = fn->Call(env->context(), types, type_idx,
-          type_argv);
-      if (ret.IsEmpty()) {
-        return;
-      }
-      type_idx = 0;
-    }
-  }
-
-  if (name_idx > 0) {
-    MaybeLocal<Value> ret = fn->Call(env->context(), names, name_idx,
-        name_argv);
-    if (ret.IsEmpty()) {
-      return;
-    }
-  }
-
-  if (type_idx > 0) {
-    MaybeLocal<Value> ret = fn->Call(env->context(), types, type_idx,
-        type_argv);
-    if (ret.IsEmpty()) {
-      return;
-    }
+    name_v.push_back(filename.ToLocalChecked());
+    type_v.push_back(Integer::New(isolate, ent.type));
   }
 
   Local<Array> result = Array::New(isolate, 2);
-  result->Set(0, names);
-  result->Set(1, types);
+  result->Set(0, Array::New(isolate, name_v.data(), name_v.size()));
+  result->Set(1, Array::New(isolate, type_v.data(), type_v.size()));
   req_wrap->Resolve(result);
 }
 
@@ -1497,18 +1441,8 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
 
     CHECK_GE(req_wrap_sync.req.result, 0);
     int r;
-    Local<Array> names = Array::New(isolate, 0);
-    Local<Function> fn = env->push_values_to_array_function();
-    Local<Value> name_v[NODE_PUSH_VAL_TO_ARRAY_MAX];
-    size_t name_idx = 0;
-
-    Local<Value> types;
-    Local<Value> type_v[NODE_PUSH_VAL_TO_ARRAY_MAX];
-    size_t type_idx;
-    if (with_types) {
-      types = Array::New(isolate, 0);
-      type_idx = 0;
-    }
+    std::vector<Local<Value>> name_v;
+    std::vector<Local<Value>> type_v;
 
     for (int i = 0; ; i++) {
       uv_dirent_t ent;
@@ -1537,49 +1471,19 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
         return;
       }
 
-      name_v[name_idx++] = filename.ToLocalChecked();
-
-      if (name_idx >= arraysize(name_v)) {
-        MaybeLocal<Value> ret = fn->Call(env->context(), names, name_idx,
-                                         name_v);
-        if (ret.IsEmpty()) {
-          return;
-        }
-        name_idx = 0;
-      }
+      name_v.push_back(filename.ToLocalChecked());
 
       if (with_types) {
-        type_v[type_idx++] = Integer::New(isolate, ent.type);
-
-        if (type_idx >= arraysize(type_v)) {
-          MaybeLocal<Value> ret = fn->Call(env->context(), types, type_idx,
-              type_v);
-          if (ret.IsEmpty()) {
-            return;
-          }
-          type_idx = 0;
-        }
+        type_v.push_back(Integer::New(isolate, ent.type));
       }
     }
 
-    if (name_idx > 0) {
-      MaybeLocal<Value> ret = fn->Call(env->context(), names, name_idx, name_v);
-      if (ret.IsEmpty()) {
-        return;
-      }
-    }
-
-    if (with_types && type_idx > 0) {
-      MaybeLocal<Value> ret = fn->Call(env->context(), types, type_idx, type_v);
-      if (ret.IsEmpty()) {
-        return;
-      }
-    }
 
+    Local<Array> names = Array::New(isolate, name_v.data(), name_v.size());
     if (with_types) {
       Local<Array> result = Array::New(isolate, 2);
       result->Set(0, names);
-      result->Set(1, types);
+      result->Set(1, Array::New(isolate, type_v.data(), type_v.size()));
       args.GetReturnValue().Set(result);
     } else {
       args.GetReturnValue().Set(names);

test/parallel/test-benchmark-fs.js

@@ -16,5 +16,7 @@ runBenchmark('fs', [
   'statType=fstat',
   'statSyncType=fstatSync',
   'encodingType=buf',
-  'filesize=1024'
+  'filesize=1024',
+  'dir=.github',
+  'withFileTypes=false'
 ], { NODE_TMPDIR: tmpdir.path, NODEJS_BENCHMARK_ZERO_ALLOWED: 1 });