fs: make stats date fields lazy

[anonrig]

Fixes nodejs/performance#137

Thanks @fabiospampinato!

[fabiosantoscode]

Thank @fabiospampinato !

[fabiospampinato]

That was quick! Thank you!

[rluvaton]

LGTM

[MoLow]

LGTM, I wonder if the getter code can be written a little more DRYly, but that can be done in a follow-up

[robbiespeed]

Isn't this a breaking change?

• Object.entries(stats) (also keys,values) will no longer contain date fields. Those methods only operate on enumerable own fields, not enumerable fields in the prototype chain.
• Time fields are no longer settable (that seems less likely to be an issue though)

To be clear I think lazy dates are a good idea, just not sure what the projects stance is on small surface area breaking changes like this.

A few thoughts and suggestions:

In the current state there's a bit of a odd inconsistency with enumeration. Before access the date fields will show up in a for in loop, but they won't after. This is because enumerable: true is only set on the prototype.

I'd suggest not making the properties enumerable on instance and prototype for consistency.

Rather than set the value using ObjectDefineProperty, use private fields to store the cached date objects. It performs just as well if not slightly better in benchmarks thanks to V8s excellent optimization of private fields.

ObjectDefineProperty in it's current use could also have downstream performance issues since when benchmarking I noticed it can deopt functions presumable due to some kind of internal shape change to the objects (it didn't affect this benchmark since the function deopted statsDefinePropsRunA is pretty light anyway).

Using private fields with getters like in the benchmark code also has the benefit that own properties remain consistent on the object, even if different from previous node versions (which seems unavoidable anyway).

[mcollina]

I always forget about Object.entries()... this is unfortunately a semver major change.

This will definitely trigger a shape change and slower code paths if the dates are accessed multiple times. I don't think it would be a problem in this case.

[joyeecheung]

We should probably run the benchmarks to verify if it’s actually faster.

Also: why can’t they be created as own properties? Would that be much slower? If we just define then as own properties it doesn’t have to be semver major.

[joyeecheung]

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1473

[anonrig]

@joyeecheung @mcollina Updated the implementation so that we don't have a server-major change here. Appreciate if you could review again.

[H4ad]

I think it will be faster to have get atime instead of ObjectDefineProperties because it will probably be slower than just dateFromMs.

[anonrig]

That breaks the tests and makes this PR a semver-major

[H4ad]

Hm... in this case, maybe you can try cache this object outside the constructor using Symbol instead of private properties.

And without set, this is already a semver-major.

[robbiespeed]

The property descriptors could be stored on the class as static privates then be used with define properties like you have. I think this would improve perf a bit because each instance will no longer need to create it's own getter functions and objects for define properties, and V8 should be able to optimize the getters across all function instances. I can test that theory tomorrow.

[fabiospampinato]

Shouldn't these getters/setters be defined on the prototype of the class rather than for each instance? I wouldn't be surprised if defining them for each instance is more expensive than creating the 4 Date objects in the first place.

---

Oh I guess that breaks enumerability maybe? Then there may be no performant fully backwards compatible way to implement this and I guess we should just pull the tooth out and go for the best incompatible implementation and just wait for it to land eventually.

[robbiespeed]

Updated the benchmark to include more cases, and the ones that keep backward compat unfortunately perform worse than eagerly creating the dates. #50908 (comment)

[joyeecheung]

I am confused...why does it need to be a class with private fields now? I think it can just stay as a function (otherwise it breaks compatibility even further because fs.Stats.call(anotherObject) used to be something people could do). If what you need is to not recompute the Date object after the first access, you can just do another ObjectDefineProperty call in the getter to replace it with a data value, which is what we typically do for lazily-computed properties, then there is no need for any private fields because *Ms fields are public. Presumably that's also faster for people who do access these Date objects after the first access (because then they would be getting the replaced data value directly, instead of triggering a getter to get the cached private field indirectly)

[robbiespeed]

I am confused...why does it need to be a class with private fields now

@joyeecheung I was the one who recommended private fields, since using ObjectDefineProperty causes deopts to down stream functions, as well as adds inconsistencies to enumerability see my above comment for more.

On another note I updated the benchmark, and it turns out that any implementation that tries to preserve enumerability on the object using defineProperties on the instance during creating, results in worse performance than just creating the dates. benchmark code

So it seems for more performance a breaking change is required somewhere. How about just removing all date object fields as the breaking change, I know that might seem drastic, but once Temporal is standardized having these date fields is going to feel a bit dated (sorry for pun). Users can always manually convert from Ms to whatever they choose without much effort.

[fabiospampinato]

How about just removing all date object fields as the breaking change, I know that might seem drastic, but once Temporal is standardized having these date fields is going to feel a bit dated (sorry for pun). Users can always manually convert from Ms to whatever they choose without much effort.

This is IMO the best option if one was thinking about a better Stats implementation in isolation.

