module: addBuiltinLibsToObject refactoring #14085
Conversation
nodejs-github-bot
added
the
module
vsemozhetbyt
added
the
performance
| // Make built-in modules available directly (loaded lazily). | ||
| builtinLibs.forEach((name) => { | ||
| const configurable = true; | ||
| const enumerable = false; |
There was a problem hiding this comment.
I don't think these are necessary. I would just keep these values inline to ensure no variable lookup has to be done by V8.
| object[libname] = value; | ||
| } | ||
|
|
||
| for (var n = 0, len = builtinLibs.length; n < len; n++) { |
There was a problem hiding this comment.
No need to cache the length, V8 can work fine with using .length in the conditional part.
| // Goals of this mechanism are: | ||
| // - Lazy loading of built-in modules | ||
| // - Having all built-in modules available as non-enumerable properties | ||
| // - Allowing the user to re-assign these variables as if there were no | ||
| // pre-existing globals with the same name. | ||
| const libname = builtinLibs[n]; | ||
| const set = setReal.bind(null, libname); |
There was a problem hiding this comment.
Why bind() this method but not the getter? I think we should be consistent. I do not know offhand if either method is more efficient, but since there are not so many built-in modules it may not make much difference.
|
Honestly I would be surprised if these changes made a noticeable improvement in startup time. As @vsemozhetbyt mentioned, it would be a good idea to show that it does via benchmark(s). |
|
If benchmarks will not show a noticeable improvement I will just close this PR then. |
|
This code is used for So I’d look at this patch based on the change in readablity rather than its value as a possible performance improvement. I’m pretty neutral on it – the previous code wasn’t too pretty, so I can understand that there’s room for enhancements. |
addaleax
removed
the
performance
|
@addaleax where/how should I learn the codebase to do useful PR's? I would like to but I am new here. I will close this PR |
Replace forEach-loop by for-loop for several reasons. One of them being performance. Loop over a small array is fast anyway, but this will make code more consistent across the project. Feels like Node.js is trying to be as fast as possible. Move "setReal" function definition out of the loop and make it bindable. Defining a function inside loop is considered bad practice in general. Predefine configurable and enumerable as these are staying consistent across the function.
|
@kuroljov One thing that might help with getting you going is that you might want to look at our test coverage and try to improve that: https://coverage.nodejs.org/ (Linux only atm). If you’re specifically looking for performance improvements, I’m not sure myself. I would assume there are a few low-hanging fruits around since we switched to V8 5.9 on |
@addaleax thank you. @vsemozhetbyt and @mscdex would you suggest something? |
|
@kuroljov You might check the NodeTodo website for some suggestions. |
|
Cross-refs) #14062 (comment) ... |
|
Thank you. This might be helpful |
Replace
forEachloop byforloop for several reasons. One of thembeing performance. Loop over a small array is fast anyway, but this will
make code more consistent across the project. Feels like Node.js is
trying to be as fast as possible
Move
setRealfunction definition out of the loop and makeit bindable. Defining a function inside loop is considered bad
practice in general.
Predefine
configurableandenumerableas these are staying consistentacross the function.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
internal/module