Skip to content

Commit

Permalink
fs: refactor stats array to be more generic
Browse files Browse the repository at this point in the history
- 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: #19714
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
joyeecheung committed Apr 4, 2018
1 parent 30fe55e commit f7049a2
Show file tree
Hide file tree
Showing 9 changed files with 168 additions and 163 deletions.
29 changes: 11 additions & 18 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -56,7 +56,8 @@ const {
nullCheck,
preprocessSymlinkDestination,
Stats,
statsFromValues,
getStatsFromBinding,
getStatsFromGlobalBinding,
stringToFlags,
stringToSymlinkType,
toUnixTimestamp,
Expand Down Expand Up @@ -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));
};
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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);

Expand All @@ -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() {
Expand Down
15 changes: 9 additions & 6 deletions lib/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ const { isUint8Array } = require('internal/util/types');
const {
copyObject,
getOptions,
getStatsFromBinding,
isUint32,
modeNum,
nullCheck,
preprocessSymlinkDestination,
statsFromValues,
stringToFlags,
stringToSymlinkType,
toUnixTimestamp,
Expand Down Expand Up @@ -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) {
Expand Down
32 changes: 22 additions & 10 deletions lib/internal/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -424,7 +435,8 @@ module.exports = {
nullCheck,
preprocessSymlinkDestination,
realpathCacheKey: Symbol('realpathCacheKey'),
statsFromValues,
getStatsFromBinding,
getStatsFromGlobalBinding,
stringToFlags,
stringToSymlinkType,
Stats,
Expand Down
2 changes: 1 addition & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
7 changes: 4 additions & 3 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,10 @@ class Environment {

inline AliasedBuffer<double, v8::Float64Array>* 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<std::unique_ptr<fs::FileHandleReadWrap>>&
file_handle_read_wrap_freelist();

Expand Down Expand Up @@ -822,9 +826,6 @@ class Environment {
bool http_parser_buffer_in_use_ = false;
std::unique_ptr<http2::http2_state> 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<double, v8::Float64Array> fs_stats_field_array_;

std::vector<std::unique_ptr<fs::FileHandleReadWrap>>
Expand Down
Loading

0 comments on commit f7049a2

Please sign in to comment.