Revert "lib: lazy instantiation of fs.Stats dates"

[addaleax]

This reverts commit 9836cf5.

Ref: npm/npm#16734

@sciolist @refack @nodejs/ctc

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

fs

[mscdex]

Has anyone checked if this would have been caught with make test-npm?

[refack]

Has anyone checked if this would have been caught with make test-npm?

@mscdex I did, it wouldn't have.

IMHO we should first and foremost revert this from v8.x, v8.x-staging, but I'm not strongly opposed to reverting from master and recooking it.

For reference we also have #13173 that has been cooking for a while and is verified to fix npm/npm#16734.

[addaleax]

For reference we also have #13173 that has been cooking for a while and is verified to fix npm/npm#16734.

… and it’s quite likely that that regresses performance.

I know reverting sucks, but at this point I see no solution that should make it into v8.x, and we can’t tell how much of an impact this has by v9.x; so we might as well revert and re-visit.

[refack]

… and it’s quite likely that that regresses performance.

I know reverting sucks, but at this point I see no solution that should make it into v8.x, and we can’t tell how much of an impact this has by v9.x; so we might as well revert and re-visit.

Yeah it's a bit of a shame to revert, but understandable. I'm just giving another option.
(As for performance #13173 has a slight improvement over the baseline [master - 9836cf5])

There is also a third option to revert the semver-major bits, and still land #13173, which is semver-minor and solves #8276, and #13255

[refack]

So #13257 is the third option.
Reverts the semver-major bits of 9836cf5, but still adds new fields and solves #8276, and #13255.

It could be used to to a "mild" revert of 9836cf5, or it could be discussed after this PR land with the full revert.

[refack]

LGTM

[sciolist]

Sounds like a good call to me.

[addaleax]

@sciolist I’m glad to hear you agree. If you don’t mind, I’ll re-open #12818 after this lands so we can revisit for a later Node version. :)

[sciolist]

@addaleax I've never had problems with the performance of stat myself, I was working on another issue in Stat and was asked to look into this. :)

You can reopen the original PR if you'd like, but I can't really think of an approach right now that would solve the issue with Object.keys.

[refack]

"Dry-run" CI: https://ci.nodejs.org/job/node-test-commit/10207/

[refack]

Need to revert a109032 as well

[refack]

@addaleax I've never had problems with the performance of stat myself, I was working on another issue in Stat and was asked to look into this. :)

@sciolist Your comment reminded me that in a109032 we added a test that will fail after this revert.

[refack]

So some context.

1. Original PR was src: round nsec instead of truncating stat times #12607 to solve a bug with truncation of the timestamps on Windows
2. During discussion we spun off Lazy stat dates #12818 that turned the Date to non-own properties
3. Lazy stat dates #12818 landed before src: round nsec instead of truncating stat times #12607 so their dependency got inverted, src: round nsec instead of truncating stat times #12607 landed with a test that depends on 9836cf5.

[addaleax]

@sciolist Your comment reminded me that in a109032 we added a test that will fail after this revert.

No it doesn’t, I’ve accounted for that in the revert; as you see, the CI is looking good.

didn’t look at the changes / CI is good

[refack]

@sciolist Your comment reminded me that in a109032 we added a test that will fail after this revert.

No it doesn’t, I’ve accounted for that in the revert; as you see, the CI is looking god.

Ohh 👍 good job.

[jasnell]

oh good, thanks for the clarification @addaleax ... I was pretty certain that it had been accounted for but the confirmation is definitely appreciated.

[sciolist]

@refack Yeah I did actually check the changes to make sure it wouldn't be affected. Probably should have mentioned. :) (Or, probably shouldn't have made the +0.5 change in the lazy date PR to begin with.. just didn't notice I hadn't removed it until it was already merged.)

[refack]

@refack Yeah I did actually check the changes to make sure it wouldn't be affected. Probably should have mentioned. :) (Or, probably shouldn't have made the +0.5 change in the lazy date PR to begin with.. just didn't notice I hadn't removed it until it was already merged.)

Bottom line is: The "process" worked. The npm bug will not be in the release, and now we have time to recook the lazy-dates/exposed-millis change (maybe we can manipulate Stats with the V8 C++ API)

[jasnell]

I've dropped the commit from v8.x, pulling this off the 8 milestone.

[addaleax]

Landed in ae6c704

[refack]

Ohh I wanted to recommend adding the regression test from 13173.
NM. IMHO #13173 is a more mature version of #12818, hopefully we can land it with the test.