assert: lazy load fs

[jasnell]

Originally part of #20734 ...

Lazy load fs in assert

Checklist

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

[jasnell]

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

[joyeecheung]

Lazy loading fs probably does not make a lot of difference given how the bootstrapping works, but if for the sake of consistency, LGTM

[joyeecheung]

Hmm, by the way, maybe we want to have another more global way of knowing if a module is already loaded. In the case of a commonly used module like fs, it does not make a lot of sense to test if the module is loaded by testing if a small subset of methods are assigned to values - it is likely already loaded even before this module is even compiled (or not, since assert is a very commonly used module as well, but it may be the case in other modules)

[gireeshpunathil]

given how the bootstrapping works

@joyeecheung - can you please elaborate on that, for my learning? Does the bootstrap load every core modules that is available in the lib folder at the startup? Even so, there is a minor improvement through lazy loading as it saves couple of JS invocations in the best case? Do we have benchmarks / tests that measures latency with and without lazy loading? thanks!

[gireeshpunathil]

more global way of knowing if a module is already loaded.

Isn't it best leave it to the require API rather than opening up that complexity in the application?

[joyeecheung]

@gireeshpunathil No, we do not read code of internal modules from disk since they are compiled into the binary, but

a) if the binary is started to execute a script provided by users, or do -r, then it's bound to use fs at some point
b) if the binary is started as a repl, then repl is going to look for history files, then it's also going to use fs at some point

The only case where fs is unnecessary that I can think of is node -e but that's not a very popular use case compared to others.

[jasnell]

Lazy loading fs probably does not make a lot of difference given how the bootstrapping works

It's not about avoiding the load, it's about being more deliberate about the ordering of loading and only doing the require lookup when absolutely necessary, even if the module is already in the cache.

[joyeecheung]

Also we do have benchmarks for start up times: https://benchmarking.nodejs.org/ the source should be in the benchmarking repo.

EDIT: see https://github.com/nodejs/benchmarking/tree/master/benchmarks/start_stop_time

[jasnell]

For a super-quick and rough startup benchmark, try node -pe "perf_hooks.performance.nodeTiming.duration"

For something more detailed, #20728 will emit trace events that are quite helpful for profiling when used alongside the node.bootstrap and v8 categories, although they do come with their own overhead.

[joyeecheung]

Lazy loading fs probably does not make a lot of difference given how the bootstrapping works

It's not about avoiding the load, it's about being more deliberate about the ordering of loading and only doing the require lookup when absolutely necessary, even if the module is already in the cache.

@jasnell

Yes, I understand, just trying to look at it from a more distant perspective.

more global way of knowing if a module is already loaded.

Isn't it best leave it to the require API rather than opening up that complexity in the application?

@gireeshpunathil The require used in the internal modules is a different function from the one exposed to the users (see the comments at the top of internal/bootstrap/loaders.js), so we can change our internal require however we want - also, there is already a NativeModule._cache, and NativeModule is not accessible to users, we have tests (test-loaders-hidden-from-users) for that.

[joyeecheung]

By the way, I used perf to see the start up times when I first looked into the bootstrapping performance. On my Ubuntu server with perf version 4.16.rc7.g3eb2ce8(just the version I compiled when I grabbed the linux master) , v8 6.7.173 and node v10.1.0 (not the same v8 version but the order of magnitude should be close anyway)

root@vultr:~# perf stat `which node` -e '1'
Performance counter stats for '/root/.nvm/versions/node/v10.1.0/bin/node -e 1':
103.083916 task-clock (msec) # 0.994 CPUs utilized
79 context-switches # 0.766 K/sec
0 cpu-migrations # 0.000 K/sec
2,187 page-faults # 0.021 M/sec
<not supported> cycles
<not supported> instructions
<not supported> branches
<not supported> branch-misses
0.103723818 seconds time elapsed
root@vultr:~# perf stat "/root/.jsvu/engines/v8/v8" --natives_blob="/root/.jsvu/engines/v8/natives_blob.bin" --snapshot_blob="/root/.jsvu/engines/v8/snapshot_blob.bin" -e '1'
Performance counter stats for '/root/.jsvu/engines/v8/v8 --natives_blob=/root/.jsvu/engines/v8/natives_blob.bin --snapshot_blob=/root/.jsvu/engines/v8/snapshot_blob.bin -e 1':
18.425743 task-clock (msec) # 0.959 CPUs utilized
7 context-switches # 0.380 K/sec
0 cpu-migrations # 0.000 K/sec
3,392 page-faults # 0.184 M/sec
<not supported> cycles
<not supported> instructions
<not supported> branches
<not supported> branch-misses
0.019216215 seconds time elapsed

perf record & perf report would be able to give more details about where all the time goes.

[BridgeAR]

I doubt it is possible to lazy load fs and I do not see a real win by this change therefore. So I tend to -0.5 at the moment. If we have use cases where we do not eagerly load fs I would be +1.

[jasnell]

This is more about deferring the load. I'd like to have it so that assert loads early and quickly without side effects.

[BridgeAR]

@jasnell but there is (almost) no extra load if fs is already loaded somewhere else because the module is now cached?

[jasnell]

Closing this for now. Will revisit it later.