process: specialize building and storage of process.config #24816

joyeecheung · Dec 3, 2018

Instead of treating config.gypi as a JavaScript file, specialize
the processing in js2c and make the serialized result a real JSON
string (with 'true' and 'false' converted to boolean values) so
we don't have to use a custom deserializer during bootstrap.

In addition, store the JSON string separately in NativeModuleLoader,
and keep it separate from the map of the builtin source code, so
we don't have to put it onto NativeModule._source and delete it
later, though we still preserve it in process.binding('natives'),
which we don't use anymore.

This patch also makes the map of builtin source code and the
config.gypi string available through side-effect-free getters
in C++.

Checklist

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

nodejs-github-bot · Dec 3, 2018

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1815/pipeline

joyeecheung · Dec 3, 2018

CI: https://ci.nodejs.org/job/node-test-commit/23912/

joyeecheung · Dec 3, 2018

joyeecheung reviewed Dec 3, 2018

View changes

joyeecheung · Dec 3, 2018

test/parallel/test-bootstrap-modules.js

@@ -9,7 +9,7 @@ const common = require('../common');
 const assert = require('assert');

 const isMainThread = common.isMainThread;
-const kMaxModuleCount = isMainThread ? 58 : 80;
+const kMaxModuleCount = isMainThread ? 59 : 81;


This is incremented because previously we use getInternalBinding to load native_module which bypasses the caching and process.moduleLoadList bookkeeping. This patch uses internalBinding instead so it is now being counted into the process.moduleLoadList

devsnek · Dec 3, 2018

devsnek approved these changes Dec 3, 2018

View changes

joyeecheung · Dec 4, 2018

Resume: https://ci.nodejs.org/job/node-test-commit/23916/

joyeecheung · Dec 4, 2018

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

bnoordhuis · Dec 4, 2018

If you're going through all that preprocessing trouble anyway, can't you turn config.gypi into a real JS object literal, write that to a file and include it in the build? That gets rid of the overhead of deserializing it, too.

joyeecheung · Dec 4, 2018

@bnoordhuis Do you mean prefixing it with a module.exports = so that it can be required? We will still need to parse and compile the content of that string, and we still need to delete it from NativeModule._source so it always needs some special treatment, I am not sure what the benefit is compared to JSON.parse?

bnoordhuis · Dec 4, 2018

I mean that you write a .js file that contains this:

module.exports = {
  target_defaults: {
    // ...
  },
  variables: {
    // ...
  },
};

I.e., don't store it as a string, store it as a JS object literal.

joyeecheung · Dec 4, 2018

@bnoordhuis But it will still be stored, in the binary, as a string, just like the source code of other builtin modules? So essentially we go from doing this

// C++
source_[id].ToStringChecked();  // pass it into to JS
// JS
JSON.parse(someStringFromCpp);

to

// C++
auto str = source[id].ToStringChecked();
ScriptCompiler::Source source(str, ...);
auto func = ScriptCompiler::CompileFunctionInContext(...source...);  // return to JS

// JS
const fn = compileFunction(id);
fn(process, ...);  // where you get the module.exports

bnoordhuis · Dec 4, 2018

Right, but the plan is to move to precompiled snapshots eventually, isn't it?

joyeecheung · Dec 5, 2018

@bnoordhuis Eventually, maybe (I believe we are currently stuck on gyp refactoring)...but the result of JSON.parse can also be part of the snapshot? And it is somewhat clearer to me if the config gets some special treatment like this because it has to be treated specially in the end since it’s not require-able, and it’s eassentially data, not code.

joyeecheung · Dec 5, 2018

Resume: https://ci.nodejs.org/job/node-test-pull-request/19221/

bnoordhuis · Dec 5, 2018

but the result of JSON.parse can also be part of the snapshot?

Okay, fair enough. My point was more that if you're putting in the work to make it better, why not go the extra mile to make it great? The only reason it's using JSON.parse() at all is that it was the least amount of effort back when process.config was added.

joyeecheung · Dec 5, 2018

Okay, fair enough. My point was more that if you're putting in the work to make it better, why not go the extra mile to make it great? The only reason it's using JSON.parse() at all is that it was the least amount of effort back when process.config was added.

I think the reason would be, I personally think using JSON.parse is a better approach already? If ew do it with an object literal, aside from adding the module.exports prefix in js2c.py, I think we'd also need to restore the delete NativeModule._source.config after setting up process.config, or exclude it from the list of module that can be required in another way somehow - to me it's easier to understand if config does not show up in that list in the first place.

