Skip to content
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.

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

src: round nsec instead of truncating stat times #12607

Closed
wants to merge 10 commits into from

Conversation

@sciolist
Copy link
Contributor

commented Apr 23, 2017

Truncation of submillisecond timestamp accuracy on windows creates situations where attempting to change access or modification times becomes off by one ms. Example:

var fs = require('fs');
var targetMTime = new Date('2017-04-08T17:59:38.008Z');

fs.writeFileSync("./file1", "");
fs.utimesSync("./file1", targetMTime, targetMTime);

var actualMTime = fs.statSync("./file1").mtime;

console.log(actualMTime); // 2017-04-08T17:59:38.007Z
console.log(actualMTime.getTime() === targetMTime.getTime()); // false

This PR fixes the issue by rounding nsec rather than truncating in node_file.cc. A concern with that is that stat is a rather hot bit of code.. adding in 4 rounding operations could make a noticeable difference.

Another option could be to increment the return from toUnixTimestamp in fs.js enough to offset the precision loss, that would at least fix the issue if only node processes are updating the files.. but not particularly robust.

A third solution is to ignore the problem. It likely isn't something that affects that many users.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2017

sciolist added a commit to sciolist/yarn that referenced this pull request Apr 23, 2017
accept 1ms difference in filedates as equal on windows
a nodejs issue causes certain dates to be off by 1ms after calling utimes

See: nodejs/node#12607
@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2017

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2017

From a CI log:

../src/node_file.cc: In function ‘void node::FillStatsArray(double*, const uv_stat_t*)’:
../src/node_file.cc:469:121: error: ‘round’ was not declared in this scope
   X(10, atim)
@sciolist

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2017

Mmm needs to include math for linux, had only built on Windows.
I'll run all these benchmarks, which are taking a while, and correct it.

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2017

@sciolist

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2017

Linux benchmark results:

jdp@vrt:~/node$ cat c.csv | Rscript benchmark/compare.R
                                                 improvement confidence     p.value
 fs/bench-statSync.js kind="fstatSync" n=1000000     -2.21 %         ** 0.004221974
 fs/bench-statSync.js kind="lstatSync" n=1000000     -1.09 %            0.146230478
 fs/bench-statSync.js kind="statSync" n=1000000       0.67 %            0.415415495

Windows benchmark results:

C:\dev\node>Rscript benchmark\compare.R <c.csv
                                                  improvement confidence      p.value
 fs\\bench-statSync.js kind="fstatSync" n=1000000     -1.50 %         ** 1.763399e-03
 fs\\bench-statSync.js kind="lstatSync" n=1000000     -2.36 %        *** 3.457501e-06
 fs\\bench-statSync.js kind="statSync" n=1000000      -0.77 %            2.111487e-01

Pretty substantial hit. Especially for linux considering it can't even have sub-ms filetimes.
Could be ifdef'ed to only round on Windows (if that's the only supported platform with higher resolution filetimes,) but that's still not great.

@refack refack added the libuv label Apr 23, 2017

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

@refack How does uv_uptime relate to this?

@refack

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

@refack How does uv_uptime relate to this?

🤦I was half asleep.

@refack refack removed the libuv label Apr 23, 2017

@refack

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

