path: replace duplicate conditions by functions

[daynin]

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

A benchmark results (there are no significant changes):

 confidence improvement accuracy (*) (**) (***) path/basename-posix.js n=1000000 pathext="" 0.57 % ±0.86% ±1.15% ±1.49% path/basename-posix.js n=1000000 pathext="/" -0.43 % ±0.86% ±1.15% ±1.50% path/basename-posix.js n=1000000 pathext="/foo" 0.26 % ±0.67% ±0.89% ±1.16% path/basename-posix.js n=1000000 pathext="/foo/.bar.baz" -0.09 % ±0.28% ±0.37% ±0.48% path/basename-posix.js n=1000000 pathext="/foo/.bar.baz|.baz" -0.08 % ±0.17% ±0.23% ±0.29% path/basename-posix.js n=1000000 pathext="/foo/bar/baz/asdf/quux.html" 0.05 % ±0.21% ±0.27% ±0.36% path/basename-posix.js n=1000000 pathext="/foo/bar/baz/asdf/quux.html|.html" -0.05 % ±0.16% ±0.21% ±0.27% path/basename-posix.js n=1000000 pathext="foo" 0.10 % ±0.48% ±0.64% ±0.83% path/basename-posix.js n=1000000 pathext="foo/bar." 0.00 % ±0.30% ±0.40% ±0.52% path/basename-posix.js n=1000000 pathext="foo/bar.|." 0.01 % ±0.28% ±0.37% ±0.49% path/basename-win32.js n=1000000 pathext="" 0.09 % ±0.75% ±1.00% ±1.30% path/basename-win32.js n=1000000 pathext="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html" -0.12 % ±0.20% ±0.26% ±0.34% path/basename-win32.js n=1000000 pathext="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html|.html" 0.03 % ±0.13% ±0.18% ±0.23% path/basename-win32.js n=1000000 pathext="C:\\\\" 0.03 % ±0.42% ±0.56% ±0.73% path/basename-win32.js n=1000000 pathext="C:\\\\foo" -0.03 % ±0.40% ±0.53% ±0.70% path/basename-win32.js n=1000000 pathext="D:\\\\foo\\\\.bar.baz" -0.06 % ±0.19% ±0.26% ±0.33% path/basename-win32.js n=1000000 pathext="E:\\\\foo\\\\.bar.baz|.baz" * 0.17 % ±0.15% ±0.20% ±0.26% path/basename-win32.js n=1000000 pathext="foo" -0.26 % ±0.63% ±0.84% ±1.11% path/basename-win32.js n=1000000 pathext="foo\\\\bar." -0.22 % ±0.24% ±0.32% ±0.42% path/basename-win32.js n=1000000 pathext="foo\\\\bar.|." -0.06 % ±0.23% ±0.31% ±0.40% path/dirname-posix.js n=1000000 path="" 0.73 % ±1.60% ±2.14% ±2.79% path/dirname-posix.js n=1000000 path="/" -0.18 % ±0.88% ±1.17% ±1.53% path/dirname-posix.js n=1000000 path="/foo" 0.23 % ±0.61% ±0.81% ±1.05% path/dirname-posix.js n=1000000 path="/foo/bar" 0.25 % ±0.73% ±0.97% ±1.26% path/dirname-posix.js n=1000000 path="/foo/bar/baz/asdf/quux" -0.13 % ±0.35% ±0.47% ±0.61% path/dirname-posix.js n=1000000 path="foo" -0.10 % ±0.85% ±1.14% ±1.48% path/dirname-posix.js n=1000000 path="foo/bar" -0.13 % ±0.80% ±1.06% ±1.39% path/dirname-win32.js n=1000000 path="" 0.03 % ±0.82% ±1.09% ±1.41% path/dirname-win32.js n=1000000 path="\\\\" 0.37 % ±0.70% ±0.93% ±1.21% path/dirname-win32.js n=1000000 path="\\\\foo" -0.39 % ±0.51% ±0.68% ±0.89% path/dirname-win32.js n=1000000 path="C:\\\\foo\\\\bar" -0.33 % ±0.50% ±0.67% ±0.88% path/dirname-win32.js n=1000000 path="D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux" -0.05 % ±0.22% ±0.29% ±0.38% path/dirname-win32.js n=1000000 path="foo" -0.47 % ±0.49% ±0.65% ±0.85% path/dirname-win32.js n=1000000 path="foo\\\\bar" 0.08 % ±0.34% ±0.46% ±0.60% path/extname-posix.js n=1000000 path="" 0.05 % ±0.79% ±1.05% ±1.37% path/extname-posix.js n=1000000 path="/" 0.10 % ±0.75% ±1.00% ±1.30% path/extname-posix.js n=1000000 path="/foo" -0.11 % ±0.32% ±0.43% ±0.56% path/extname-posix.js n=1000000 path="/foo/bar/baz/asdf/quux.foobarbazasdfquux" -0.05 % ±0.11% ±0.14% ±0.19% path/extname-posix.js n=1000000 path="/foo/bar/baz/asdf/quux" -0.07 % ±0.37% ±0.49% ±0.64% path/extname-posix.js n=1000000 path="foo/.bar.baz" -0.16 % ±0.21% ±0.27% ±0.36% path/extname-posix.js n=1000000 path="foo/bar/...baz.quux" 0.09 % ±0.16% ±0.22% ±0.29% path/extname-posix.js n=1000000 path="foo/bar/..baz.quux" 0.11 % ±0.13% ±0.17% ±0.22% path/extname-posix.js n=1000000 path="index.html" -0.25 % ±0.52% ±0.70% ±0.92% path/extname-posix.js n=1000000 path="index" 0.11 % ±0.24% ±0.32% ±0.41% path/extname-win32.js n=1000000 path="" -0.50 % ±1.06% ±1.41% ±1.84% path/extname-win32.js n=1000000 path="\\\\" * -1.08 % ±0.83% ±1.10% ±1.44% path/extname-win32.js n=1000000 path="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.foobarbazasdfquux" -0.04 % ±0.22% ±0.29% ±0.38% path/extname-win32.js n=1000000 path="C:\\\\foo" -0.33 % ±0.41% ±0.54% ±0.71% path/extname-win32.js n=1000000 path="D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux" -0.20 % ±0.81% ±1.08% ±1.41% path/extname-win32.js n=1000000 path="foo\\\\.bar.baz" 0.08 % ±0.30% ±0.39% ±0.51% path/extname-win32.js n=1000000 path="foo\\\\bar\\\\...baz.quux" * -0.21 % ±0.20% ±0.27% ±0.35% path/extname-win32.js n=1000000 path="foo\\\\bar\\\\..baz.quux" 0.03 % ±0.34% ±0.45% ±0.58% path/extname-win32.js n=1000000 path="index.html" * 0.27 % ±0.22% ±0.30% ±0.39% path/extname-win32.js n=1000000 path="index" 0.20 % ±0.56% ±0.75% ±0.97% path/format-posix.js n=10000000 props="/|/home/user/dir|index.html|.html|index" 0.18 % ±0.28% ±0.38% ±0.49% path/format-win32.js n=10000000 props="C:\\\\|C:\\\\path\\\\dir|index.html|.html|index" -0.02 % ±0.28% ±0.37% ±0.48% path/isAbsolute-posix.js n=1000000 path="." -0.41 % ±1.15% ±1.53% ±2.00% path/isAbsolute-posix.js n=1000000 path="" 0.18 % ±1.21% ±1.62% ±2.11% path/isAbsolute-posix.js n=1000000 path="/baz/.." -0.95 % ±1.18% ±1.57% ±2.05% path/isAbsolute-posix.js n=1000000 path="/foo/bar" -0.74 % ±1.01% ±1.34% ±1.75% path/isAbsolute-posix.js n=1000000 path="bar/baz" 0.30 % ±1.27% ±1.69% ±2.20% path/isAbsolute-win32.js n=1000000 path="." -0.25 % ±0.94% ±1.25% ±1.63% path/isAbsolute-win32.js n=1000000 path="" 0.42 % ±1.35% ±1.79% ±2.34% path/isAbsolute-win32.js n=1000000 path="//server" 0.00 % ±1.23% ±1.64% ±2.13% path/isAbsolute-win32.js n=1000000 path="bar\\\\baz" -0.06 % ±0.63% ±0.84% ±1.10% path/isAbsolute-win32.js n=1000000 path="C:\\\\baz\\\\.." -0.37 % ±0.58% ±0.78% ±1.02% path/isAbsolute-win32.js n=1000000 path="C:baz\\\\.." 0.24 % ±0.78% ±1.04% ±1.37% path/join-posix.js n=1000000 paths="/foo|bar||baz/asdf|quux|.." 0.10 % ±0.15% ±0.20% ±0.26% path/join-win32.js n=1000000 paths="C:\\\\foo|bar||baz\\\\asdf|quux|.." -0.13 % ±0.18% ±0.23% ±0.31% path/makeLong-win32.js n=1000000 path="\\\\\\\\?\\\\foo" -0.15 % ±0.57% ±0.76% ±0.99% path/makeLong-win32.js n=1000000 path="\\\\\\\\foo\\\\bar" -0.21 % ±0.58% ±0.78% ±1.01% path/makeLong-win32.js n=1000000 path="C:\\\\foo" -0.24 % ±0.48% ±0.64% ±0.84% path/makeLong-win32.js n=1000000 path="foo\\\\bar" -0.32 % ±1.36% ±1.82% ±2.37% path/normalize-posix.js n=1000000 path="." 0.27 % ±0.43% ±0.58% ±0.76% path/normalize-posix.js n=1000000 path="" -0.08 % ±0.67% ±0.90% ±1.17% path/normalize-posix.js n=1000000 path="/../" -0.24 % ±0.67% ±0.89% ±1.17% path/normalize-posix.js n=1000000 path="/foo" 0.07 % ±0.46% ±0.61% ±0.79% path/normalize-posix.js n=1000000 path="/foo/bar" -0.07 % ±0.28% ±0.37% ±0.48% path/normalize-posix.js n=1000000 path="/foo/bar//baz/asdf/quux/.." 0.27 % ±0.52% ±0.69% ±0.92% path/normalize-win32.js n=1000000 path="." 0.25 % ±0.35% ±0.47% ±0.61% path/normalize-win32.js n=1000000 path="" 0.65 % ±0.77% ±1.03% ±1.34% path/normalize-win32.js n=1000000 path="C:\\\\..\\\\" 0.06 % ±0.40% ±0.54% ±0.70% path/normalize-win32.js n=1000000 path="C:\\\\foo" 0.09 % ±0.44% ±0.58% ±0.76% path/normalize-win32.js n=1000000 path="C:\\\\foo\\\\bar" -0.26 % ±0.44% ±0.59% ±0.76% path/normalize-win32.js n=1000000 path="C:\\\\foo\\\\bar\\\\\\\\baz\\\\asdf\\\\quux\\\\.." 0.08 % ±0.25% ±0.34% ±0.44% path/parse-posix.js n=1000000 path="" * -0.99 % ±0.96% ±1.28% ±1.67% path/parse-posix.js n=1000000 path="/" 1.01 % ±3.71% ±4.94% ±6.43% path/parse-posix.js n=1000000 path="/foo" -0.01 % ±0.39% ±0.52% ±0.67% path/parse-posix.js n=1000000 path="/foo/bar.baz" 0.02 % ±0.38% ±0.51% ±0.66% path/parse-posix.js n=1000000 path="/foo/bar/baz/asdf/.quux" -0.00 % ±0.19% ±0.25% ±0.33% path/parse-posix.js n=1000000 path="foo/.bar.baz" -0.12 % ±0.37% ±0.49% ±0.64% path/parse-posix.js n=1000000 path="foo/bar" 0.14 % ±0.39% ±0.53% ±0.68% path/parse-win32.js n=1000000 path="" -0.39 % ±0.82% ±1.09% ±1.41% path/parse-win32.js n=1000000 path="\\\\foo" 0.10 % ±0.42% ±0.56% ±0.73% path/parse-win32.js n=1000000 path="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\.quux" -0.09 % ±0.21% ±0.28% ±0.37% path/parse-win32.js n=1000000 path="C:\\\\" 0.40 % ±0.81% ±1.07% ±1.40% path/parse-win32.js n=1000000 path="C:\\\\foo" 0.11 % ±0.64% ±0.85% ±1.10% path/parse-win32.js n=1000000 path="E:\\\\foo\\\\bar.baz" -0.13 % ±0.17% ±0.23% ±0.30% path/parse-win32.js n=1000000 path="foo\\\\.bar.baz" 0.21 % ±0.33% ±0.45% ±0.58% path/parse-win32.js n=1000000 path="foo\\\\bar" -0.03 % ±0.45% ±0.59% ±0.78% path/relative-posix.js n=1000000 paths="/|/" -0.45 % ±0.95% ±1.26% ±1.64% path/relative-posix.js n=1000000 paths="/|/var" -0.04 % ±0.30% ±0.40% ±0.52% path/relative-posix.js n=1000000 paths="/data/orandea/test/aaa|/data/orandea/impl/bbb" 0.14 % ±0.24% ±0.32% ±0.42% path/relative-posix.js n=1000000 paths="/foo/bar/baz/quux|/" -0.07 % ±0.10% ±0.13% ±0.17% path/relative-posix.js n=1000000 paths="/foo/bar/baz/quux|/foo/bar/baz/quux" 0.52 % ±0.98% ±1.30% ±1.69% path/relative-posix.js n=1000000 paths="/foo/bar/baz/quux|/var/log" 0.05 % ±0.29% ±0.38% ±0.50% path/relative-posix.js n=1000000 paths="/var|/bin" 0.01 % ±0.40% ±0.53% ±0.69% path/relative-win32.js n=1000000 paths="C:\\\\|D:\\\\" -0.09 % ±0.18% ±0.24% ±0.32% path/relative-win32.js n=1000000 paths="C:\\\\foo\\\\bar\\\\baz\\\\quux|C:\\\\" 0.15 % ±0.26% ±0.34% ±0.45% path/relative-win32.js n=1000000 paths="C:\\\\foo\\\\bar\\\\baz|C:\\\\foo\\\\bar\\\\baz" -0.17 % ±0.39% ±0.51% ±0.67% path/relative-win32.js n=1000000 paths="C:\\\\foo\\\\BAR\\\\BAZ|C:\\\\foo\\\\bar\\\\baz" 0.13 % ±0.59% ±0.79% ±1.03% path/relative-win32.js n=1000000 paths="C:\\\\orandea\\\\test\\\\aaa|C:\\\\orandea\\\\impl\\\\bbb" -1.00 % ±1.85% ±2.49% ±3.30% path/resolve-posix.js n=1000000 paths="" 0.19 % ±1.66% ±2.22% ±2.88% path/resolve-posix.js n=1000000 paths="|" -0.17 % ±2.09% ±2.79% ±3.66% path/resolve-posix.js n=1000000 paths="a/b/c/|../../.." -0.63 % ±1.50% ±2.00% ±2.60% path/resolve-posix.js n=1000000 paths="foo/bar|/tmp/file/|..|a/../subfile" -0.20 % ±0.86% ±1.15% ±1.50% path/resolve-win32.js n=1000000 paths="" 0.20 % ±1.53% ±2.04% ±2.65% path/resolve-win32.js n=1000000 paths="|" 0.54 % ±1.35% ±1.80% ±2.34% path/resolve-win32.js n=1000000 paths="c:/blah\\\\blah|d:/games|c:../a" -0.01 % ±0.41% ±0.55% ±0.71% path/resolve-win32.js n=1000000 paths="c:/ignore|d:\\\\a/b\\\\c/d|\\\\e.exe" -0.07 % ±0.59% ±0.79% ±1.03% 