And this brings us back to #24816 (comment) - we will be compiling and executing code to grab the data out instead of simply parsing it as data. It does unify the processing but config is already just different from other native modules - the generated content completely depends on how you run configure, and it's only executable by us when we bootstrap, and doesn't contain any code that will be executed again after that.

Or is there any reason that it's better to make the code of config executable? Like...making it require-able if you do a require('./config') in the repl from the project directory without building the binary? But I don't know what's the use case of that..?

joyeecheung · Dec 5, 2018

Also, I think if we keep it as JSON, we are still able to replace

node/Makefile

Lines 338 to 339 in 6ccc80c

    
           node_use_openssl = $(call available-node,"-p" \ 
        
           		   "process.versions.openssl != undefined")

with something done via python. Currently, if you touch something in the docs, and you just had a bad build, this will error when you try building the binary again via make test because the dependency in the Makefile is kind of messy ATM.

EDIT: or maybe that's still possible if we do a rough search/regexp matching..but having it as JSON is simpler

joyeecheung · Dec 5, 2018

Also...if we are writing config.gypi as config.js, then we will need to rewrite this somehow:

node/tools/install.py

Lines 21 to 23 in 6ccc80c

    
           def load_config(): 
        
             s = open('config.gypi').read() 
        
             return ast.literal_eval(s)

joyeecheung · Dec 6, 2018

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

joyeecheung · Dec 6, 2018

Resume: https://ci.nodejs.org/job/node-test-pull-request/19284

joyeecheung · Dec 8, 2018

Resume: https://ci.nodejs.org/job/node-test-pull-request/19321/

joyeecheung · Dec 8, 2018

I plan to land this after the 7 day wait for single approval is passed and when I manage to get the CI green, because it's conflicting with another patch of mine to share the code cache among worker threads.

Regarding rewriting config.gypi as a JS object literal, I think we can leave that to another patch if we want to do it, but mind that would involve changes to install.py which currently assumes that config.gypi can be evaluated as a python dict.

targos · Dec 8, 2018

I plan to land this after the 7 day wait for single approval is passed

Let's try to ping @nodejs/python first.

joyeecheung · Dec 10, 2018

Landed in 44a5fe1

MylesBorins · Dec 25, 2018

This doesn't land cleanly on v11.x, could it please be backported

BridgeAR · Jan 9, 2019

Ping @joyeecheung

joyeecheung · Jan 11, 2019

The following ancestor commits of 44a5fe1 are not on v11.x-staging

    - 14707b681d lib: fix nits in lib/internal/bootstrap/cache.js, https://github.com/nodejs/node/pull/24581
         lib/internal/bootstrap/cache.js
    - 7778c035a0 src: use STL containers instead of v8 values for static module data, https://github.com/nodejs/node/pull/24384
         lib/internal/bootstrap/cache.js
         src/env.h
         src/node_native_module.cc
         src/node_native_module.h
         src/node_union_bytes.h
         tools/js2c.py
    - bd765d61d7 src: compile native modules and their code cache in C++, https://github.com/nodejs/node/pull/24221
         lib/internal/bootstrap/cache.js
         src/env.h
         src/node_native_module.cc
         src/node_native_module.h
    - 350bef6a10 lib: add crypto dependant modules cannotUseCache, https://github.com/nodejs/node/pull/24100
         lib/internal/bootstrap/cache.js
    - 9c0c3f1443 lib: fix code cache generation, https://github.com/nodejs/node/pull/23855
         lib/internal/bootstrap/cache.js
    - 63b06551f4 src,lib: make process.binding('config') internal, https://github.com/nodejs/node/pull/23400
         lib/internal/bootstrap/loaders.js
         lib/internal/bootstrap/node.js
    - edcb950090 src: use NativeModuleLoader to compile all the bootstrappers, https://github.com/nodejs/node/pull/24775
         lib/internal/bootstrap/loaders.js
         lib/internal/bootstrap/node.js
    - 1db808ca5f lib: fix comment nits in bootstrap\loaders.js, https://github.com/nodejs/node/pull/24641
         lib/internal/bootstrap/loaders.js
    - a89b873e63 lib: remove duplicated noop function, https://github.com/nodejs/node/pull/24770
         lib/internal/bootstrap/node.js
    - 976065d9cb lib: do not register DOMException in a module, https://github.com/nodejs/node/pull/24708
         lib/internal/bootstrap/node.js
    - 36f483b79b lib: move setupAllowedFlags() into per_thread.js, https://github.com/nodejs/node/pull/24704
         lib/internal/bootstrap/node.js
         lib/internal/process/per_thread.js
    - 7b8058a39e process: refactor the bootstrap mode branching for readability, https://github.com/nodejs/node/pull/24673
         lib/internal/bootstrap/node.js
    - 0f18a40374 lib: change callbacks to arrow function, https://github.com/nodejs/node/pull/24625
         lib/internal/bootstrap/node.js
    - 771585fcf5 lib: refactor setupInspector in bootstrap/node.js, https://github.com/nodejs/node/pull/24446
         lib/internal/bootstrap/node.js
    - 5877836a33 src: remove pushValueToArray and SetupProcessObject, https://github.com/nodejs/node/pull/24264
         lib/internal/bootstrap/node.js
         src/env.h
    - 4ebb3f35e2 process: simplify check in previousValueIsValid(), https://github.com/nodejs/node/pull/24836
         lib/internal/process/per_thread.js
    - 87b808f761 src,lib: move `natives` and `constants` to `internalBinding()`, https://github.com/nodejs/node/pull/23663
         lib/internal/process/per_thread.js
    - 61a89630ee inspector: split the HostPort being used and the one parsed from CLI, https://github.com/nodejs/node/pull/24772
         src/env.h
    - ee80aaab13 http2: set js callbacks once, https://github.com/nodejs/node/pull/24063
         src/env.h
    - fe303b9b2d tls: include elliptic curve X.509 public key info, https://github.com/nodejs/node/pull/24358
         src/env.h
    - 5f9b624766 src: re-sort the symbol macros, https://github.com/nodejs/node/pull/24382
         src/env.h
    - d4654d89be deps: introduce `llhttp`, https://github.com/nodejs/node/pull/24059
         src/env.h
    - f01518edfd src: improve StreamBase write throughput, https://github.com/nodejs/node/pull/23843
         src/env.h
    - 5c5bb36b74 src: avoid extra `Persistent` in `DefaultTriggerAsyncIdScope`, https://github.com/nodejs/node/pull/23844
         src/env.h
    - aa943d098e http: make parser choice a runtime flag, https://github.com/nodejs/node/pull/24739
         src/node_binding.cc
    - 3d668262a0 src: move C++ binding/addon related code into node_binding{.h, .cc}, https://github.com/nodejs/node/pull/24701
         src/node_binding.cc
    - 0c65314e0e src: fix type mismatch warnings from missing priv, https://github.com/nodejs/node/pull/24737
         src/node_native_module.cc
         src/node_native_module.h
    - 804138093a src: use NativeModuleLoader to compile per_context.js, https://github.com/nodejs/node/pull/24660
         src/node_native_module.cc
         src/node_native_module.h
    - 333783643e console: lazy load process.stderr and process.stdout, https://github.com/nodejs/node/pull/24534
         test/parallel/test-bootstrap-modules.js
    - f895b5a58e src: cache the result of GetOptions() in JS land, https://github.com/nodejs/node/pull/24091
         test/parallel/test-bootstrap-modules.js
    - 26b58eabc6 tools: prepare tools/js2c.py for Python 3, https://github.com/nodejs/node/pull/24798
         tools/js2c.py

addaleax · Jan 14, 2019

I’ve backported this to v11.x while resolving a tiny merge conflict in the counter in test/parallel/test-bootstrap-modules.js in 0e2fbe4.

nodejs-github-bot added the lib / src label Dec 3, 2018

joyeecheung requested review from addaleax and devsnek and removed request for addaleax Dec 3, 2018

joyeecheung force-pushed the joyeecheung:process-config branch from 84e3a30 to 3bdcfd2 Dec 4, 2018

joyeecheung force-pushed the joyeecheung:process-config branch from 3bdcfd2 to 42ab2d3 Dec 6, 2018

joyeecheung closed this Dec 10, 2018

MylesBorins added the backport-requested-v11.x label Dec 25, 2018

targos added this to Backport requested in v11.x Dec 28, 2018

addaleax removed the backport-requested-v11.x label Jan 14, 2019

refack added build python labels Jan 14, 2019

BridgeAR moved this from Backport requested to Backported in v11.x Jan 14, 2019

