module: resolve _-prefixed dependencies to real path by alexanderGugel · Pull Request #6460 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

module (require())

Description of change

Since #5950, require('some-symlink') no longer resolves the module to its
real path, but instead uses the path of the symlink itself as a cache key.

This change allows users to resolve dependencies to their real path when
required using the _-prefix.

Example using old mechanism (pre v6.0.0):

node_modules/_real-module -> ../real-module
real-modules/index.js

require('./real_module') === require('real_module')

Example using new mechanism (post v6.0.0):

node_modules/real-module -> ../real-module
real-modules/index.js

require('./real_module') !== require('real_module')

As discussed in #3402

Why was the lazy-loading of the extensions removed?

Simplified the control flow, since tryFindPath was split out in order to reduce redundancy (just like readFilePath).

More than happy to add it back in, but seemed like a very marginal cost to me.

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

Looks like it's causing test-cwd-enoent-preload.js to fail just about everywhere.

Despite the test failure, I ran the module loader benchmark before and after this change (and after fixing a const causing an aborted optimization):

module/module-loader.js thousands=50 fullPath=true: ./node-new: 9.8836 ./node-old: 11.831 .. -16.46%
module/module-loader.js thousands=50 fullPath=false: ./node-new: 8.3574 ./node-old: 9.4516 . -11.58%

@mscdex @thealphanerd Fixed.

@mscdex Yes, this is expected, since now we need to check if the prefixed symlink exists for every required module. I don't think there is any way around in any of the proposed solution.

Update Actually there is, but is means the precedence changes (default to new behaviour if both files exist).

The above two lines are contributing to v8 aborting optimization of Module._findPath() because of the const usage. They should be reverted to var for now.

@alexanderGugel ... Thank you very much for this. I'm still skeptical if this is the right approach. As an interim solution, I'm currently leaning towards a flag based fix that conditionally reverts the change that was made with the long term solution being closer to the two cache model I described. I'll have some time to walk through this PR in detail soon tho. Really appreciate you putting this together tho!

Since nodejs#5950, `require('some-symlink')` no longer resolves the module to its real path, but instead uses the path of the symlink itself as a cache key. This change allows users to resolve dependencies to their real path when required using the _-prefix. Example using old mechanism (pre v6.0.0): --- node_modules/_real-module -> ../real-module real-modules/index.js require('./real_module') === require('real_module') Example using new mechanism (post v6.0.0): --- node_modules/real-module -> ../real-module real-modules/index.js require('./real_module') !== require('real_module') As discussed in nodejs#3402

Sorry, was too fast. Didn't actually fix the process.cwd() issue.

That way stat() only needs to be called once in most cases (assuming non _-prefixed symlinks are more common).

@mscdex How can I run those benchmarks? 37b7b5e should fix the perf issue.

This also makes this change mostly backwards compatible with the symlink change introduced in v6.0.0.

The above two lines need to be changed to use var instead of const to avoid aborted optimziations of _findPath() by v8.

fixed.

const in for-loop can not be optimized.

* Fixes `test-cwd-enoent-preload.js` * Minor performance improvement

Actually, this line was fine. It was just the other 3 that were causing problems.

@mscdex Now fixed test-cwd-enoent-preload.js (ca804f5).

@alexanderGugel Assuming node-old is a copy of the binary representing node before this PR, you'd do something like: node benchmark/compare -r -g ./node ./node-old -- module module-loader

With the most recent changes I now see:

module/module-loader.js thousands=50 fullPath=true: ./node: 11.377 ./node-old: 11.781 . -3.43%
module/module-loader.js thousands=50 fullPath=false: ./node: 9.4919 ./node-old: 9.504 . -0.13%

@mscdex Thanks. I would assume the performance regression is most likely because of functions not being optimised rather than unneeded syscalls, since the benchmarks most likely don't even cover those.

@alexanderGugel Can you explain what you mean by "not being optimized?"

37b7b5e means there are no additional stats when the module can be found via a non _-prefixed linkname, meaning that the only scenario in which an additional stat is needed is when the module could not be found, and we need to check if a _-prefixed version exists.

Going to bed now.

I will look into additional optimizations

Ensure that `require('some-module/index.js')` is being resolved properly when `some-module` is a _-prefixed symlink.

@mscdex Any update on the optimizations? Or is ~ 3.4 % in the acceptable range?

Also there seems to be a discussion going on about whether or not the module API is actually locked. #6528 - This will probably affect this PR.

new ci: https://ci.nodejs.org/job/node-test-pull-request/2467/

@alexanderGugel I haven't had time yet unfortunately. I will try to dig into it this week though.