[bnoordhuis]

Nice, LGTM. The change in performance doesn't look statistically significant.

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

[daynin]

I resolved a conflict here

[apapirovski]

This is nice! Just some nits below.

[apapirovski]

Nit: this would be slightly nicer aligned with the code part above, rather than just indented two spaces.

[apapirovski]

Unnecessary parentheses here.

[BridgeAR]

I would also inline path.charCodeAt(len - 1).

[apapirovski]

This is unnecessary. This could all be just one if statement. path.length >= 2 && path.charCodeAt(1) === CHAR_COLON && isWindowsDeviceRoot(path.charCodeAt(0))

[BridgeAR]

LGTM. I would just go ahead and inline lots of charCodeAt calls.

[BridgeAR]

I would also inline path.charCodeAt(len - 1).

[BridgeAR]

It should be fine to inline path.charCodeAt(j).

[BridgeAR]

It should be fine to inline path.charCodeAt(1).

[BridgeAR]

It should be fine to inline path.charCodeAt(j)

[BridgeAR]

It should be fine to inline pathCharCodeAt(j)

[BridgeAR]

It should be fine to inline path.charCodeAt(2)

[BridgeAR]

Ok, this applies to a lot of places here ;-)

[daynin]

@BridgeAR there are too many places like this. I think it will be better to fix it in a another pull request. What do you think?

