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

src: cache the result of GetOptions() in JS land #24091

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@joyeecheung
Copy link
Member

joyeecheung commented Nov 4, 2018

Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
internal/options module for faster access to the values in JS land.

src: cache the result of GetOptions() in JS land
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

@joyeecheung joyeecheung requested a review from addaleax Nov 4, 2018

@joyeecheung

This comment has been minimized.

@addaleax
Copy link
Member

addaleax left a comment

LGTM with 1 comment

@@ -211,8 +211,7 @@

NativeModule.isInternal = function(id) {
return id.startsWith('internal/') ||
(id === 'worker_threads' &&
!internalBinding('options').getOptions('--experimental-worker'));
(id === 'worker_threads' && !config.experimentalWorker);

This comment has been minimized.

@addaleax

addaleax Nov 4, 2018

Member

Can we avoid going back to process.config? 😕

This comment has been minimized.

@joyeecheung

joyeecheung Nov 4, 2018

Member

There are also two other reference to config in here, I think a better option would probably be to just specialize something for NativeModule here - maybe just directly generate the whole map when the process starts and pass it to the loader bootstrapper here and let NativeModule short-circuit the internal/options implementation. That also opens up the opportunity to emit all the warning for experimental modules in one place here instead of emitting them everywhere across the code base.

EDIT: wait, we emit them regardless of the cli options..so never mind I guess

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Nov 5, 2018

Fixed linter and the bootstrap module list test: https://ci.nodejs.org/job/node-test-pull-request/18336/

@cjihrig

cjihrig approved these changes Nov 5, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Nov 5, 2018

@refack

This comment has been minimized.

Copy link
Member

refack commented Nov 5, 2018

Instead of calling into C++ each time

I get that motivation, but couldn't we make it simpler? I'd be happy to share your reasoning.

@refack refack added process cli labels Nov 5, 2018

@devsnek

devsnek approved these changes Nov 5, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Nov 5, 2018

@refack See #24091 (comment) my reasoning is we could just eliminate calling into C++ at all eventually by creating the map before bootstrapping loaders so it will be made available in loafers.js as an argument directly. This PR just moves the calls to a central place that can be manipulated in loaders.js

@refack

refack approved these changes Nov 5, 2018

@refack

This comment has been minimized.

Copy link
Member

refack commented Nov 5, 2018

Resume: https://ci.nodejs.org/job/node-test-commit/22901/
(previous fail was infra, should be resolved)

@refack refack added the author ready label Nov 5, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Nov 6, 2018

Just FYI there are others who disagree (that's one of the reasons test/parallel/test-bootstrap-modules.js exists

@refack Just to explore about this point a bit further - I think the reason why test/parallel/test-bootstrap-modules.js exists is to make sure we load less stuff during the bootstrap if they are not really necessary. In this case, the options have to be loaded during the bootstrap no matter what since it is conceptually a dependency of the bootstrap process (whereas things like TCPWrap really are not and they are the main targets that test/parallel/test-bootstrap-modules.js tests against).
In that regard, the length of the module load list is not a very effective measure, as the compilation of a very simple module like options.js that does not depend on too many other modules only incurs a very small overhead (whereas other bigger modules may incur a much bigger overhead by running code synchronously and even invoking bindings that do much more). But again, we don't actually have a better measure to evaluate the "weight" of these native modules..so that remains to be investigated.

@refack

This comment has been minimized.

Copy link
Member

refack commented Nov 6, 2018

I think the reason why test/parallel/test-bootstrap-modules.js exists is to make sure we load less stuff during the bootstrap if they are not really necessary.

Main reason sure, but it's also been quoted as a braier for adding new (trivial) modules - #23081 (review)

Again, I'm slightly in favor... Encapsulation/Performance is just a balance we should keep in mind.

@refack

This comment has been minimized.

Copy link
Member

refack commented Nov 6, 2018

ubuntu-on-arm had a flake, so resume again: https://ci.nodejs.org/job/node-test-commit/22902/

@Trott Trott closed this Nov 8, 2018

Trott added a commit to Trott/io.js that referenced this pull request Nov 8, 2018

src: cache the result of GetOptions() in JS land
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

PR-URL: nodejs#24091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Nov 8, 2018

Landed in f895b5a

amel-true added a commit to amel-true/node that referenced this pull request Nov 10, 2018

src: cache the result of GetOptions() in JS land
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

PR-URL: nodejs#24091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>

tadhgcreedon pushed a commit to tadhgcreedon/node that referenced this pull request Nov 11, 2018

src: cache the result of GetOptions() in JS land
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

PR-URL: nodejs#24091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>

BridgeAR added a commit that referenced this pull request Nov 14, 2018

src: cache the result of GetOptions() in JS land
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

PR-URL: #24091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@BridgeAR BridgeAR referenced this pull request Nov 14, 2018

Merged

Release proposal: v11.2 #24350

kiyomizumia added a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018

src: cache the result of GetOptions() in JS land
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

PR-URL: nodejs#24091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>

codebytere added a commit that referenced this pull request Dec 13, 2018

src: cache the result of GetOptions() in JS land
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

PR-URL: #24091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins added a commit that referenced this pull request Dec 22, 2018

src: cache the result of GetOptions() in JS land
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

PR-URL: #24091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@MylesBorins MylesBorins referenced this pull request Dec 22, 2018

Merged

v10.15.0 proposal #25176

MylesBorins added a commit that referenced this pull request Dec 25, 2018

src: cache the result of GetOptions() in JS land
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

PR-URL: #24091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment