process: specialize building and storage of process.config #24816
Conversation
nodejs-github-bot
added
the
lib / src
joyeecheung
requested review from
addaleax and
devsnek
and removed request for
addaleax
| @@ -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; | |||
There was a problem hiding this comment.
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
joyeecheung
force-pushed
the
process-config
branch
from
84e3a30
to
3bdcfd2
Compare
|
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. |
|
@bnoordhuis Do you mean prefixing it with a |
|
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. |
|
@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 to |
|
Right, but the plan is to move to precompiled snapshots eventually, isn't it? |
|
@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. |
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 |
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 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 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 |
|
Also, I think if we keep it as JSON, we are still able to replace Lines 338 to 339 in 6ccc80c 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 EDIT: or maybe that's still possible if we do a rough search/regexp matching..but having it as JSON is simpler |
|
Also...if we are writing config.gypi as config.js, then we will need to rewrite this somehow: Lines 21 to 23 in 6ccc80c |
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++.
joyeecheung
force-pushed
the
process-config
branch
from
3bdcfd2
to
42ab2d3
Compare
|
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 |
Let's try to ping @nodejs/python first. |
|
Landed in 44a5fe1 |
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++.
PR-URL: #24816
Reviewed-By: Gus Caplan <me@gus.host>
|
This doesn't land cleanly on v11.x, could it please be backported |
|
Ping @joyeecheung |
|
The following ancestor commits of 44a5fe1 are not on v11.x-staging |
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++.
PR-URL: #24816
Reviewed-By: Gus Caplan <me@gus.host>
|
I’ve backported this to v11.x while resolving a tiny merge conflict in the counter in |
refack
added
build
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++.
PR-URL: nodejs#24816
Reviewed-By: Gus Caplan <me@gus.host>
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++.
PR-URL: nodejs#24816
Reviewed-By: Gus Caplan <me@gus.host>
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._sourceand delete itlater, 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), orvcbuild test(Windows) passes