[BridgeAR]

Hm, I personally think this would actually be the right place to do so. The reason is that it touches the same lines. @apapirovski what do you think?

[apapirovski]

code seems to mostly be there to avoid constantly calling path.charCodeAt(x) within the same conditional but now that these are a function, it seems like their purpose is gone. I would say that this could definitely be done in this PR even though it adds a bunch of work.

[daynin]

@BridgeAR ok, I was just worried that it might affect some logic here. Nonetheless, I'll fix it

[apapirovski]

@daynin it'll probably take some effort to validate all the places where it's safe to do so. Would suggest doing it as a separate commit. Should make reviews easier.

[daynin]

@apaprocki ok, sure

[BridgeAR]

It should be relatively straight forward. Try to remove them from the most inner function and go to the outer one. I checked it for one big function and as far as I saw it was possible to remove all of them besides a single one that should switch to be const.

[daynin]

@apapirovski@BridgeAR fixed

[apapirovski]

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

[BridgeAR]

Here in this case it would be better to keep the code above as it will otherwise require two lookups for this branch.

[BridgeAR]

Here it would be better to keep code because this branch otherwise has to do a second lookup.

[BridgeAR]

Here it would be better to keep the code because of a second lookup.

[BridgeAR]

Please code here to prevent a second lookup.

[BridgeAR]

Please keep code here.

[BridgeAR]

Please keep code here.

[daynin]

@BridgeAR fixed

[BridgeAR]

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

[BridgeAR]

Landed in b404aa5 🎉