From f7049a20068dc8a7e904b7cdd3d5b307b595dd3a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 31 Mar 2018 04:02:57 +0800 Subject: fs: refactor stats array to be more generic - Pass kFsStatsFieldsLength between JS and C++ instead of using the magic number 14 - Pass the global stats array to the completion callback of asynchronous FSReqWrap similar to how the stats arrays are passed to the FSReqPromise resolvers - Abstract the stats converter and take an offset to compute the old stats in fs.watchFile - Use templates in node::FillStatsArray and FSReqPromise in preparation for BigInt intergration - Put the global stat array filler in node_internals.h because it is shared by node_file.cc and node_stat_watcher.cc PR-URL: https://github.com/nodejs/node/pull/19714 Reviewed-By: James M Snell --- lib/fs.js | 29 +++++------ lib/fs/promises.js | 15 +++--- lib/internal/fs.js | 32 ++++++++---- src/env.cc | 2 +- src/env.h | 7 +-- src/node_file.cc | 126 +++++++---------------------------------------- src/node_file.h | 64 +++++++++++++++++++----- src/node_internals.h | 44 ++++++++++++++++- src/node_stat_watcher.cc | 12 +++-- 9 files changed, 168 insertions(+), 163 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 2de5f8dd6e..85c939cc4a 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -43,7 +43,7 @@ const { } = errors.codes; const { Readable, Writable } = require('stream'); const EventEmitter = require('events'); -const { FSReqWrap, statValues } = binding; +const { FSReqWrap, statValues, kFsStatsFieldsLength } = binding; const { FSEvent } = process.binding('fs_event_wrap'); const internalFS = require('internal/fs'); const { getPathFromURL } = require('internal/url'); @@ -56,7 +56,8 @@ const { nullCheck, preprocessSymlinkDestination, Stats, - statsFromValues, + getStatsFromBinding, + getStatsFromGlobalBinding, stringToFlags, stringToSymlinkType, toUnixTimestamp, @@ -145,9 +146,9 @@ function makeStatsCallback(cb) { throw new ERR_INVALID_CALLBACK(); } - return function(err) { + return function(err, stats) { if (err) return cb(err); - cb(err, statsFromValues()); + cb(err, getStatsFromBinding(stats)); }; } @@ -891,7 +892,7 @@ fs.fstatSync = function(fd) { const ctx = { fd }; binding.fstat(fd, undefined, ctx); handleErrorFromBinding(ctx); - return statsFromValues(); + return getStatsFromGlobalBinding(); }; fs.lstatSync = function(path) { @@ -900,7 +901,7 @@ fs.lstatSync = function(path) { const ctx = { path }; binding.lstat(pathModule.toNamespacedPath(path), undefined, ctx); handleErrorFromBinding(ctx); - return statsFromValues(); + return getStatsFromGlobalBinding(); }; fs.statSync = function(path) { @@ -909,7 +910,7 @@ fs.statSync = function(path) { const ctx = { path }; binding.stat(pathModule.toNamespacedPath(path), undefined, ctx); handleErrorFromBinding(ctx); - return statsFromValues(); + return getStatsFromGlobalBinding(); }; fs.readlink = function(path, options, callback) { @@ -1420,15 +1421,6 @@ function emitStop(self) { self.emit('stop'); } -function statsFromPrevValues() { - return new Stats(statValues[14], statValues[15], statValues[16], - statValues[17], statValues[18], statValues[19], - statValues[20] < 0 ? undefined : statValues[20], - statValues[21], statValues[22], - statValues[23] < 0 ? undefined : statValues[23], - statValues[24], statValues[25], statValues[26], - statValues[27]); -} function StatWatcher() { EventEmitter.call(this); @@ -1439,13 +1431,14 @@ function StatWatcher() { // the sake of backwards compatibility var oldStatus = -1; - this._handle.onchange = function(newStatus) { + this._handle.onchange = function(newStatus, stats) { if (oldStatus === -1 && newStatus === -1 && statValues[2/* new nlink */] === statValues[16/* old nlink */]) return; oldStatus = newStatus; - self.emit('change', statsFromValues(), statsFromPrevValues()); + self.emit('change', getStatsFromBinding(stats), + getStatsFromBinding(stats, kFsStatsFieldsLength)); }; this._handle.onstop = function() { diff --git a/lib/fs/promises.js b/lib/fs/promises.js index b4ee81c1a7..7a0389978f 100644 --- a/lib/fs/promises.js +++ b/lib/fs/promises.js @@ -23,11 +23,11 @@ const { isUint8Array } = require('internal/util/types'); const { copyObject, getOptions, + getStatsFromBinding, isUint32, modeNum, nullCheck, preprocessSymlinkDestination, - statsFromValues, stringToFlags, stringToSymlinkType, toUnixTimestamp, @@ -332,21 +332,24 @@ async function symlink(target, path, type_) { async function fstat(handle) { validateFileHandle(handle); - return statsFromValues(await binding.fstat(handle.fd, kUsePromises)); + const result = await binding.fstat(handle.fd, kUsePromises); + return getStatsFromBinding(result); } async function lstat(path) { path = getPathFromURL(path); validatePath(path); - return statsFromValues( - await binding.lstat(pathModule.toNamespacedPath(path), kUsePromises)); + const result = await binding.lstat(pathModule.toNamespacedPath(path), + kUsePromises); + return getStatsFromBinding(result); } async function stat(path) { path = getPathFromURL(path); validatePath(path); - return statsFromValues( - await binding.stat(pathModule.toNamespacedPath(path), kUsePromises)); + const result = await binding.stat(pathModule.toNamespacedPath(path), + kUsePromises); + return getStatsFromBinding(result); } async function link(existingPath, newPath) { diff --git a/lib/internal/fs.js b/lib/internal/fs.js index 4834d92fa0..6ff4152aa4 100644 --- a/lib/internal/fs.js +++ b/lib/internal/fs.js @@ -134,6 +134,10 @@ function preprocessSymlinkDestination(path, type, linkPath) { } } +function dateFromNumeric(num) { + return new Date(num + 0.5); +} + // Constructor for file stats. function Stats( dev, @@ -165,10 +169,10 @@ function Stats( this.mtimeMs = mtim_msec; this.ctimeMs = ctim_msec; this.birthtimeMs = birthtim_msec; - this.atime = new Date(atim_msec + 0.5); - this.mtime = new Date(mtim_msec + 0.5); - this.ctime = new Date(ctim_msec + 0.5); - this.birthtime = new Date(birthtim_msec + 0.5); + this.atime = dateFromNumeric(atim_msec); + this.mtime = dateFromNumeric(mtim_msec); + this.ctime = dateFromNumeric(ctim_msec); + this.birthtime = dateFromNumeric(birthtim_msec); } Stats.prototype._checkModeProperty = function(property) { @@ -203,11 +207,18 @@ Stats.prototype.isSocket = function() { return this._checkModeProperty(S_IFSOCK); }; -function statsFromValues(stats = statValues) { - return new Stats(stats[0], stats[1], stats[2], stats[3], stats[4], stats[5], - stats[6] < 0 ? undefined : stats[6], stats[7], stats[8], - stats[9] < 0 ? undefined : stats[9], stats[10], stats[11], - stats[12], stats[13]); +function getStatsFromBinding(stats, offset = 0) { + return new Stats(stats[0 + offset], stats[1 + offset], stats[2 + offset], + stats[3 + offset], stats[4 + offset], stats[5 + offset], + stats[6 + offset] < 0 ? undefined : stats[6 + offset], + stats[7 + offset], stats[8 + offset], + stats[9 + offset] < 0 ? undefined : stats[9 + offset], + stats[10 + offset], stats[11 + offset], + stats[12 + offset], stats[13 + offset]); +} + +function getStatsFromGlobalBinding(offset = 0) { + return getStatsFromBinding(statValues, offset); } function stringToFlags(flags) { @@ -424,7 +435,8 @@ module.exports = { nullCheck, preprocessSymlinkDestination, realpathCacheKey: Symbol('realpathCacheKey'), - statsFromValues, + getStatsFromBinding, + getStatsFromGlobalBinding, stringToFlags, stringToSymlinkType, Stats, diff --git a/src/env.cc b/src/env.cc index 16a24260f6..4be70036e3 100644 --- a/src/env.cc +++ b/src/env.cc @@ -104,7 +104,7 @@ Environment::Environment(IsolateData* isolate_data, #endif handle_cleanup_waiting_(0), http_parser_buffer_(nullptr), - fs_stats_field_array_(isolate_, kFsStatsFieldsLength), + fs_stats_field_array_(isolate_, kFsStatsFieldsLength * 2), 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 e6a4e1dc05..23758c8d12 100644 --- a/src/env.h +++ b/src/env.h @@ -644,6 +644,10 @@ class Environment { inline AliasedBuffer* fs_stats_field_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(); @@ -822,9 +826,6 @@ class Environment { bool http_parser_buffer_in_use_ = false; std::unique_ptr http2_state_; - // 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 = 2 * 14; AliasedBuffer fs_stats_field_array_; std::vector> diff --git a/src/node_file.cc b/src/node_file.cc index 5e88f76c40..14d908cd6f 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -45,42 +45,6 @@ namespace node { -void FillStatsArray(AliasedBuffer* fields_ptr, - const uv_stat_t* s, int offset) { - 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] = -1; -#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] = -1; -#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 -} - namespace fs { using v8::Array; @@ -432,12 +396,9 @@ void FSReqWrap::Reject(Local reject) { MakeCallback(env()->oncomplete_string(), 1, &reject); } -void FSReqWrap::FillStatsArray(const uv_stat_t* stat) { - node::FillStatsArray(env()->fs_stats_field_array(), stat); -} - -void FSReqWrap::ResolveStat() { - Resolve(Undefined(env()->isolate())); +void FSReqWrap::ResolveStat(const uv_stat_t* stat) { + node::FillGlobalStatsArray(env(), stat); + Resolve(env()->fs_stats_field_array()->GetJSArray()); } void FSReqWrap::Resolve(Local value) { @@ -452,65 +413,12 @@ void FSReqWrap::SetReturnValue(const FunctionCallbackInfo& args) { args.GetReturnValue().SetUndefined(); } -void FSReqPromise::SetReturnValue(const FunctionCallbackInfo& args) { - Local context = env()->context(); - args.GetReturnValue().Set( - object()->Get(context, env()->promise_string()).ToLocalChecked() - .As()->GetPromise()); -} - void NewFSReqWrap(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); Environment* env = Environment::GetCurrent(args.GetIsolate()); new FSReqWrap(env, args.This()); } -FSReqPromise::FSReqPromise(Environment* env) - : FSReqBase(env, - env->fsreqpromise_constructor_template() - ->NewInstance(env->context()).ToLocalChecked(), - AsyncWrap::PROVIDER_FSREQPROMISE), - stats_field_array_(env->isolate(), 14) { - auto resolver = Promise::Resolver::New(env->context()).ToLocalChecked(); - object()->Set(env->context(), env->promise_string(), - resolver).FromJust(); -} - -FSReqPromise::~FSReqPromise() { - // Validate that the promise was explicitly resolved or rejected. - CHECK(finished_); -} - -void FSReqPromise::Reject(Local reject) { - finished_ = true; - HandleScope scope(env()->isolate()); - InternalCallbackScope callback_scope(this); - Local value = - object()->Get(env()->context(), - env()->promise_string()).ToLocalChecked(); - Local resolver = value.As(); - resolver->Reject(env()->context(), reject).FromJust(); -} - -void FSReqPromise::FillStatsArray(const uv_stat_t* stat) { - node::FillStatsArray(&stats_field_array_, stat); -} - -void FSReqPromise::ResolveStat() { - Resolve(stats_field_array_.GetJSArray()); -} - -void FSReqPromise::Resolve(Local value) { - finished_ = true; - HandleScope scope(env()->isolate()); - InternalCallbackScope callback_scope(this); - Local val = - object()->Get(env()->context(), - env()->promise_string()).ToLocalChecked(); - Local resolver = val.As(); - resolver->Resolve(env()->context(), value).FromJust(); -} - FSReqAfterScope::FSReqAfterScope(FSReqBase* wrap, uv_fs_t* req) : wrap_(wrap), req_(req), @@ -563,8 +471,7 @@ void AfterStat(uv_fs_t* req) { FSReqAfterScope after(req_wrap, req); if (after.Proceed()) { - req_wrap->FillStatsArray(&req->statbuf); - req_wrap->ResolveStat(); + req_wrap->ResolveStat(&req->statbuf); } } @@ -751,7 +658,7 @@ inline FSReqBase* GetReqWrap(Environment* env, Local value) { if (value->IsObject()) { return Unwrap(value.As()); } else if (value->StrictEquals(env->fs_use_promises_symbol())) { - return new FSReqPromise(env); + return new FSReqPromise(env); } return nullptr; } @@ -893,7 +800,7 @@ static void Stat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); const int argc = args.Length(); - CHECK_GE(argc, 1); + CHECK_GE(argc, 2); BufferValue path(env->isolate(), args[0]); CHECK_NE(*path, nullptr); @@ -907,8 +814,8 @@ static void Stat(const FunctionCallbackInfo& args) { FSReqWrapSync req_wrap_sync; int err = SyncCall(env, args[2], &req_wrap_sync, "stat", uv_fs_stat, *path); if (err == 0) { - FillStatsArray(env->fs_stats_field_array(), - static_cast(req_wrap_sync.req.ptr)); + node::FillGlobalStatsArray(env, + static_cast(req_wrap_sync.req.ptr)); } } } @@ -917,7 +824,7 @@ static void LStat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); const int argc = args.Length(); - CHECK_GE(argc, 1); + CHECK_GE(argc, 2); BufferValue path(env->isolate(), args[0]); CHECK_NE(*path, nullptr); @@ -932,8 +839,8 @@ static void LStat(const FunctionCallbackInfo& args) { int err = SyncCall(env, args[2], &req_wrap_sync, "lstat", uv_fs_lstat, *path); if (err == 0) { - FillStatsArray(env->fs_stats_field_array(), - static_cast(req_wrap_sync.req.ptr)); + node::FillGlobalStatsArray(env, + static_cast(req_wrap_sync.req.ptr)); } } } @@ -942,7 +849,7 @@ static void FStat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); const int argc = args.Length(); - CHECK_GE(argc, 1); + CHECK_GE(argc, 2); CHECK(args[0]->IsInt32()); int fd = args[0].As()->Value(); @@ -956,8 +863,8 @@ static void FStat(const FunctionCallbackInfo& args) { FSReqWrapSync req_wrap_sync; int err = SyncCall(env, args[2], &req_wrap_sync, "fstat", uv_fs_fstat, fd); if (err == 0) { - FillStatsArray(env->fs_stats_field_array(), - static_cast(req_wrap_sync.req.ptr)); + node::FillGlobalStatsArray(env, + static_cast(req_wrap_sync.req.ptr)); } } } @@ -1908,6 +1815,11 @@ void Initialize(Local target, env->SetMethod(target, "mkdtemp", Mkdtemp); + target->Set(env->context(), + FIXED_ONE_BYTE_STRING(env->isolate(), "kFsStatsFieldsLength"), + Integer::New(env->isolate(), env->kFsStatsFieldsLength)) + .FromJust(); + target->Set(context, FIXED_ONE_BYTE_STRING(env->isolate(), "statValues"), env->fs_stats_field_array()->GetJSArray()).FromJust(); diff --git a/src/node_file.h b/src/node_file.h index 1925e400f2..d6c8aa443c 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -61,10 +61,9 @@ class FSReqBase : public ReqWrap { return buffer_; } - virtual void FillStatsArray(const uv_stat_t* stat) = 0; virtual void Reject(Local reject) = 0; virtual void Resolve(Local value) = 0; - virtual void ResolveStat() = 0; + virtual void ResolveStat(const uv_stat_t* stat) = 0; virtual void SetReturnValue(const FunctionCallbackInfo& args) = 0; const char* syscall() const { return syscall_; } @@ -90,31 +89,72 @@ class FSReqWrap : public FSReqBase { FSReqWrap(Environment* env, Local req) : FSReqBase(env, req, AsyncWrap::PROVIDER_FSREQWRAP) { } - void FillStatsArray(const uv_stat_t* stat) override; void Reject(Local reject) override; void Resolve(Local value) override; - void ResolveStat() override; + void ResolveStat(const uv_stat_t* stat) override; void SetReturnValue(const FunctionCallbackInfo& args) override; private: DISALLOW_COPY_AND_ASSIGN(FSReqWrap); }; +template class FSReqPromise : public FSReqBase { public: - explicit FSReqPromise(Environment* env); + explicit FSReqPromise(Environment* env) + : FSReqBase(env, + env->fsreqpromise_constructor_template() + ->NewInstance(env->context()).ToLocalChecked(), + AsyncWrap::PROVIDER_FSREQPROMISE), + stats_field_array_(env->isolate(), env->kFsStatsFieldsLength) { + auto resolver = Promise::Resolver::New(env->context()).ToLocalChecked(); + object()->Set(env->context(), env->promise_string(), + resolver).FromJust(); + } - ~FSReqPromise() override; + ~FSReqPromise() override { + // Validate that the promise was explicitly resolved or rejected. + CHECK(finished_); + } - void FillStatsArray(const uv_stat_t* stat) override; - void Reject(Local reject) override; - void Resolve(Local value) override; - void ResolveStat() override; - void SetReturnValue(const FunctionCallbackInfo& args) override; + void Reject(Local reject) override { + finished_ = true; + HandleScope scope(env()->isolate()); + InternalCallbackScope callback_scope(this); + Local value = + object()->Get(env()->context(), + env()->promise_string()).ToLocalChecked(); + Local resolver = value.As(); + resolver->Reject(env()->context(), reject).FromJust(); + } + + void Resolve(Local value) override { + finished_ = true; + HandleScope scope(env()->isolate()); + InternalCallbackScope callback_scope(this); + Local val = + object()->Get(env()->context(), + env()->promise_string()).ToLocalChecked(); + Local resolver = val.As(); + resolver->Resolve(env()->context(), value).FromJust(); + } + + void ResolveStat(const uv_stat_t* stat) override { + node::FillStatsArray(&stats_field_array_, stat); + Resolve(stats_field_array_.GetJSArray()); + } + + void SetReturnValue(const FunctionCallbackInfo& args) override { + Local val = + object()->Get(env()->context(), + env()->promise_string()).ToLocalChecked(); + Local resolver = val.As(); + args.GetReturnValue().Set(resolver->GetPromise()); + } private: bool finished_ = false; - AliasedBuffer stats_field_array_; + AliasedBuffer stats_field_array_; DISALLOW_COPY_AND_ASSIGN(FSReqPromise); }; diff --git a/src/node_internals.h b/src/node_internals.h index 6439ccc7a2..8cc1aa1a8f 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -307,8 +307,48 @@ v8::Maybe ProcessEmitDeprecationWarning(Environment* env, const char* warning, const char* deprecation_code); -void FillStatsArray(AliasedBuffer* fields_ptr, - const uv_stat_t* s, int offset = 0); +template +void 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] = -1; +#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] = -1; +#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 +} + +inline void FillGlobalStatsArray(Environment* env, + const uv_stat_t* s, + int offset = 0) { + node::FillStatsArray(env->fs_stats_field_array(), s, offset); +} void SetupProcessObject(Environment* env, int argc, diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index 32b416c466..2ff3af633f 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -107,10 +107,14 @@ void StatWatcher::Callback(uv_fs_poll_t* handle, HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); - FillStatsArray(env->fs_stats_field_array(), curr); - FillStatsArray(env->fs_stats_field_array(), prev, 14); - Local arg = Integer::New(env->isolate(), status); - wrap->MakeCallback(env->onchange_string(), 1, &arg); + node::FillGlobalStatsArray(env, curr); + node::FillGlobalStatsArray(env, prev, env->kFsStatsFieldsLength); + + Local argv[2] { + Integer::New(env->isolate(), status), + env->fs_stats_field_array()->GetJSArray() + }; + wrap->MakeCallback(env->onchange_string(), arraysize(argv), argv); } -- cgit v1.2.3