lib: improve lazy loading #14167

BridgeAR · 2017-07-11T13:30:54Z

The function call plus the existence check is more expensive than inlining the lazy require.

A few lazyAssert calls were obsolete as the assert would have to be required before.

I also moved a few lazy requires in the specific code path as it would not be required at all in some cases.

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

Affected core subsystem(s)

lib

The lazy assert should already be in place when calling any of the E functions.

refack · 2017-07-11T14:57:49Z

I don't like it as is. It spreads the require / process.binding, making it hard to audit dependencies.
What you could do is, close to the top:

Object.defineProperty(this, 'lasyErrors', {
  get() {
    const value = require('errors');
    Object.defineProperty(this, 'lasyErrors', {value});
    return value;
  }
);

Fishrock123 · 2017-07-11T15:22:42Z

Does this expense actually add up to anything significant in a realistic scenario?

BridgeAR · 2017-07-11T15:36:04Z

@refack I guess you mean something like the following?

const lazy = {};

Object.defineProperty(lazy, 'util', {
  configurable: true,
  get() {
    const value = require('util');
    Object.defineProperty(lazy, 'util', { value, enumerable: true });
    return value;
  }
});

const util = lazy.util;

I know that spreading is not nice but if it's a function call or not is not much of a difference in that case.

@Fishrock123 I'll run some benchmarks later on. I just checked the different lazy loading times them self and those are way faster.

refack · 2017-07-11T16:17:17Z

I know that spreading is not nice but if it's a function call or not is not much of a difference in that case.

I agree that dynamically it's the same, the difference is in static analysis, if it's a function (or property) at the top it's easier to spot, visually or with tools.

As for a property you can set it on the module or on an explicit lazy holder. Then use modules.util or lazy.util all over

refack · 2017-07-11T16:28:48Z

Actually lazy is a better choice (I keep discovering that and forgetting it):

const lazy = {
  get util() {
    const value = require('util');
    Object.defineProperty(this, 'util', { value, enumerable: true });
    return value;
  },
  ...
}

jasnell · 2017-07-12T15:33:26Z

Overall I'm not a fan. I prefer to keep lazy loading only when absolutely necessary, mostly to avoid circular references caused by early loading.

refack · 2017-07-12T18:35:37Z

lib/internal/process.js

-  }
-  return _lazyConstants;
-}
+var lazyConstants;


Re #14167 (comment) comment this could be inlined.

refack · 2017-07-12T18:36:14Z

lib/internal/process/stdio.js

-    errors = require('internal/errors');
-  return errors;
-}
+var lazyError;


refack · 2017-07-12T18:36:32Z

lib/internal/process/warning.js

@@ -5,24 +5,12 @@ const prefix = `(${process.release.name}:${process.pid}) `;

 exports.setup = setupProcessWarnings;

-var errors;
-var fs;
+var lazyErrors;


errors can be inlined

refack · 2017-07-12T23:26:38Z

Simpler which is definitely better 👍
What about the lazy property holder? Should be performant, and readable...

Do not lazy load if not necessary and prevent function calls.

BridgeAR · 2017-07-12T23:33:43Z

@refack there's still the property lookup and it is still faster to just check if the variable is defined or not. This was the way how lazy loading was used in other spots as well and with the simple if it should hopefully be readable again.

refack · 2017-07-12T23:34:57Z

@refack there's still the property lookup and it is still faster to just check if the variable is defined or not. This was the way how lazy loading was used in other spots as well and with the simple if it should hopefully be readable again.

Ack
(but only because we took a %200 startup slowdown hit and have been chasing timing out tests all day 😉)

BridgeAR · 2017-07-12T23:37:25Z

lib/net.js

-var cluster;
-var dns;
+// Lazily loaded
+var cluster = null;


This could probably be required up front as well but cluster is likely not required in many places that use net and therefore I kept it as is.

So could you document that:

// `cluster` is only used by `listenInCluster` so for startup performance // reasons it's lazy loaded.

TimothyGu · 2017-07-13T01:34:45Z

LGTM.

Aside from this PR, I'm not too big of a fan of using assert in the code base anywhere other than in tests. Unlike assert(3), the module is unnecessarily big for something that can usually be done in two lines.

BridgeAR · 2017-07-18T17:52:53Z

@refack I added the comment.

I also moved a statement to prevent lazy loading under some conditions.

refack · 2017-07-19T19:02:02Z

The link to the CI: https://ci.nodejs.org/job/node-test-commit/11256/ ✔️

* internal/errors - assert should already be in place when calling any of the message generating functions. * No lazy load if not necessary. * Replace function calls with `if`s. PR-URL: nodejs#14167 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

