path: fix win32 volume-relative paths

[TimothyGu]

path.resolve() and path.join() are left alone in this commit due to
a lack of clear semantics.

Fixes: #14405

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

path

[tniessen]

Thanks for working on this!

[tniessen]

I don't think this is a valid Windows path, is it?

[refack]

path does not do validation, but it's not:

[tniessen]

I don't mean we should validate it, I just don't think we should test against invalid paths.

[TimothyGu]

I don't see any harm in having it here. In fact we should keep it just for compatibility until some future fix that needs to remove it.

[tniessen]

Could you add tests for NTFS alterate streams? e.g. ['cd:foo', '']. Not just to test root, but also the other properties used in checkParseFormat.

[tniessen]

Again, a test for NTFS alternate streams would be nice: basename('cd:foo') === 'cd:foo'

[refack]

path does not do validation, but it's not:

[refack]

Optional:

Could you parameterize this part like the others are:

[[a,b]].forEach(([tested, expected]) => ...

(not the scope of this PR but still would be nice)

[TimothyGu]

I'd rather leave that to a dedicated PR, as there are many other tests that do not use this pattern in the file.

[refack]

could you use startsWith, so it won't pop up when grepping for indexOf

[TimothyGu]

I don't quite know the reason for grepping indexOf, but changed nevertheless.

[refack]

@TimothyGu could you run performance comparison, just for reference?
(minimal set could be with --filter parse path i.e. node benchmark/compare.js --old .\nodefwithout.exe --new Release\nodewith.exe --filter parse path > path.csv)

[refack]

P.S. thanks for doing this 👍! Playing with path is not exactly a fun time...

[TimothyGu]

@refack For good measure:

$ node benchmark/compare.js --old ./node-master --new ./node-14440 --runs 10 --filter parse-win32 path > path-win32.csv
$ Rscript benchmark/compare.R < path-win32.csv 
                                                                             improvement confidence      p.value
 path/parse-win32.js n=1000000 path=""                                           -1.44 %            1.527455e-01
 path/parse-win32.js n=1000000 path="\\\\foo"                                    29.10 %        *** 4.505598e-07
 path/parse-win32.js n=1000000 path="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\.quux"      3.27 %        *** 7.782213e-11
 path/parse-win32.js n=1000000 path="C:\\\\"                                      0.36 %            2.234293e-01
 path/parse-win32.js n=1000000 path="C:\\\\foo"                                  33.79 %        *** 5.225296e-12
 path/parse-win32.js n=1000000 path="E:\\\\foo\\\\bar.baz"                        0.09 %            7.339049e-01
 path/parse-win32.js n=1000000 path="foo\\\\.bar.baz"                             1.26 %        *** 3.623360e-06
 path/parse-win32.js n=1000000 path="foo\\\\bar"                                 -0.14 %            6.443696e-01

[refack]

Thank you

[TimothyGu]

However:

                                                                                                improvement confidence      p.value
 path/extname-win32.js n=1000000 path=""                                                           -74.64 %        *** 1.674087e-26
 path/extname-win32.js n=1000000 path="\\\\"                                                        -4.91 %        *** 1.755024e-07
 path/extname-win32.js n=1000000 path="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.foobarbazasdfquux"     -7.25 %        *** 1.823382e-05
 path/extname-win32.js n=1000000 path="C:\\\\foo"                                                  -21.36 %        *** 5.547522e-18
 path/extname-win32.js n=1000000 path="D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux"                    -23.40 %        *** 3.036449e-06
 path/extname-win32.js n=1000000 path="foo\\\\.bar.baz"                                             -7.85 %        *** 1.491586e-14
 path/extname-win32.js n=1000000 path="foo\\\\bar\\\\...baz.quux"                                  -12.20 %        *** 4.673165e-05
 path/extname-win32.js n=1000000 path="foo\\\\bar\\\\..baz.quux"                                   -11.68 %        *** 1.828409e-13
 path/extname-win32.js n=1000000 path="index.html"                                                  -8.17 %        *** 1.836566e-09
 path/extname-win32.js n=1000000 path="index"                                                       -7.79 %          * 2.396233e-02

There isn't anything I can do to improve the worst case (path=""). On the other hand, two factors alleviate the drastic change:

• The same piece of code is in basename() already.
• V8 5.9 used in master is known to not handle charCodeAt() well, and Node.js canary which has V8 6.1 restores the original performance for that benchmark while making all other benchmarks faster as well.

[refack]

There isn't anything I can do to improve the worst case (path=""). On the other hand, two factors alleviate the drastic change:

Since this is a bug fix IMHO we can absorb the performance degradation for this case. Especially since the most common case doesn't take a bit hit (and the absolute numbers are probably tiny anyway):

 path/extname-win32.js n=1000000 path="index.html"              -8.17 %        *** 1.836566e-09

[addaleax]

@TimothyGu This would need to be rebased ;)

[tniessen]

@TimothyGu My bad, sorry, I landed f7f590c this morning. You can safely drop my changes to winPaths.

[TimothyGu]

@addaleax @tniessen Rebased. I compared and cherry-picked some changes in f7f590c not already in test-path.js, so this should benefit other path.* functions as well.

[tniessen]

LGTM, no opinion on performance. Thanks for fixing this :)

[TimothyGu]

Landed in 2791761.

[MylesBorins]

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

[TimothyGu]

@MylesBorins Done: #14787