8000 Lazy stat dates by sciolist · Pull Request #12818 · nodejs/node · GitHub
[go: up one dir, main page]
Skip to content

Lazy stat dates #12818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sciolist wants to merge 5 commits into from
Closed

Lazy stat dates #12818

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ccbc92f
lib: lazy instantiation of fs dates
sciolist May 3, 2017
71b01c6
lint
sciolist May 3, 2017
dcbe6b2
remove blksize from test
sciolist May 22, 2017
1eb7196
lint
8000 sciolist May 22, 2017
61ff138
blocks as well, why not
sciolist May 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
lib: lazy instantiation of fs dates
  • Loading branch information
@sciolist
sciolist committed Jun 2, 2017
commit ccbc92f1be52b65b53bdb3467f9186e8ea44ff75
71 changes: 67 additions & 4 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,76 @@ function Stats(
this.ino = ino;
this.size = size;
this.blocks = blocks;
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._atim_msec = atim_msec;
this._mtim_msec = mtim_msec;
this._ctim_msec = ctim_msec;
this._birthtim_msec = birthtim_msec;
Copy link
Member
@addaleax addaleax May 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might as well make these public? ¯\_(ツ)_/¯ If not, I agree that using Symbols may be better.

Copy link
Member
@jasnell jasnell May 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the Symbol approach for these.

}
fs.Stats = Stats;

Object.defineProperties(Stats.prototype, {
atime: {
configurable: true,
enumerable: true,
get() {
return this._atime !== undefined ?
this._atime :
(this._atime = new Date(this._atim_msec + 0.5));
},
Copy link
Member
@jasnell jasnell May 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can eliminate some code duplication here by having a factory function for the getters... e.g.

function makeGetter(name, src) {
  return function() {
    return this[name] !== undefined ?
      this[name] : (this[name] = new Date(this[src + 0.5));
  };
}

// ...

Object.defineProperties(Stats.prototype, {
  atime: {
    configurable: true,
    enumerable: true,
    get: makeGetter('_atime', '_atim_msec'),
    // etc
  }
});

Copy link
Member
@jasnell jasnell May 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea here would be to use another defineProperty within the getter to replace the value once the date is created...

var m = {};
Object.defineProperty(m, 'a', {
  configurable: true,
  enumerable: true,
  get() {
    const val = 1;
    delete this.a;
    Object.defineProperty(this, 'a', { configurable: true, enumerable: true, value: val } );
    return val;
  }
});

The getter is called once, during which time it is deleted, and the static value is set... and will be returned every time after. Should have a slightly better performance profile.

Copy link
Contributor
@refack refack May 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu suggested that in the previous PR, it wasn't performant enough for the "single access" case - #12607 (comment)

Copy link
Member
@jasnell jasnell May 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's fine then. I'd still suggest the factory function to avoid the code duplication

Copy link
Contributor Author
@sciolist sciolist May 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, yeah I have tried these variations.

Unfortunately a single defineProperty is more expensive than 4 date creations, so you'd lose the benefits of the laziness if you need to access even a single date.

this[somevariable] is slow enough compared to this.mtime that the difference becomes pretty significant in a function like stat, that isn't very heavy to begin with. That drops accessing one time-field twice (my test is if (stat.ctime.getTime() !== stat.ctime.getTime()) throw err;) to a 65% improvement over the non-lazy code, down from a 105% improvement if done the ugly way. But I can definitely change to that if it's preferable.. I didn't write it this way because I think it's beautiful. :)

set(value) { return this._atime = value; }
},
mtime: {
configurable: true,
enumerable: true,
get() {
return this._mtime !== undefined ?
this._mtime :
(this._mtime = new Date(this._mtim_msec + 0.5));
},
set(value) { return this._mtime = value; }
},
ctime: {
configurable: true,
enumerable: true,
get() {
return this._ctime !== undefined ?
this._ctime :
(this._ctime = new Date(this._ctim_msec + 0.5));
},
set(value) { return this._ctime = value; }
},
birthtime: {
configurable: true,
enumerable: true,
get() {
return this._birthtime !== undefined ?
this._birthtime :
(this._birthtime = new Date(this._birthtim_msec + 0.5));
},
set(value) { return this._birthtime = value; }
},
});

Stats.prototype.toJSON = function toJSON() {
return {
dev: this.dev,
mode: this.mode,
nlink: this.nlink,
uid: this.uid,
gid: this.gid,
rdev: this.rdev,
blksize: this.blksize,
ino: this.ino,
size: this.size,
blocks: this.blocks,
atime: this.atime,
ctime: this.ctime,
mtime: this.mtime,
birthtime: this.birthtime
};
}


Stats.prototype._checkModeProperty = function(property) {
return ((this.mode & S_IFMT) === property);
};
Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-fs-stat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,5 +98,21 @@ fs.stat(__filename, common.mustCall(function(err, s) {
console.log(`isSymbolicLink: ${JSON.stringify(s.isSymbolicLink())}`);
assert.strictEqual(false, s.isSymbolicLink());

assert.ok(s.atime instanceof Date);
assert.ok(s.mtime instanceof Date);
assert.ok(s.ctime instanceof Date);
assert.ok(s.birthtime instanceof Date);
}));

fs.stat(__filename, common.mustCall(function(err, s) {
const json = JSON.parse(JSON.stringify(s));
const keys = [
Copy link
Contributor
@refack refack May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought: either put a comment saying why we exclude blksize
Or put if(!common.isWindows) keys.push('blksize')
Your call but pick one.

Copy link
Contributor
@refack refack May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just found out need to do the same for 'blocks' (undefined on Windows)

Copy link
Contributor Author
@sciolist sciolist May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added an if(!common.isWindows) bit, seems reasonable.

'dev', 'mode', 'nlink', 'uid',
'gid', 'rdev', 'blksize', 'ino',
'size', 'blocks', 'atime', 'mtime',
'ctime', 'birthtime'
];
keys.forEach(function(k) {
assert.ok(json[k] !== undefined && json[k] !== null);
});
}));
0