refack · 2017-07-19T19:16:19Z

Landed in b55ab01

addaleax · 2017-07-24T16:58:06Z

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it make sense let me know and we’ll add the dont-land-on label.

jasnell · 2017-09-20T21:07:17Z

Ping @BridgeAR .. re: backport.

* internal/errors - assert should already be in place when calling any of the message generating functions. * No lazy load if not necessary. * Replace function calls with `if`s. PR-URL: nodejs#14167 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

* internal/errors - assert should already be in place when calling any of the message generating functions. * No lazy load if not necessary. * Replace function calls with `if`s. PR-URL: #14167 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

MylesBorins · 2017-10-17T18:14:27Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

BridgeAR · 2017-12-11T06:22:18Z

I do not see the necessity to backport this. So I changed the labels accordingly.

internal/errors: remove obsolete lazy assert

59559ca

The lazy assert should already be in place when calling any of the E functions.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 11, 2017

refack suggested changes Jul 12, 2017

View reviewed changes

lib: improve lazy requires

a2e95aa

Do not lazy load if not necessary and prevent function calls.

BridgeAR force-pushed the improve-lazy-loading branch from e62d447 to a2e95aa Compare July 12, 2017 23:31

refack approved these changes Jul 12, 2017

View reviewed changes

BridgeAR commented Jul 12, 2017

View reviewed changes

BridgeAR added 2 commits July 18, 2017 14:49

fixup: add comment

34732c6

fixup: move a statement

9b657aa

refack closed this Jul 19, 2017

addaleax added the backport-requested-v8.x label Jul 24, 2017

This was referenced Sep 20, 2017

assert: improve AssertionError in case of "Errors" #15025

Closed

[v8.x backport] lib: improve lazy requires #15513

Closed

jasnell added backported-to-v8.x and removed backport-requested-v8.x labels Sep 21, 2017

refack mentioned this pull request Oct 16, 2017

lib: move duplicate spliceOne into internal/util #16221

Closed

2 tasks

MylesBorins added the backport-requested-v6.x label Oct 17, 2017

BridgeAR added dont-land-on-v6.x and removed backport-requested-v6.x labels Dec 11, 2017

BridgeAR deleted the improve-lazy-loading branch April 1, 2019 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: improve lazy loading #14167

lib: improve lazy loading #14167

BridgeAR commented Jul 11, 2017 •

edited

Loading

refack commented Jul 11, 2017

Fishrock123 commented Jul 11, 2017

BridgeAR commented Jul 11, 2017

refack commented Jul 11, 2017 •

edited

Loading

refack commented Jul 11, 2017

jasnell commented Jul 12, 2017

refack Jul 12, 2017

refack Jul 12, 2017

refack Jul 12, 2017

refack commented Jul 12, 2017

BridgeAR commented Jul 12, 2017

refack commented Jul 12, 2017 •

edited

Loading

BridgeAR Jul 12, 2017

refack Jul 12, 2017

TimothyGu commented Jul 13, 2017

BridgeAR commented Jul 18, 2017

refack commented Jul 19, 2017

refack commented Jul 19, 2017

addaleax commented Jul 24, 2017

jasnell commented Sep 20, 2017

MylesBorins commented Oct 17, 2017

BridgeAR commented Dec 11, 2017

lib: improve lazy loading #14167

lib: improve lazy loading #14167

Conversation

BridgeAR commented Jul 11, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Jul 11, 2017

Fishrock123 commented Jul 11, 2017

BridgeAR commented Jul 11, 2017

refack commented Jul 11, 2017 • edited Loading

refack commented Jul 11, 2017

jasnell commented Jul 12, 2017

refack Jul 12, 2017

Choose a reason for hiding this comment

refack Jul 12, 2017

Choose a reason for hiding this comment

refack Jul 12, 2017

Choose a reason for hiding this comment

refack commented Jul 12, 2017

BridgeAR commented Jul 12, 2017

refack commented Jul 12, 2017 • edited Loading

BridgeAR Jul 12, 2017

Choose a reason for hiding this comment

refack Jul 12, 2017

Choose a reason for hiding this comment

TimothyGu commented Jul 13, 2017

BridgeAR commented Jul 18, 2017

refack commented Jul 19, 2017

refack commented Jul 19, 2017

addaleax commented Jul 24, 2017

jasnell commented Sep 20, 2017

MylesBorins commented Oct 17, 2017

BridgeAR commented Dec 11, 2017

BridgeAR commented Jul 11, 2017 •

edited

Loading

refack commented Jul 11, 2017 •

edited

Loading

refack commented Jul 12, 2017 •

edited

Loading