The problems would be that any code accessing those properties will have to be updated, and new Date(stats.ctimeMs) throws if we are asking for bigints, one has to write new Date(Number(stats.ctimeMs)), which is a bit annoying.

A positive aspect of the breaking change might be that we can check how the community reacts to it, maybe people want more of these changes. It won't be particularly representative, but maybe a pool can be made about this and it might provide useful information?

[anonrig]

I haven't had any time today but tomorrow morning I'll address @joyeecheung's comments and update the PR.

[joyeecheung]

I was the one who recommended private fields, since using ObjectDefineProperty causes deopts to down stream functions, as well as adds inconsistencies to enumerability see my above comment for more.

I am aware of the optimizations in private fields (I worked on the optimizations in V8 myself), but I don't think the semantics of private fields (or just class fields) fit into the use case here. Class fields are data properties and have to be eagerly created, whereas here we are trying to lazily create Date objects, which can only be done as accessor properties - either on the Stats object itself, or on the prototype. The PR currently can only make use of private fields as an internal cache computed inside the accessor, but the accessor have to be there nonetheless to make it lazy. If we are already implementing the laziness with an accessor, in the accessor we can just replace the accessor itself with the computed data property upon first access, so in subsequent access it would be a direct access on the replaced data property, instead of going through another accessor -> private field dance (this is a common pattern we do for lazily initialized globals).

How about just removing all date object fields as the breaking change, I know that might seem drastic, but once Temporal is standardized having these date fields is going to feel a bit dated (sorry for pun)

The date object fields are part of a stable, documented API, to remove them we need to go through deprecation cycles, which means we'd end up using accessors to emit warnings before we actually remove them (and this typically requires 2 semver-major to complete), then we are back to the performance overhead problem caused by accessors. Also the slowdown caused by Date object is not a constant, just the current situation in V8. If the plan is to replace them with Temporals, it's hard to say the creation of Temporals can be any faster than Date objects in the near future (as new features typically take more time to optimize).

A positive aspect of the breaking change might be that we can check how the community reacts to it, maybe people want more of these changes. It won't be particularly representative, but maybe a pool can be made about this and it might provide useful information?

Note that to this day ~30% of Node.js users use v18, ~10% of Node.js users uses v20 and ~5% uses v21, the rest are mostly on EOL release lines. Whatever semver-major changes we land here won't reach more than 10% of the users until 1 year later, meanwhile breakages will just make it harder for people to upgrade so the more breakages we do the slower it is for optimizations to reach end users (especially if the path it's not critical for their use case). If it's semver-patch, however, it can reach end users on v18 and v20 fairly quickly.

I think we should reduce breakages to the minimum in order for more users to benefit from the optimization soon-ish. If lazily creating Date objects in own property accessors is still faster than eagerly creating Date objects, then we can do that as semver-patch and backport them to v20 and v18. Then do a semver-major to move the accessor to the prototype to reduce the overhead of the Object.defineProperties in the constructor. The class conversion would be semver-major and seems unnecessary for the optimization being done here, because we need accessors for the laziness and class fields aren't accessors.

[anonrig]

I implemented a non-breaking version of lazy getters, which is 30% slower than the current one due to the overhead of ObjectDefineProperties. Any suggestions?

❯ node-benchmark-compare fs.csv
                                                                                  confidence improvement accuracy (*)   (**)  (***)
fs/bench-statSync-failure.js throwType='noThrow' statSyncType='lstatSync' n=10000                -0.56 %       ±1.29% ±1.72% ±2.26%
fs/bench-statSync-failure.js throwType='noThrow' statSyncType='statSync' n=10000                 -0.12 %       ±0.97% ±1.29% ±1.68%
fs/bench-statSync-failure.js throwType='throw' statSyncType='fstatSync' n=10000                  -0.41 %       ±0.71% ±0.95% ±1.24%
fs/bench-statSync-failure.js throwType='throw' statSyncType='lstatSync' n=10000                  -0.44 %       ±0.92% ±1.23% ±1.60%
fs/bench-statSync-failure.js throwType='throw' statSyncType='statSync' n=10000                    0.50 %       ±1.39% ±1.85% ±2.41%
fs/bench-statSync.js statSyncType='fstatSync' n=10000                                    ***    -39.11 %       ±0.80% ±1.07% ±1.39%
fs/bench-statSync.js statSyncType='lstatSync' n=10000                                    ***    -30.38 %       ±1.30% ±1.73% ±2.27%
fs/bench-statSync.js statSyncType='statSync' n=10000                                     ***    -31.45 %       ±0.81% ±1.08% ±1.41%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 8 comparisons, you can thus expect the following amount of false-positive results:
  0.40 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.08 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

[joyeecheung]

It seems the slowdown mostly comes from runtime call overhead and jumping C++/JS hoops in V8. I think in that case the only thing that can be done on Node.js's side is implementing it as accessors on the prototype. Though that's a semver-major change so won't be released until next April.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/56351/

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/56367/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/56420/

[nodejs-github-bot]

Landed in abbdc3e