fs: optimize realpath using uv_fs_realpath()

[jhamhader]

If cache is not used, use the native uv_fs_realpath() which is faster
then the JS implementation by a few orders of magnitude.

See #2680

[bnoordhuis]

Cross-referencing libuv/libuv#531 which is still open and has some unresolved issues.

I'm not sure if this is a zero risk change. fs.realpathSync() is used by require(). If there are any discrepancies between the current implementation and realpath(3), that can cause regressions in the module loader.

[Fishrock123]

I'm not sure if this is a zero risk change. fs.realpathSync() is used by require(). If there are any discrepancies between the current implementation and realpath(3), that can cause regressions in the module loader.

cc @bmeck who may have some insight

[bmeck]

I would say this is probably safe on *nix, but not Windows for now

• on *nix PATH_MAX will become the maximum string size, only important for
  virtual file systems
• unsure if it will muck with filenames on windows due to the
  case-insensitive filesystem
• fs: add uv_fs_realpath() libuv/libuv#531 needs to fix Windows XP to land
  still

On Fri, Oct 30, 2015 at 9:18 AM, Jeremiah Senkpiel <notifications@github.com

wrote:

I'm not sure if this is a zero risk change. fs.realpathSync() is used by
require(). If there are any discrepancies between the current
implementation and realpath(3), that can cause regressions in the module
loader.

cc @bmeck https://github.com/bmeck who may have some insight

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

[jhamhader]

Updated realpath() and realpathSync() to use JS implementation when using Windows XP.

[Fishrock123]

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

[jhamhader]

Well, this depends on libuv/libuv#531

[saghul]

The libuv bits landed. I suggest this one is postponed until Windows XP is dropped so Node doesn't have to care about uv_fs_realpath not working on that platform.

[ChALkeR]

@saghul It seems to me that 5.x is branched, and XP support will be dropped in 6.0 (current master): #3804. That given, I see no obstacles for relying on uv_fs_realpath in master.

[ChALkeR]

@saghul Ah, on the other hand, perhaps XP support being dropped should be documented first.

[saghul]

@ChALkeR yeah, I was thinking about 6.0. It can be documented as a feature! "Removed baggage, aka Windows XP" :-P

On a more serious note, master has libuv 1.8.0 now, so is probably a good time to rebase and give it a go on the CI? @jhamhader

[jhamhader]

The current code sends Windows XP to the original JS implementation of realpath().
Should the PR rely on the assumption that upon its merge, Windows XP will be dropped? Or are we to handle Windows XP?

[saghul]

@jhamhader my undestanding is that Node 6.0 would drop XP (see #3804) so IMHO you can rely on that.

[jhamhader]

Ready for CI

[saghul]

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

[jhamhader]

Fixing the tests to accommodate with Windows case insensitive paths.

[jhamhader]

Fixed

[saghul]

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

[jhamhader]

Fixed linter errors (my bad! :\ )

Any idea why Windows 10 fails with on test-debug-no-context.js:

  ...
not ok 40 test-debug-no-context.js
# 
# assert.js:89
#   throw new assert.AssertionError({
#   ^
# AssertionError: 3221225477 === 0
#     at ChildProcess.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015\label\win10\test\parallel\test-debug-no-context.js:15:10)
#     at ChildProcess.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015\label\win10\test\common.js:401:15)
#     at emitTwo (events.js:88:13)
#     at ChildProcess.emit (events.js:173:7)
#     at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)

[saghul]

No idea, sorry.

There is one thing that worries me a bit: we are only using the "fast" variant when the cache is not used, which means that we have to keep 2 implementations which basically do the same, right? Was the cache added for speed? If so, would it be ok to drop it now? (that would be semver major, I guess)

Thoughts @nodejs/collaborators ?

[mscdex]

3221225477 means an access violation/segfault occurred. I saw that test failure the other week but haven't been seeing it lately (and I haven't seen it happen locally in my Windows VM). I haven't looked into the cause though.

[trevnorris]

@saghul honestly the call to uv_fs_realpath() is probably going to be faster that going through the complicated cache mechanism. we should do some benchmarking, and if it's not (as I suspect) then let's just dump it.

EDIT: Would it not work to simply always make the call to node::RealPath() immediately?

[isaacs]

One concern: if node's fs.realpath will start throwing on systems that lack PATH_MAX, then now we have to handle that, which is pretty inconvenient.

How bad would it be to fall back to the JS implementation if libuv can't do realpath safely?

[isaacs]

Re the issue in node-glob, I consider throwing at different levels to be no big deal, and will adjust the tests accordingly. Bottom line, it'll eventually raise an error if it's following symlinks through a loop, which is the point.

The important thing is that node's fs.realpath is safe, predictable, and fast. Imo, "safe, predictable, fast" is the right priority order. Removing realpath from systems that used to have it is pretty disruptive, and reduces predictability. Having a buffer overflow vuln on those systems is unsafe, so even worse.

[addaleax]

@isaacs libuv/libuv#843 makes libuv not even build on systems without PATH_MAX, fwiw. And no such platform has been found so far.

[MylesBorins]

it is worth mentioning that @bnoordhuis brought up some concerns, especially regarding freeBSD not having to have PATH_MAX defined

There is a possibility that the commit could be reverted.

[Fishrock123]

I would prefer to attempt to work around edge cases rather than revert outright. The speed improvement here is actually really important to some users. (iirc that originally brought up this possibility!)

[trevnorris]

@Fishrock123 You're correct that performance was how this whole thing came about. Here's the original issue posted by @joliss about issue with the Broccoli build tool because of the excessive slowness: nodejs/node-v0.x-archive#7902 (original post where I first suggested using realpath(3) where supported nodejs/node-v0.x-archive#7902 (comment)).

The new performance gains are much better, but still not where they could be. Haven't had time to take an in depth look, but definitely agree with @Fishrock123 that we should take a look at fixing the issue instead of doing a full revert because of them.

[jasnell]

+1... While reverting should be an option on the table if it proves
necessary, we should seek to find an alternative before doing so.
On Apr 27, 2016 4:14 PM, "Trevor Norris" notifications@github.com wrote:

@Fishrock123 https://github.com/Fishrock123 You're correct that
performance was how this whole thing came about. Here's the original issue
posted by @joliss https://github.com/joliss about issue with the
Broccoli build tool because of the excessive slowness:
nodejs/node-v0.x-archive#7902
nodejs/node-v0.x-archive#7902 (original post
where I first suggested using realpath(3) where supported nodejs/node-v0.x-archive#7902
(comment)
nodejs/node-v0.x-archive#7902 (comment)).
At the bottom is the link to the PR, and its 7 month journey, to finally
landing.

The new performance gains are much better, but still not where they could
be. Haven't had time to take an in depth look, but definitely agree with
@Fishrock123 https://github.com/Fishrock123 that we should take a look
at fixing the issue instead of doing a full revert because of them.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3594 (comment)