lib: lazy loading and cleanup

[jasnell]

This is a start towards an very incremental refactoring of core module loading the occurs at startup. Every core module that is loaded automatically adds to the overall cold start time of the node.js process. While this specific PR is not likely to have much impact, the goal is to start moving towards lazy-loading where it is practical. Fixed a bug or two and cleaned up a few other bits along the way.

Checklist

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

[mscdex]

We should definitely have some benchmark runs for the affected modules to make sure no runtime overhead is incurred with these changes.

[jasnell]

Yep, I'll be running some in a bit. I was fairly selective to avoid hot paths but benchmarks would definitely be good.

[mscdex]

Perhaps we could do more traditional lazy loading and avoid the call to require() every time, by checking the undefined-ness of one of the variables.

Something like:

{
  let openSync;
  let closeSync;
  let readSync;
  var getBuffer = function getBuffer(filename, assertLine) {
    if (openSync === undefined)
      ({ openSync, closeSync, readSync } = require('fs'));

    // ...
  };
}

Or we could just move the variables to the top level since some of them are used in other places to avoid requiring the same function (e.g. openSync) multiple times for different fs functions (e.g. getBuffer and getErrMessage)?

[BridgeAR]

I suggest to move the require in the if (warn) check to only require it once and to use a extra variable.

[BridgeAR]

path is definitely always loaded eager and I do not see how we can prevent that as it is necessary during bootstrapping. So this should probably not be lazy loaded.

[BridgeAR]

If I am not mistaken it will not (easily) be possible to lazy load internal/util. I tried to do that and it is used for a lot of things that are eagerly loaded.

[BridgeAR]

I think it is great to do more lazy loading but this will only reduce two files loaded. One is dns (that conflicts with #20567) and the other one is internal/fs/promises.

However, this changes multiple requires to lazy load that are going to be loaded on bootstrap one way or the other. When working on #20567 I payed careful attention to only lazy load things that won't be lazy loaded otherwise. So I personally would like to have a approach where we always only add lazy loading where we make sure that a file is indeed not required for bootstrapping anymore. This should be easy as soon as #20567 landed by just lowering the number of files loaded in the corresponding test. That way we are able to verify the gain instead of just complicating code paths that won't have any benefit otherwise (even though there are a couple of other things in this PR that are worth doing).

[jasnell]

I think it is great to do more lazy loading but this will only reduce two files loaded. One is dns (that conflicts with #20567) and the other one is internal/fs/promises.

As I said in the original post: "While this specific PR is not likely to have much impact, the goal is to start moving towards lazy-loading where it is practical. Fixed a bug or two and cleaned up a few other bits along the way."

The goal here is not to be comprehensive but to take a very incremental and slow approach to ensure things aren't broken as we go.

[jasnell]

Thinking about this PR again... I'm going to split the commits up into smaller PRs. Too much happening in this one.

[jasnell]

Moved various commits out to separate PRs. Remaining commits in this PR can be handled separately. Closing.