From c0a9a83c51b7dcaa2ab68b87cc8eadfb59d85a89 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sun, 23 Sep 2018 21:30:08 -0400 Subject: [PATCH] src: refactor FillStatsArray PR-URL: https://github.com/nodejs/node/pull/23793 Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung --- lib/internal/fs/watchers.js | 4 +- src/env.cc | 4 +- src/env.h | 9 ++-- src/node_file.cc | 18 ++++---- src/node_file.h | 91 ++++++++++++++++++++++++++++++++++++- src/node_internals.h | 52 --------------------- src/node_stat_watcher.cc | 15 ++---- 7 files changed, 112 insertions(+), 81 deletions(-) diff --git a/lib/internal/fs/watchers.js b/lib/internal/fs/watchers.js index 0863664110372e..f542a1eab1d177 100644 --- 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 } = internalBinding('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 index ab8c4082f107ab..a1b24a2f15b9fe 100644 --- a/src/env.cc +++ b/src/env.cc @@ -169,8 +169,8 @@ Environment::Environment(IsolateData* isolate_data, trace_category_state_(isolate_, kTraceCategoryCount), stream_base_state_(isolate_, StreamBase::kNumStreamBaseStateFields), 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 index 0b8da0c23c1791..7f35f604946d68 100644 --- a/src/env.h +++ b/src/env.h @@ -82,6 +82,11 @@ struct PackageConfig { }; } // namespace loader +// 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. @@ -712,10 +717,6 @@ class Environment { inline AliasedBuffer* 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>& file_handle_read_wrap_freelist(); diff --git a/src/node_file.cc b/src/node_file.cc index cf3b944a5504bb..fdb0406f60a981 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -426,7 +426,7 @@ void FSReqCallback::Reject(Local reject) { } void FSReqCallback::ResolveStat(const uv_stat_t* stat) { - Resolve(node::FillGlobalStatsArray(env(), stat, use_bigint())); + Resolve(FillGlobalStatsArray(env(), use_bigint(), stat)); } void FSReqCallback::Resolve(Local value) { @@ -899,8 +899,8 @@ static void Stat(const FunctionCallbackInfo& args) { return; // error info is in ctx } - Local arr = node::FillGlobalStatsArray(env, - static_cast(req_wrap_sync.req.ptr), use_bigint); + Local arr = FillGlobalStatsArray(env, use_bigint, + static_cast(req_wrap_sync.req.ptr)); args.GetReturnValue().Set(arr); } } @@ -930,8 +930,8 @@ static void LStat(const FunctionCallbackInfo& args) { return; // error info is in ctx } - Local arr = node::FillGlobalStatsArray(env, - static_cast(req_wrap_sync.req.ptr), use_bigint); + Local arr = FillGlobalStatsArray(env, use_bigint, + static_cast(req_wrap_sync.req.ptr)); args.GetReturnValue().Set(arr); } } @@ -960,8 +960,8 @@ static void FStat(const FunctionCallbackInfo& args) { return; // error info is in ctx } - Local arr = node::FillGlobalStatsArray(env, - static_cast(req_wrap_sync.req.ptr), use_bigint); + Local arr = FillGlobalStatsArray(env, use_bigint, + static_cast(req_wrap_sync.req.ptr)); args.GetReturnValue().Set(arr); } } @@ -2150,8 +2150,8 @@ void Initialize(Local 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 index 31242e1a1b1055..96f8221520bd33 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -148,6 +148,92 @@ class FSReqCallback : public FSReqBase { DISALLOW_COPY_AND_ASSIGN(FSReqCallback); }; +// 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 ::value>> +constexpr NativeT ToNative(uv_timespec_t ts) { + // This template has exactly two specializations below. + static_assert(std::is_arithmetic::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(ts.tv_sec); + const double full_sec = u_sec * 1000.0; + /* NOLINTNEXTLINE(runtime/int) */ + const auto u_nsec = static_cast(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(ts.tv_sec); + const auto full_sec = static_cast(u_sec) * 1000UL; + /* NOLINTNEXTLINE(runtime/int) */ + const auto u_nsec = static_cast(ts.tv_nsec); + const auto full_nsec = static_cast(u_nsec) / 1000'000UL; + return full_sec + full_nsec; +} + +#undef constexpr // end N3652 bug workaround + +template +constexpr void FillStatsArray(AliasedBuffer* 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(s->st_atim)); + fields->SetValue(offset + 11, ToNative(s->st_mtim)); + fields->SetValue(offset + 12, ToNative(s->st_ctim)); + fields->SetValue(offset + 13, ToNative(s->st_birthtim)); +} + +inline Local 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 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& args) override { diff --git a/src/node_internals.h b/src/node_internals.h index cdb7492f437d5c..b6cd628086dcb2 100644 --- 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 @@ -271,57 +270,6 @@ v8::Maybe ProcessEmitDeprecationWarning(Environment* env, const char* warning, const char* deprecation_code); -template -v8::Local FillStatsArray(AliasedBuffer* fields_ptr, - const uv_stat_t* s, int offset = 0) { - AliasedBuffer& 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 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 bootstrapper); void SetupProcessObject(Environment* env, diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index 2ad5ea2ffafd79..7cf06393bb7484 100644 --- 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 #include @@ -80,15 +80,10 @@ void StatWatcher::Callback(uv_fs_poll_t* handle, HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); - Local arr = node::FillGlobalStatsArray(env, curr, - wrap->use_bigint_); - node::FillGlobalStatsArray(env, prev, wrap->use_bigint_, - env->kFsStatsFieldsLength); + Local arr = fs::FillGlobalStatsArray(env, wrap->use_bigint_, curr); + USE(fs::FillGlobalStatsArray(env, wrap->use_bigint_, prev, true)); - Local argv[2] { - Integer::New(env->isolate(), status), - arr - }; + Local argv[2] = { Integer::New(env->isolate(), status), arr }; wrap->MakeCallback(env->onchange_string(), arraysize(argv), argv); }