To rectify my half-baked-sleepy-thought from before, let me suggest a different solution:
In fs.js replace the value properties with lazy getters. you get the Round and a performance gain.

 lib/fs.js | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index c1d8db9f8..8759dcf83 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -157,10 +157,14 @@ function Stats(
   this.ino = ino;
   this.size = size;
   this.blocks = blocks;
-  this.atime = new Date(atim_msec);
-  this.mtime = new Date(mtim_msec);
-  this.ctime = new Date(ctim_msec);
-  this.birthtime = new Date(birthtim_msec);
+  Object.defineProperties(fs, {
+    atime: {enumerable: true, get() {return new Date(Math.Round(atim_msec))}},
+    mtime: {enumerable: true, get() {return new Date(Math.Round(mtim_msec))}},
+    ctime: {enumerable: true, get() {return new Date(Math.Round(ctim_msec))}},
+    birthtime: {enumerable: true, get() {
+        return  new Date(Math.Round(birthtim_msec))
+    }}
+  }
 }
 fs.Stats = Stats;
@refack

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

Also there's the Truncate(x + 0.5) trick that might be more performant

FYI on windows the values are only accurate to 0.1 millisecond

Time values CreationTime, LastAccessTime, LastWriteTime, and ChangeTime are expressed in absolute system time format. Absolute system time is the number of 100-nanosecond intervals since the start of the year 1601 in the Gregorian calendar.
from https://msdn.microsoft.com/en-us/library/windows/hardware/ff545762(v=vs.85).aspx

src/node_file.cc Outdated
@@ -464,7 +465,7 @@ void FillStatsArray(double* fields, const uv_stat_t* s) {
// Dates.
#define X(idx, name) \
fields[idx] = (static_cast<double>(s->st_##name.tv_sec) * 1000) + \
(static_cast<double>(s->st_##name.tv_nsec / 1000000)); \
round(static_cast<double>(s->st_##name.tv_nsec) / 1000000); \

This comment has been minimized.

Copy link
@refack

refack Apr 23, 2017

Member

I'm not a C++ maven but maybe
(static_cast<double>((s->st_##name.tv_nsec + 500000) / 1000000))
Would be faster

Also If you're already here could you #define the magic numbers please?

@sciolist

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2017

@refack yeah truncating and adding 0.5ms is another option. felt a bit ugly to suggest but it would fix the issue with likely lower impact on performance. rounding seems like the more correct approach, but considering the slowdown it is worth considering trunc+add.

i'm not too sure that using lazy properties on the stat object would be good.. i did consider it, but it's a bit unexpected that reading time values after stat finishes involves a function call. might not really matter, though.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2017

AFAIK the only issue with adding 0.5 and dividing would be if the value was negative, but since that should/will never be the case here, it may be safe to do. It would be interesting to see the performance difference anyway compared to just round().

@refack

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

In hot spots we generally prefer performance over "the most correct solution"
So I'm +1 for the Truncate(x + 0.5) solution
But I have a feeling that the eventual new Date() is much more heavy, and at least on windows atime and birthtime are useless, so why pay for them.
So I'm +5 for the lazy properties

@refack

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

but since that should/will never be the case here

name.tv_nsec is an unsigned long 👍

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2017

But I have a feeling that the eventual new Date() is much more heavy, and at least on windows atime and birthtime are useless, so why pay for them.

There is/was previous discussion about these kinds of changes to fs.Stats in another thread.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

There is/was previous discussion about these kinds of changes to fs.Stats in another thread.

@mscdex couldn't find anything recent about the times... Do you remember where?

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2017

@refack

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

@mscdex thanks!

@sciolist

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2017

I'll try to run some more benchmarks tomorrow. Been up for a few days now, getting tired..

If we're looking into the lazy prop approach i'd expect to have to define them once on the prototype rather than in the constructor, as defineProperty is plenty more expensive than instantiating a Date. To do that we'll need a way to get the msec times outside the constructor, and also the return values should likely be cached in get(). I can give it a look when i have a little time to see what effect it would have.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

If we're looking into the lazy prop approach i'd expect to have to define them once on the prototype rather than in the constructor, as defineProperty is plenty more expensive than instantiating a Date. To do that we'll need a way to get the msec times outside the constructor, and also the return values should likely be cached in get(). I can give it a look when i have a little time to see what effect it would have.

Yes on all 👍

@refack

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

The define in the ctor allowed me to hide them in a closure, but you could store the raw numbers in _XXXX members and answer #8276

Daniel15 added a commit to yarnpkg/yarn that referenced this pull request Apr 24, 2017
accept 1ms difference in filedates as equal on windows (#3235)
a nodejs issue causes certain dates to be off by 1ms after calling utimes

See: nodejs/node#12607
refack added a commit to refack/node that referenced this pull request May 26, 2017
fs: expose Stats times as Numbers
also set Date cache field in constructor

Ref: nodejs#12607
Ref: nodejs#12818
@refack refack referenced this pull request May 27, 2017
@refack refack referenced this pull request May 27, 2017
3 of 4 tasks complete
refack added a commit to refack/node that referenced this pull request May 28, 2017
fs: expose Stats times as Numbers
This also reverts commit 9836cf5.

Fixes: npm/npm#16734
Ref: nodejs#12607
Ref: nodejs#12818
@refack refack referenced this pull request May 28, 2017
2 of 2 tasks complete
jasnell added a commit that referenced this pull request May 28, 2017
src,fs: calculate fs times without truncation
also added some missing bits that didn't make it into #12818

PR-URL: #12607
Refs: #12818
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@refack refack referenced this pull request May 29, 2017
3 of 3 tasks complete
refack added a commit to refack/node that referenced this pull request Jun 4, 2017
fs: expose Stats times as Numbers
* convert ’ to ' to turn md file to ASCII

Fixes: nodejs#8276
Refs: nodejs#12607
Refs: nodejs#12818
Refs: nodejs#13256
refack added a commit to refack/node that referenced this pull request Jun 7, 2017
fs: expose Stats times as Numbers
PR-URL: nodejs#13173
Fixes: nodejs#8276
Refs: nodejs#12607
Refs: nodejs#12818
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell added a commit that referenced this pull request Jun 7, 2017
fs: expose Stats times as Numbers
PR-URL: #13173
Fixes: #8276
Refs: #12607
Refs: #12818
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

Should this land on v6.x?

@refack

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

Should this land on v6.x?

Yes

  • current change-set is a Windows only bug fix. It should land iff #13281 lands.
  • #13173 depends on those two.
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

This is not landing cleanly, would you be willing to do a backport with the commits you mention?

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.