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

lib: reduce startup time #20567

Closed
wants to merge 20 commits into from
Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 7, 2018

This reduces the overall startup time by > 10 % on my local machine (comparing 100 runs) by lazy loading a couple modules. At the same time it also reduces the memory usage:

node-cp$ out/Release/node -e 'console.log(process.memoryUsage())'
{ rss: 38727680,
  heapTotal: 6086656,
  heapUsed: 3687208,
  external: 8264 }
node-cp$ node -e 'console.log(process.memoryUsage())'
{ rss: 24526848,
  heapTotal: 6610944,
  heapUsed: 4465864,
  external: 8672 }

In the bootstrap phase our code is not optimized by the compiler and each file extra takes a while.

We load a total of 96 NativeModules and Bindings when just executing node -e 'console.log(process.moduleLoadList.length)'.

With this patch that is reduced to 73! There might still be a few more modules that could be removed but it is becoming more tricky now.

I tried to use a very minimal invasive approach to actually only lazy load where it made sense. If a module would be necessary one way or the other, I did not rewrite that.

Ping @joyeecheung @hashseed @mcollina

I wonder if it is possible to reduce the streams files a bit further.

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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 7, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented May 7, 2018

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing to keep in mind is that when we start doing snapshots we'll want to revert this

@@ -287,6 +292,7 @@ assert.notEqual = function notEqual(actual, expected, message) {

// The equivalence assertion tests a deep equality relation.
assert.deepEqual = function deepEqual(actual, expected, message) {
if (isDeepEqual === undefined) lazyLoadComparison();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, you could assign isDeepEqual a function that loads and then overwrites itself. That way you can be sure you caught all the use sites.

Copy link
Member

@joyeecheung joyeecheung May 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hashseed Can we just use something like

let comparison;
function lazyComparison() {
  if (comparison === undefined) {
    comparison = require('internal/util/comparisons');
  }
  return comparison;
}

And use lazyComparison() everywhere for simplicy? I guess V8 should be able to inline that?

Copy link
Member

@hashseed hashseed May 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would have to use lazyComparison()(args) the way you defined it. Inlining shouldn't be an issue here.

I'd even go a bit further and define a wrapper that can be reused:

function lazify(loader) {
  let loaded = undefined;
  return function(...args) {
    if (loaded === undefined) {
      loaded = loader();
      loader = undefined;
    }
    return loaded(...args);
  };
}

You can then use this to load comparison:

let comparison = lazify(() => require('internal/util/comparisons'));

I wonder though whether inlining that would be an issue. @bmeurer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran benchmarks on all three of the variations and looking at the numbers it seems like everything gets inlined (both ways). Depending on how I write the benchmark, the outcome is favorable for some other different versions.

'use strict';

let foo;

let tmpBaz;
function baz() {
  if (tmpBaz === undefined) tmpBaz = require('util');
  return tmpBaz;
}

const lazyUtil = lazify(() => require('util'));
function lazify(loader) {
  let loaded;
  return function() {
    if (loaded === undefined) {
      loaded = loader();
      loader = undefined;
    }
    return loaded;
  };
}

function a(a, b) {
  if (foo === undefined) foo = require('util');
  return foo.isDeepStrictEqual(a, b);
}

function b(a, b) {
  return baz().isDeepStrictEqual(a, b);
}

function c(a, b) {
  return lazyUtil().isDeepStrictEqual(a, b);
}

function bench(fn) {
  const arr = [];
  arr.push(fn(0, 0));
  arr.push(fn(1, 1));
  arr.push(fn(2, 2));
  arr.push(fn(3, 3));
  console.time(`Runtime ${fn.name}`);
  for (var i = 0; i < 1e7; i++) {
    // if (i % 1e4 === 0) {
    //   foo = undefined;
    //   tmpBaz = undefined;
    // }
    arr.push(fn(i, i));
  }
  console.timeEnd(`Runtime ${fn.name}`);
  return arr;
}

for (var i = 0; i < 5; i++) {
  bench(a);
  bench(b);
  bench(c);
}

Without the if block, a is the winner with ~450ms vs ~500ms. With the if block, b is the winner with ~450ms vs ~500ms.

I personally normally use it the way as is right now but I see the point for it being error prone. Even though it also some times allows to only lazy load once outside of a loop instead of running a function in a hot loop.

Copy link
Member Author

@BridgeAR BridgeAR May 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should agree on one specific way how to do this. I do not have a strong opinion (anymore) and would like to let others decide which one to pick.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like Yang's way if it is fast.

@joyeecheung
Copy link
Member

one thing to keep in mind is that when we start doing snapshots we'll want to revert this

@devsnek Why though? It does not hurt to make the moduleLoadList cleaner, and it may help the snapshot effort now that there will be less bootstrap code to watch out for external dependencies

@hashseed
Copy link
Member

hashseed commented May 7, 2018

Having more code in the startup snapshot is a double-edged sword. Benefits include less time spent on compiling internal scripts if they end up being required. Drawbacks include more memory use and higher GC pressure.


const assert = require('assert');

assert(process.moduleLoadList.length <= 73, process.moduleLoadList);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better way to test this would be to start a child process printing process._rawDebug(JSON.stringify(process.moduleLoadList)) to stdout to reduce the noise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what way does the current way produce noise?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You won't need to require('assert'), which pollutes the moduleLoadList, I guess.

Copy link
Member Author

@BridgeAR BridgeAR May 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, calling assert could indeed pollute the list. Even though that is currently not the case. But that can be easily worked around by writing:

const list = process.moduleLoadList.slice();

const assert = require('assert');

assert(list <= 73, list);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BridgeAR Using a child process also makes the eslint-disable node-core/required-modules and the comment explaining why unnecessary.

@joyeecheung
Copy link
Member

Having more code in the startup snapshot is a double-edged sword. Benefits include less time spent on compiling internal scripts if they end up being required. Drawbacks include more memory use and higher GC pressure.

@hashseed Yeah also I think for some applications where the start up performance does make a difference, the modules eliminated from the initial moduleLoadList won't be necessary anyway. For example test runners and frontend build systems may not need dns at all, and applications that do need to make dns calls may not care about start up performance since at some point they will wait for a dns query to complete and that could take much longer than the few milliseconds saved by precompiling scripts.

@jasnell
Copy link
Member

jasnell commented May 7, 2018

I'll give this a thorough review later, and I appreciate the separate commits but, to be honest, I'd have much preferred each of these in separate PRs. This one or is touching a LOT of stuff and has a non-zero risk of introducing regressions. It will be important to review very carefully

@BridgeAR
Copy link
Member Author

BridgeAR commented May 7, 2018

@jasnell i combined it because i wanted to see the overall impact of it.

@jasnell
Copy link
Member

jasnell commented May 7, 2018

Yeah, I get it, it just significantly increases the risk.

@@ -483,6 +491,7 @@ Module._load = function(request, parent, isMain) {
}

if (experimentalModules && isMain) {
if (asyncESM === undefined) lazyLoadESM();
asyncESM.loaderPromise.then((loader) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this one requires everything that loads? Mind you, it might just end up getting loaded anyways.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not specifically test for this case but if esm is used everything should be necessary (at least if more than one file is imported). This is just convenience in this case.

// Ordinarily test files must require('common') but that action causes
// the global console to be compiled, defeating the purpose of this test.
// This makes sure no additional files are added without carefully considering
// lazy loading. Please adjust the value if necessary.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me, but I'd certainly like to see a version using Yang's lazy-function approach, it it works out ok.

@BridgeAR
Copy link
Member Author

BridgeAR commented May 14, 2018

I looked into using @hashseed s lazy function approach but after playing around with it I would actually like to change that in a follow up PR and land this as is.

The reason is that this lazy loading approach can only load a specific function and there are multiple patterns how to lazy load that are all a bit different. It is for example not possible to lazy load classes with that either.

Therefore I would like to work on something like this later on.

@Fishrock123 @joyeecheung @hashseed is that fine for you?

@mcollina mcollina mentioned this pull request Oct 23, 2018
4 tasks
@BridgeAR BridgeAR deleted the reduce-startup-time2 branch January 20, 2020 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.