src: refactor FillStatsArray

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
nodejs · Jan 13, 2019 · acf7e7d · acf7e7d
1 parent 70e7d53
commit acf7e7d
Show file tree

Hide file tree

Showing 7 changed files with 112 additions and 81 deletions.
diff --git a/lib/internal/fs/watchers.js b/lib/internal/fs/watchers.js
@@ -2,7 +2,7 @@
 
 const errors = require('internal/errors');
 const {
-  kFsStatsFieldsLength,
+  kFsStatsFieldsNumber,
   StatWatcher: _StatWatcher
 } = process.binding('fs');
 const { FSEvent } = process.binding('fs_event_wrap');
@@ -48,7 +48,7 @@ function onchange(newStatus, stats) {
 
   self[kOldStatus] = newStatus;
   self.emit('change', getStatsFromBinding(stats),
-            getStatsFromBinding(stats, kFsStatsFieldsLength));
+            getStatsFromBinding(stats, kFsStatsFieldsNumber));
 }
 
 // FIXME(joyeecheung): this method is not documented.

diff --git a/src/env.cc b/src/env.cc
@@ -118,8 +118,8 @@ Environment::Environment(IsolateData* isolate_data,
       makecallback_cntr_(0),
       should_abort_on_uncaught_toggle_(isolate_, 1),
       http_parser_buffer_(nullptr),
-      fs_stats_field_array_(isolate_, kFsStatsFieldsLength * 2),
-      fs_stats_field_bigint_array_(isolate_, kFsStatsFieldsLength * 2),
+      fs_stats_field_array_(isolate_, kFsStatsBufferLength),
+      fs_stats_field_bigint_array_(isolate_, kFsStatsBufferLength),
       context_(context->GetIsolate(), context) {
   // We'll be creating new objects so make sure we've entered the context.
   v8::HandleScope handle_scope(isolate());

diff --git a/src/env.h b/src/env.h
@@ -84,6 +84,11 @@ struct PackageConfig {
 #define NODE_PUSH_VAL_TO_ARRAY_MAX 8
 #endif
 
+// Stat fields buffers contain twice the number of entries in an uv_stat_t
+// because `fs.StatWatcher` needs room to store 2 `fs.Stats` instances.
+constexpr size_t kFsStatsFieldsNumber = 14;
+constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2;
+
 // PER_ISOLATE_* macros: We have a lot of per-isolate properties
 // and adding and maintaining their getters and setters by hand would be
 // difficult so let's make the preprocessor generate them for us.
@@ -698,10 +703,6 @@ class Environment {
   inline AliasedBuffer<uint64_t, v8::BigUint64Array>*
       fs_stats_field_bigint_array();
 
-  // stat fields contains twice the number of entries because `fs.StatWatcher`
-  // needs room to store data for *two* `fs.Stats` instances.
-  static const int kFsStatsFieldsLength = 14;
-
   inline std::vector<std::unique_ptr<fs::FileHandleReadWrap>>&
       file_handle_read_wrap_freelist();
 

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -426,7 +426,7 @@ void FSReqWrap::Reject(Local<Value> reject) {
 }
 
 void FSReqWrap::ResolveStat(const uv_stat_t* stat) {
-  Resolve(node::FillGlobalStatsArray(env(), stat, use_bigint()));
+  Resolve(FillGlobalStatsArray(env(), use_bigint(), stat));
 }
 
 void FSReqWrap::Resolve(Local<Value> value) {
@@ -949,8 +949,8 @@ static void Stat(const FunctionCallbackInfo<Value>& args) {
       return;  // error info is in ctx
     }
 
-    Local<Value> arr = node::FillGlobalStatsArray(env,
-        static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr), use_bigint);
+    Local<Value> arr = FillGlobalStatsArray(env, use_bigint,
+        static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
     args.GetReturnValue().Set(arr);
   }
 }
@@ -980,8 +980,8 @@ static void LStat(const FunctionCallbackInfo<Value>& args) {
       return;  // error info is in ctx
     }
 
-    Local<Value> arr = node::FillGlobalStatsArray(env,
-        static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr), use_bigint);
+    Local<Value> arr = FillGlobalStatsArray(env, use_bigint,
+        static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
     args.GetReturnValue().Set(arr);
   }
 }
@@ -1010,8 +1010,8 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {
       return;  // error info is in ctx
     }
 
-    Local<Value> arr = node::FillGlobalStatsArray(env,
-        static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr), use_bigint);
+    Local<Value> arr = FillGlobalStatsArray(env, use_bigint,
+        static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
     args.GetReturnValue().Set(arr);
   }
 }