BridgeAR referenced this pull request Jan 16, 2019
Merged
v11.7.0 proposal #25537

MylesBorins referenced this pull request Jan 24, 2019
Merged
v11.8.0 proposal #25687

joyeecheung referenced this pull request Jan 29, 2019
Open
build,tools: refactor build config into config.json #25798
2 of 2 tasks complete

nodejs/node

Perfect your code

process: specialize building and storage of process.config #24816

Conversation

joyeecheung commented Dec 3, 2018

Checklist

This comment has been minimized.

nodejs-github-bot commented Dec 3, 2018

nodejs-github-bot added the lib / src label Dec 3, 2018

This comment has been minimized.

joyeecheung commented Dec 3, 2018

joyeecheung requested review from addaleax and devsnek and removed request for addaleax Dec 3, 2018

joyeecheung reviewed Dec 3, 2018 View changes

This comment has been minimized.

joyeecheung Dec 3, 2018

devsnek approved these changes Dec 3, 2018 View changes

This comment has been minimized.

joyeecheung commented Dec 4, 2018

joyeecheung force-pushed the joyeecheung:process-config branch from 84e3a30 to 3bdcfd2 Dec 4, 2018

This comment has been minimized.

joyeecheung commented Dec 4, 2018 • edited

This comment has been minimized.

bnoordhuis commented Dec 4, 2018

This comment has been minimized.

joyeecheung commented Dec 4, 2018 • edited

This comment has been minimized.

bnoordhuis commented Dec 4, 2018

This comment has been minimized.

joyeecheung commented Dec 4, 2018

This comment has been minimized.

bnoordhuis commented Dec 4, 2018

This comment has been minimized.

joyeecheung commented Dec 5, 2018

This comment has been minimized.

joyeecheung commented Dec 5, 2018

This comment has been minimized.

bnoordhuis commented Dec 5, 2018

This comment has been minimized.

joyeecheung commented Dec 5, 2018

This comment has been minimized.

joyeecheung commented Dec 5, 2018 • edited

This comment has been minimized.

joyeecheung commented Dec 5, 2018

joyeecheung force-pushed the joyeecheung:process-config branch from 3bdcfd2 to 42ab2d3 Dec 6, 2018

This comment has been minimized.

joyeecheung commented Dec 6, 2018 • edited

This comment has been minimized.

joyeecheung commented Dec 6, 2018

This comment has been minimized.

joyeecheung commented Dec 8, 2018

This comment has been minimized.

joyeecheung commented Dec 8, 2018

This comment has been minimized.

targos commented Dec 8, 2018

This comment has been minimized.

joyeecheung commented Dec 10, 2018

joyeecheung closed this Dec 10, 2018

joyeecheung added a commit that referenced this pull request Dec 10, 2018

MylesBorins added the backport-requested-v11.x label Dec 25, 2018

This comment has been minimized.

MylesBorins commented Dec 25, 2018

targos added this to Backport requested in v11.x Dec 28, 2018

This comment has been minimized.

BridgeAR commented Jan 9, 2019

This comment has been minimized.

joyeecheung commented Jan 11, 2019 • edited

addaleax added a commit that referenced this pull request Jan 14, 2019

This comment has been minimized.

addaleax commented Jan 14, 2019

addaleax removed the backport-requested-v11.x label Jan 14, 2019

refack added build python labels Jan 14, 2019

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

BridgeAR moved this from Backport requested to Backported in v11.x Jan 14, 2019

BridgeAR referenced this pull request Jan 16, 2019

v11.7.0 proposal #25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019

MylesBorins referenced this pull request Jan 24, 2019

v11.8.0 proposal #25687

joyeecheung referenced this pull request Jan 29, 2019

build,tools: refactor build config into config.json #25798

joyeecheung reviewed Dec 3, 2018

View changes

devsnek approved these changes Dec 3, 2018

View changes

joyeecheung force-pushed the joyeecheung:process-config branch from `84e3a30` to `3bdcfd2` Dec 4, 2018

joyeecheung commented Dec 4, 2018 •

edited

joyeecheung commented Dec 4, 2018 •

edited

joyeecheung commented Dec 5, 2018 •

edited

joyeecheung force-pushed the joyeecheung:process-config branch from `3bdcfd2` to `42ab2d3` Dec 6, 2018

joyeecheung commented Dec 6, 2018 •

edited

joyeecheung commented Jan 11, 2019 •

edited