From ce4c8c823ca03d065fb880c50789ba136722c8e7 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 7 Apr 2018 16:57:22 +0800 Subject: [PATCH] fs: return stats to JS in sync methods - Reduce reference to the global `statValues` by returning the changed stats array from the synchronous methods. Having a local returned value also makes the future integration of BigInt easier. - Also returns the filled array from node::FillGlobalStatsArray and node::FillStatsArray in the C++ side. PR-URL: https://github.com/nodejs/node/pull/20167 Reviewed-By: Ruben Bridgewater Reviewed-By: Anna Henningsen Reviewed-By: Khaidi Chu Reviewed-By: Gus Caplan --- lib/fs.js | 63 ++++++++++++++++++++-------------------- lib/internal/fs.js | 6 ---- src/node_file.cc | 30 ++++++++++++------- src/node_internals.h | 12 ++++---- src/node_stat_watcher.cc | 4 +-- 5 files changed, 60 insertions(+), 55 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 6c8417030b7ac7..d71e31565fbad6 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -57,7 +57,6 @@ const { preprocessSymlinkDestination, Stats, getStatsFromBinding, - getStatsFromGlobalBinding, stringToFlags, stringToSymlinkType, toUnixTimestamp, @@ -158,10 +157,10 @@ function isFd(path) { fs.Stats = Stats; -function isFileType(fileType) { +function isFileType(stats, fileType) { // Use stats array directly to avoid creating an fs.Stats instance just for // our internal use. - return (statValues[1/* mode */] & S_IFMT) === fileType; + return (stats[1/* mode */] & S_IFMT) === fileType; } // Don't allow mode to accidentally be overwritten. @@ -330,15 +329,15 @@ function readFileAfterOpen(err, fd) { binding.fstat(fd, req); } -function readFileAfterStat(err) { +function readFileAfterStat(err, stats) { var context = this.context; if (err) return context.close(err); var size; - if (isFileType(S_IFREG)) - size = context.size = statValues[8]; + if (isFileType(stats, S_IFREG)) + size = context.size = stats[8]; else size = context.size = 0; @@ -411,11 +410,12 @@ function readFileAfterClose(err) { function tryStatSync(fd, isUserFd) { const ctx = {}; - binding.fstat(fd, undefined, ctx); + const stats = binding.fstat(fd, undefined, ctx); if (ctx.errno !== undefined && !isUserFd) { fs.closeSync(fd); throw errors.uvException(ctx); } + return stats; } function tryCreateBuffer(size, fd, isUserFd) { @@ -450,10 +450,10 @@ fs.readFileSync = function(path, options) { var isUserFd = isFd(path); // file descriptor ownership var fd = isUserFd ? path : fs.openSync(path, options.flag || 'r', 0o666); - tryStatSync(fd, isUserFd); + const stats = tryStatSync(fd, isUserFd); var size; - if (isFileType(S_IFREG)) - size = statValues[8]; + if (isFileType(stats, S_IFREG)) + size = stats[8]; else size = 0; var pos = 0; @@ -890,27 +890,29 @@ fs.stat = function(path, callback) { fs.fstatSync = function(fd) { validateUint32(fd, 'fd'); const ctx = { fd }; - binding.fstat(fd, undefined, ctx); + const stats = binding.fstat(fd, undefined, ctx); handleErrorFromBinding(ctx); - return getStatsFromGlobalBinding(); + return getStatsFromBinding(stats); }; fs.lstatSync = function(path) { path = getPathFromURL(path); validatePath(path); const ctx = { path }; - binding.lstat(pathModule.toNamespacedPath(path), undefined, ctx); + const stats = binding.lstat(pathModule.toNamespacedPath(path), + undefined, ctx); handleErrorFromBinding(ctx); - return getStatsFromGlobalBinding(); + return getStatsFromBinding(stats); }; fs.statSync = function(path) { path = getPathFromURL(path); validatePath(path); const ctx = { path }; - binding.stat(pathModule.toNamespacedPath(path), undefined, ctx); + const stats = binding.stat(pathModule.toNamespacedPath(path), + undefined, ctx); handleErrorFromBinding(ctx); - return getStatsFromGlobalBinding(); + return getStatsFromBinding(stats); }; fs.readlink = function(path, options, callback) { @@ -1439,7 +1441,7 @@ function StatWatcher() { this._handle.onchange = function(newStatus, stats) { if (oldStatus === -1 && newStatus === -1 && - statValues[2/* new nlink */] === statValues[16/* old nlink */]) return; + stats[2/* new nlink */] === stats[16/* old nlink */]) return; oldStatus = newStatus; self.emit('change', getStatsFromBinding(stats), @@ -1666,7 +1668,8 @@ fs.realpathSync = function realpathSync(p, options) { // continue if not a symlink, break if a pipe/socket if (knownHard[base] || (cache && cache.get(base) === base)) { - if (isFileType(S_IFIFO) || isFileType(S_IFSOCK)) { + if (isFileType(statValues, S_IFIFO) || + isFileType(statValues, S_IFSOCK)) { break; } continue; @@ -1682,10 +1685,10 @@ fs.realpathSync = function realpathSync(p, options) { var baseLong = pathModule.toNamespacedPath(base); const ctx = { path: base }; - binding.lstat(baseLong, undefined, ctx); + const stats = binding.lstat(baseLong, undefined, ctx); handleErrorFromBinding(ctx); - if (!isFileType(S_IFLNK)) { + if (!isFileType(stats, S_IFLNK)) { knownHard[base] = true; if (cache) cache.set(base, base); continue; @@ -1696,8 +1699,8 @@ fs.realpathSync = function realpathSync(p, options) { var linkTarget = null; var id; if (!isWindows) { - var dev = statValues[0].toString(32); - var ino = statValues[7].toString(32); + var dev = stats[0].toString(32); + var ino = stats[7].toString(32); id = `${dev}:${ino}`; if (seenLinks[id]) { linkTarget = seenLinks[id]; @@ -1778,7 +1781,7 @@ fs.realpath = function realpath(p, options, callback) { // On windows, check that the root exists. On unix there is no need. if (isWindows && !knownHard[base]) { - fs.lstat(base, function(err) { + fs.lstat(base, function(err, stats) { if (err) return callback(err); knownHard[base] = true; LOOP(); @@ -1811,7 +1814,8 @@ fs.realpath = function realpath(p, options, callback) { // continue if not a symlink, break if a pipe/socket if (knownHard[base]) { - if (isFileType(S_IFIFO) || isFileType(S_IFSOCK)) { + if (isFileType(statValues, S_IFIFO) || + isFileType(statValues, S_IFSOCK)) { return callback(null, encodeRealpathResult(p, options)); } return process.nextTick(LOOP); @@ -1820,14 +1824,11 @@ fs.realpath = function realpath(p, options, callback) { return fs.lstat(base, gotStat); } - function gotStat(err) { + function gotStat(err, stats) { if (err) return callback(err); - // Use stats array directly to avoid creating an fs.Stats instance just for - // our internal use. - // if not a symlink, skip to the next path part - if (!isFileType(S_IFLNK)) { + if (!stats.isSymbolicLink()) { knownHard[base] = true; return process.nextTick(LOOP); } @@ -1837,8 +1838,8 @@ fs.realpath = function realpath(p, options, callback) { // dev/ino always return 0 on windows, so skip the check. let id; if (!isWindows) { - var dev = statValues[0].toString(32); - var ino = statValues[7].toString(32); + var dev = stats.dev.toString(32); + var ino = stats.ino.toString(32); id = `${dev}:${ino}`; if (seenLinks[id]) { return gotTarget(null, seenLinks[id], base); diff --git a/lib/internal/fs.js b/lib/internal/fs.js index b431cb910c2506..0cfb89a9d3d084 100644 --- a/lib/internal/fs.js +++ b/lib/internal/fs.js @@ -35,7 +35,6 @@ const { UV_FS_SYMLINK_DIR, UV_FS_SYMLINK_JUNCTION } = process.binding('constants').fs; -const { statValues } = process.binding('fs'); const isWindows = process.platform === 'win32'; @@ -217,10 +216,6 @@ function getStatsFromBinding(stats, offset = 0) { stats[12 + offset], stats[13 + offset]); } -function getStatsFromGlobalBinding(offset = 0) { - return getStatsFromBinding(statValues, offset); -} - function stringToFlags(flags) { if (typeof flags === 'number') { return flags; @@ -442,7 +437,6 @@ module.exports = { preprocessSymlinkDestination, realpathCacheKey: Symbol('realpathCacheKey'), getStatsFromBinding, - getStatsFromGlobalBinding, stringToFlags, stringToSymlinkType, Stats, diff --git a/src/node_file.cc b/src/node_file.cc index dcbdf86579df61..89c53afc5b197e 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -410,8 +410,7 @@ void FSReqWrap::Reject(Local reject) { } void FSReqWrap::ResolveStat(const uv_stat_t* stat) { - node::FillGlobalStatsArray(env(), stat); - Resolve(env()->fs_stats_field_array()->GetJSArray()); + Resolve(node::FillGlobalStatsArray(env(), stat)); } void FSReqWrap::Resolve(Local value) { @@ -832,10 +831,13 @@ static void Stat(const FunctionCallbackInfo& args) { FS_SYNC_TRACE_BEGIN(stat); int err = SyncCall(env, args[2], &req_wrap_sync, "stat", uv_fs_stat, *path); FS_SYNC_TRACE_END(stat); - if (err == 0) { - node::FillGlobalStatsArray(env, - static_cast(req_wrap_sync.req.ptr)); + if (err != 0) { + return; // error info is in ctx } + + Local arr = node::FillGlobalStatsArray(env, + static_cast(req_wrap_sync.req.ptr)); + args.GetReturnValue().Set(arr); } } @@ -859,10 +861,13 @@ static void LStat(const FunctionCallbackInfo& args) { int err = SyncCall(env, args[2], &req_wrap_sync, "lstat", uv_fs_lstat, *path); FS_SYNC_TRACE_END(lstat); - if (err == 0) { - node::FillGlobalStatsArray(env, - static_cast(req_wrap_sync.req.ptr)); + if (err != 0) { + return; // error info is in ctx } + + Local arr = node::FillGlobalStatsArray(env, + static_cast(req_wrap_sync.req.ptr)); + args.GetReturnValue().Set(arr); } } @@ -885,10 +890,13 @@ static void FStat(const FunctionCallbackInfo& args) { FS_SYNC_TRACE_BEGIN(fstat); int err = SyncCall(env, args[2], &req_wrap_sync, "fstat", uv_fs_fstat, fd); FS_SYNC_TRACE_END(fstat); - if (err == 0) { - node::FillGlobalStatsArray(env, - static_cast(req_wrap_sync.req.ptr)); + if (err != 0) { + return; // error info is in ctx } + + Local arr = node::FillGlobalStatsArray(env, + static_cast(req_wrap_sync.req.ptr)); + args.GetReturnValue().Set(arr); } } diff --git a/src/node_internals.h b/src/node_internals.h index 492a36296fb498..50047e330724fe 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -313,7 +313,7 @@ v8::Maybe ProcessEmitDeprecationWarning(Environment* env, const char* deprecation_code); template -void FillStatsArray(AliasedBuffer* fields_ptr, +v8::Local FillStatsArray(AliasedBuffer* fields_ptr, const uv_stat_t* s, int offset = 0) { AliasedBuffer& fields = *fields_ptr; fields[offset + 0] = s->st_dev; @@ -347,12 +347,14 @@ void FillStatsArray(AliasedBuffer* fields_ptr, X(12, ctim) X(13, birthtim) #undef X + + return fields_ptr->GetJSArray(); } -inline void FillGlobalStatsArray(Environment* env, - const uv_stat_t* s, - int offset = 0) { - node::FillStatsArray(env->fs_stats_field_array(), s, offset); +inline v8::Local FillGlobalStatsArray(Environment* env, + const uv_stat_t* s, + int offset = 0) { + return node::FillStatsArray(env->fs_stats_field_array(), s, offset); } void SetupProcessObject(Environment* env, diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index 878d82b8dfe951..41683a0dc1d36c 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -108,12 +108,12 @@ void StatWatcher::Callback(uv_fs_poll_t* handle, HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); - node::FillGlobalStatsArray(env, curr); + Local arr = node::FillGlobalStatsArray(env, curr); node::FillGlobalStatsArray(env, prev, env->kFsStatsFieldsLength); Local argv[2] { Integer::New(env->isolate(), status), - env->fs_stats_field_array()->GetJSArray() + arr }; wrap->MakeCallback(env->onchange_string(), arraysize(argv), argv); }