It looks like the commit that broke this in the first place is about to be reverted, which would render this PR obsolete (#6537).

So maybe the default should be changed... use the realpath of symlinks by default and use the path of the symlink if the linkname starts with _ - so exactly the other way around.

That way backwards compatibility with v5.0.0 could be preserved while adding the ability to conditionally use the v6.0.0 mechanism (= path of symlink).

@alexanderGugel,

That way backwards compatibility with v5.0.0 could be preserved while adding the ability to conditionally use the v6.0.0 mechanism (= path of symlink).

+1 as a possible way to address the peer dependency bug. However, we should explore other ideas as well.

Since we reverted to the old behavior and put the new behavior behind a flag is it safe to close this?

Yup, although there is still no way to have both solutions.

We still need a way get both behaviors working without a flag -- but I don't think this option was going to work (at least not on its own).

MylesBorins added the module Issues and PRs related to the module subsystem. label Apr 29, 2016

alexanderGugel force-pushed the relative-symlink branch from c784c7b to 82cce24 Compare April 29, 2016 01:30

mscdex reviewed Apr 29, 2016
View reviewed changes

alexanderGugel force-pushed the relative-symlink branch from 82cce24 to 9cf3bdd Compare April 29, 2016 02:46

mscdex reviewed Apr 29, 2016
View reviewed changes

alexanderGugel force-pushed the relative-symlink branch from 9cf3bdd to 6564aae Compare April 29, 2016 03:01

module: Change precedence of _-prefixed modules

37b7b5e

That way stat() only needs to be called once in most cases (assuming non _-prefixed symlinks are more common).

mscdex reviewed Apr 29, 2016
View reviewed changes

alexanderGugel added 2 commits April 29, 2016 04:15

module: Enable V8 performance optimizations

0ef4591

const in for-loop can not be optimized.

module: Only resolve to _-prefixed filename if needed

ca804f5

* Fixes `test-cwd-enoent-preload.js` * Minor performance improvement

mscdex reviewed Apr 29, 2016
View reviewed changes

nowtvicebergteam mentioned this pull request Apr 29, 2016

Don't resolve symlinks when requiring #3402

Closed

module: Test _-require resolution for nested modules

994f7ae

Ensure that `require('some-module/index.js')` is being resolved properly when `some-module` is a _-prefixed symlink.

alexanderGugel closed this Jun 6, 2016

+                  // If _basePath is a symlink, use the real path rather than the path of the
+                  // symlink as cache key.
+                  const _basePath = path.resolve(curPath, '_' + request);
+                  const basePath = path.resolve(curPath, request);

                 for (var i = 0; i < paths.length; i++) {
                   // Don't search further if path doesn't exist
-                  const curPath = paths[i];
+                  var curPath = paths[i];

module: resolve _-prefixed dependencies to real path by alexanderGugel · Pull Request #6460 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: resolve _-prefixed dependencies to real path - #6460

module: resolve _-prefixed dependencies to real path#6460alexanderGugel wants to merge 5 commits intonodejs:masternodejs/node:masterfrom alexanderGugel:relative-symlinkalexanderGugel/node-1:relative-symlinkCopy head branch name to clipboard

Conversation

alexanderGugel commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Example using old mechanism (pre v6.0.0):

Example using new mechanism (post v6.0.0):

Uh oh!

mscdex Apr 29, 2016

Choose a reason for hiding this comment

Uh oh!

alexanderGugel Apr 29, 2016

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Apr 29, 2016

Uh oh!

mscdex commented Apr 29, 2016

Uh oh!

mscdex commented Apr 29, 2016

Uh oh!

alexanderGugel commented Apr 29, 2016

Uh oh!

alexanderGugel commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex Apr 29, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

alexanderGugel commented Apr 29, 2016

Uh oh!

alexanderGugel commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex Apr 29, 2016

Choose a reason for hiding this comment

Uh oh!

alexanderGugel Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Apr 29, 2016

Choose a reason for hiding this comment

Uh oh!

alexanderGugel commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Apr 29, 2016

Uh oh!

alexanderGugel commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Apr 29, 2016

Uh oh!

alexanderGugel commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexanderGugel commented May 2, 2016

Uh oh!

MylesBorins commented May 2, 2016

Uh oh!

mscdex commented May 2, 2016

Uh oh!

alexanderGugel commented May 3, 2016

Uh oh!

dlongley commented May 3, 2016

Uh oh!

MylesBorins commented Jun 6, 2016

Uh oh!

alexanderGugel commented Jun 6, 2016

module: resolve _-prefixed dependencies to real path#6460
alexanderGugel wants to merge 5 commits into
nodejs:masterfrom
alexanderGugel:relative-symlink

alexanderGugel commented Apr 29, 2016 •
edited
Loading

alexanderGugel commented Apr 29, 2016 •
edited
Loading

alexanderGugel commented Apr 29, 2016 •
edited
Loading

alexanderGugel Apr 29, 2016 •
edited
Loading

alexanderGugel commented Apr 29, 2016 •
edited
Loading

alexanderGugel commented Apr 29, 2016 •
edited
Loading

alexanderGugel commented Apr 29, 2016 •
edited
Loading

mscdex commented Apr 29, 2016 •
edited
Loading