module: optimize platform lookup and initPath #3582
Conversation
This caches the value of `process.platform`, and creates the `modulePaths` array in a way that is more efficient and easier to read and understand (left-to-right instead of right-to-left).
|
Without having looked at the changes, I just want to say that this churn in the module system makes me uncomfortable. It has the potential to break a lot of code. |
|
LGTM |
| @@ -460,29 +461,23 @@ Module.runMain = function() { | |||
| }; | |||
|
|
|||
| Module._initPaths = function() { | |||
| const isWindows = process.platform === 'win32'; | |||
| modulePaths = []; | |||
There was a problem hiding this comment.
Yes. modulePaths is global to this module, and calling _initPaths, resets its value.
Line 485 in 810cc05
There was a problem hiding this comment.
I'd use modulePaths.length = 0.
That doesn't create a new object which has to be garbage collected
There was a problem hiding this comment.
@meinklaus _initPaths is called only once and modulePaths is undefined at the beginning.
There was a problem hiding this comment.
Looking at https://github.com/zertosh/node/blob/module-improv/lib/module.js#L41 I don't think it is undefined.
If _initPaths is only called once there is no need to assign modulePaths again.
There was a problem hiding this comment.
arr.length = 0 is magical and AFIAK it's not a convention that's used here
|
@nodejs/ctc |
|
cc @bmeck |
|
+1 on cache of Is the goal here otherwise to improve startup time? This does have some This affects tests that mutate NODE_PATH: Don't know the history of things that used the trick in these tests, but Also some modules (minimal usage but still): On Mon, Nov 2, 2015 at 10:41 AM, Jeremiah Senkpiel <notifications@github.com
|
|
-0.5 on rest* On Mon, Nov 2, 2015 at 10:56 AM, Bradley Meck bradley.meck@gmail.com
|
|
@bmeck, another goal of the changes in |
|
Not sure this is worth it anymore. Closing to reduce noise. |
This caches the value of
process.platformin a style that is consistent with the fs, os and path modules. It also improvesModule._initPathin both legibility and simplicity. Instead of creating themodulePathsarray from right-to-left viaconcats andunshifts, its now built left-to-right withpushs. Although it is a micro-optimization, the changes make it much easier to read and understand.