@@ -2237,8 +2237,8 @@ void Initialize(Local<Object> target,
   env->SetMethod(target, "mkdtemp", Mkdtemp);
 
   target->Set(context,
-              FIXED_ONE_BYTE_STRING(isolate, "kFsStatsFieldsLength"),
-              Integer::New(isolate, env->kFsStatsFieldsLength))
+              FIXED_ONE_BYTE_STRING(isolate, "kFsStatsFieldsNumber"),
+              Integer::New(isolate, kFsStatsFieldsNumber))
         .FromJust();
 
   target->Set(context,

diff --git a/src/node_file.h b/src/node_file.h
@@ -148,6 +148,92 @@ class FSReqWrap : public FSReqBase {
   DISALLOW_COPY_AND_ASSIGN(FSReqWrap);
 };
 
+// Wordaround a GCC4.9 bug that C++14 N3652 was not implemented
+// Refs: https://www.gnu.org/software/gcc/projects/cxx-status.html#cxx14
+// Refs: https://isocpp.org/files/papers/N3652.html
+#if __cpp_constexpr < 201304
+#  define constexpr inline
+#endif
+
+template <typename NativeT,
+          // SFINAE limit NativeT to arithmetic types
+          typename = std::enable_if<std::is_arithmetic<NativeT>::value>>
+constexpr NativeT ToNative(uv_timespec_t ts) {
+  // This template has exactly two specializations below.
+  static_assert(std::is_arithmetic<NativeT>::value == false, "Not implemented");
+  UNREACHABLE();
+}
+
+template <>
+constexpr double ToNative(uv_timespec_t ts) {
+  // We need to do a static_cast since the original FS values are ulong.
+  /* NOLINTNEXTLINE(runtime/int) */
+  const auto u_sec = static_cast<unsigned long>(ts.tv_sec);
+  const double full_sec = u_sec * 1000.0;
+  /* NOLINTNEXTLINE(runtime/int) */
+  const auto u_nsec = static_cast<unsigned long>(ts.tv_nsec);
+  const double full_nsec = u_nsec / 1000'000.0;
+  return full_sec + full_nsec;
+}
+
+template <>
+constexpr uint64_t ToNative(uv_timespec_t ts) {
+  // We need to do a static_cast since the original FS values are ulong.
+  /* NOLINTNEXTLINE(runtime/int) */
+  const auto u_sec = static_cast<unsigned long>(ts.tv_sec);
+  const auto full_sec = static_cast<uint64_t>(u_sec) * 1000UL;
+  /* NOLINTNEXTLINE(runtime/int) */
+  const auto u_nsec = static_cast<unsigned long>(ts.tv_nsec);
+  const auto full_nsec = static_cast<uint64_t>(u_nsec) / 1000'000UL;
+  return full_sec + full_nsec;
+}
+
+#undef constexpr  // end N3652 bug workaround
+
+template <typename NativeT, typename V8T>
+constexpr void FillStatsArray(AliasedBuffer<NativeT, V8T>* fields,
+                              const uv_stat_t* s, const size_t offset = 0) {
+  fields->SetValue(offset + 0, s->st_dev);
+  fields->SetValue(offset + 1, s->st_mode);
+  fields->SetValue(offset + 2, s->st_nlink);
+  fields->SetValue(offset + 3, s->st_uid);
+  fields->SetValue(offset + 4, s->st_gid);
+  fields->SetValue(offset + 5, s->st_rdev);
+#if defined(__POSIX__)
+  fields->SetValue(offset + 6, s->st_blksize);
+#else
+  fields->SetValue(offset + 6, 0);
+#endif
+  fields->SetValue(offset + 7, s->st_ino);
+  fields->SetValue(offset + 8, s->st_size);
+#if defined(__POSIX__)
+  fields->SetValue(offset + 9, s->st_blocks);
+#else
+  fields->SetValue(offset + 9, 0);
+#endif
+// Dates.
+  fields->SetValue(offset + 10, ToNative<NativeT>(s->st_atim));
+  fields->SetValue(offset + 11, ToNative<NativeT>(s->st_mtim));
+  fields->SetValue(offset + 12, ToNative<NativeT>(s->st_ctim));
+  fields->SetValue(offset + 13, ToNative<NativeT>(s->st_birthtim));
+}
+
+inline Local<Value> FillGlobalStatsArray(Environment* env,
+                                         const bool use_bigint,
+                                         const uv_stat_t* s,
+                                         const bool second = false) {
+  const ptrdiff_t offset = second ? kFsStatsFieldsNumber : 0;
+  if (use_bigint) {
+    auto* const arr = env->fs_stats_field_bigint_array();
+    FillStatsArray(arr, s, offset);
+    return arr->GetJSArray();
+  } else {
+    auto* const arr = env->fs_stats_field_array();
+    FillStatsArray(arr, s, offset);
+    return arr->GetJSArray();
+  }
+}
+
 template <typename NativeT = double, typename V8T = v8::Float64Array>
 class FSReqPromise : public FSReqBase {
  public:
@@ -157,7 +243,7 @@ class FSReqPromise : public FSReqBase {
                       ->NewInstance(env->context()).ToLocalChecked(),
                   AsyncWrap::PROVIDER_FSREQPROMISE,
                   use_bigint),
-        stats_field_array_(env->isolate(), env->kFsStatsFieldsLength) {
+        stats_field_array_(env->isolate(), kFsStatsFieldsNumber) {
     auto resolver = Promise::Resolver::New(env->context()).ToLocalChecked();
     object()->Set(env->context(), env->promise_string(),
                   resolver).FromJust();
@@ -191,7 +277,8 @@ class FSReqPromise : public FSReqBase {
   }
 
   void ResolveStat(const uv_stat_t* stat) override {
-    Resolve(node::FillStatsArray(&stats_field_array_, stat));
+    FillStatsArray(&stats_field_array_, stat);
+    Resolve(stats_field_array_.GetJSArray());
   }
 
   void SetReturnValue(const FunctionCallbackInfo<Value>& args) override {

diff --git a/src/node_internals.h b/src/node_internals.h
@@ -32,7 +32,6 @@
 #include "uv.h"
 #include "v8.h"
 #include "tracing/trace_event.h"
-#include "node_perf_common.h"
 #include "node_api.h"
 
 #include <stdint.h>
@@ -308,57 +307,6 @@ v8::Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
                                               const char* warning,
                                               const char* deprecation_code);
 
-template <typename NativeT, typename V8T>
-v8::Local<v8::Value> FillStatsArray(AliasedBuffer<NativeT, V8T>* fields_ptr,
-                    const uv_stat_t* s, int offset = 0) {
-  AliasedBuffer<NativeT, V8T>& fields = *fields_ptr;
-  fields[offset + 0] = s->st_dev;
-  fields[offset + 1] = s->st_mode;
-  fields[offset + 2] = s->st_nlink;
-  fields[offset + 3] = s->st_uid;
-  fields[offset + 4] = s->st_gid;
-  fields[offset + 5] = s->st_rdev;
-#if defined(__POSIX__)
-  fields[offset + 6] = s->st_blksize;
-#else
-  fields[offset + 6] = 0;
-#endif
-  fields[offset + 7] = s->st_ino;
-  fields[offset + 8] = s->st_size;
-#if defined(__POSIX__)
-  fields[offset + 9] = s->st_blocks;
-#else
-  fields[offset + 9] = 0;
-#endif
-// Dates.
-// NO-LINT because the fields are 'long' and we just want to cast to `unsigned`
-#define X(idx, name)                                                    \
-  /* NOLINTNEXTLINE(runtime/int) */                                     \
-  fields[offset + idx] = ((unsigned long)(s->st_##name.tv_sec) * 1e3) + \
-  /* NOLINTNEXTLINE(runtime/int) */                                     \
-                ((unsigned long)(s->st_##name.tv_nsec) / 1e6);          \
-
-  X(10, atim)
-  X(11, mtim)
-  X(12, ctim)
-  X(13, birthtim)
-#undef X
-
-  return fields_ptr->GetJSArray();
-}
-
-inline v8::Local<v8::Value> FillGlobalStatsArray(Environment* env,
-                                                 const uv_stat_t* s,
-                                                 bool use_bigint = false,
-                                                 int offset = 0) {
-  if (use_bigint) {
-    return node::FillStatsArray(
-        env->fs_stats_field_bigint_array(), s, offset);
-  } else {
-    return node::FillStatsArray(env->fs_stats_field_array(), s, offset);
-  }
-}
-
 void SetupBootstrapObject(Environment* env,
                           v8::Local<v8::Object> bootstrapper);
 void SetupProcessObject(Environment* env,

diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc
@@ -22,8 +22,8 @@
 #include "node_stat_watcher.h"
 #include "node_internals.h"
 #include "async_wrap-inl.h"
-#include "env-inl.h"
-#include "util-inl.h"
+#include "env.h"
+#include "node_file.h"
 
 #include <string.h>
 #include <stdlib.h>
@@ -80,15 +80,10 @@ void StatWatcher::Callback(uv_fs_poll_t* handle,
   HandleScope handle_scope(env->isolate());
   Context::Scope context_scope(env->context());
 
-  Local<Value> arr = node::FillGlobalStatsArray(env, curr,
-                                                wrap->use_bigint_);
-  node::FillGlobalStatsArray(env, prev, wrap->use_bigint_,
-                             env->kFsStatsFieldsLength);
+  Local<Value> arr = fs::FillGlobalStatsArray(env, wrap->use_bigint_, curr);
+  USE(fs::FillGlobalStatsArray(env, wrap->use_bigint_, prev, true));
 
-  Local<Value> argv[2] {
-    Integer::New(env->isolate(), status),
-    arr
-  };
+  Local<Value> argv[2] = { Integer::New(env->isolate(), status), arr };
   wrap->MakeCallback(env->onchange_string(), arraysize(argv), argv);
 }