module: optimize platform lookup and initPath

[zertosh]

This caches the value of process.platform in a style that is consistent with the fs, os and path modules. It also improves Module._initPath in both legibility and simplicity. Instead of creating the modulePaths array from right-to-left via concats and unshifts, its now built left-to-right with pushs. Although it is a micro-optimization, the changes make it much easier to read and understand.

[cjihrig]

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.

[Trott]

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

[mscdex]

LGTM

[evanlucas]

Is this necessary?

[zertosh]

Yes. modulePaths is global to this module, and calling _initPaths, resets its value.

node/lib/module.js

Line 485 in 810cc05

modulePaths = paths;

[ajafff]

I'd use modulePaths.length = 0.
That doesn't create a new object which has to be garbage collected

[targos]

@meinklaus _initPaths is called only once and modulePaths is undefined at the beginning.

[ajafff]

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.

[zertosh]

arr.length = 0 is magical and AFIAK it's not a convention that's used here

[jasnell]

@nodejs/ctc

[Fishrock123]

cc @bmeck

[bmeck]

+1 on cache of isWindows

Is the goal here otherwise to improve startup time? This does have some
breakage in the wild.

This affects tests that mutate NODE_PATH:
https://github.com/nodejs/node/search?utf8=%E2%9C%93&q=_initPaths&type=Code

Don't know the history of things that used the trick in these tests, but
might be a good place to research before pulling.

Also some modules (minimal usage but still):
http://searchnod.es/#!_initPaths/1

On Mon, Nov 2, 2015 at 10:41 AM, Jeremiah Senkpiel <[email protected]

wrote:

cc @bmeck https://github.com/bmeck

—
Reply to this email directly or view it on GitHub
#3582 (comment).

[bmeck]

-0.5 on rest*

On Mon, Nov 2, 2015 at 10:56 AM, Bradley Meck [email protected]
wrote:

+1 on cache of isWindows

Is the goal here otherwise to improve startup time? This does have some
breakage in the wild.

This affects tests that mutate NODE_PATH:
https://github.com/nodejs/node/search?utf8=%E2%9C%93&q=_initPaths&type=Code

Don't know the history of things that used the trick in these tests, but
might be a good place to research before pulling.

Also some modules (minimal usage but still):
http://searchnod.es/#!_initPaths/1

On Mon, Nov 2, 2015 at 10:41 AM, Jeremiah Senkpiel <
[email protected]> wrote:

cc @bmeck https://github.com/bmeck

—
Reply to this email directly or view it on GitHub
#3582 (comment).

[zertosh]

@bmeck, another goal of the changes in _initPath is to improve legibility – no functionally changes. If that's a blocker for just the isWindows change, then I'd be more than happy to just do that then.

[zertosh]

Not sure this is worth it anymore. Closing to reduce noise.