Skip to content

path: support file urls - #52672

Closed
avivkeller wants to merge 4 commits into
nodejs:mainfrom
avivkeller:support-path-url
Closed

path: support file urls#52672
avivkeller wants to merge 4 commits into
nodejs:mainfrom
avivkeller:support-path-url

Conversation

@avivkeller

Copy link
Copy Markdown
Member

This PR changes the path functions to support File URLs.

Related to #41521
Related to #49273
Successor of #52497

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Apr 24, 2024
@avivkelleravivkeller added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 24, 2024
@aduh95aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 24, 2024
@github-actionsgithub-actionsBot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 24, 2024
@github-actions

Copy link
Copy Markdown
Contributor
Failed to start CI
 ⚠ No approving reviews found
✘ Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/8823064389

@avivkeller

avivkeller commented Apr 24, 2024

Copy link
Copy Markdown
MemberAuthor

That's a good sign (for security) [i think]

Do CIs now need approving reviews? 🎉

@targos

Copy link
Copy Markdown
Member

They don't strictly need it. Collaborators can still start CI jobs from Jenkins.

@aduh95aduh95 removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 24, 2024

@LiviaMedeirosLiviaMedeiros left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with or without the following nits.

Comment threadlib/internal/url.js Outdated
Comment threaddoc/api/path.md Outdated

@anonriganonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a benchmark result before merging a PR like this that effects every single function in Node.js

Comment threadlib/path.js Outdated
Comment threadlib/path.js Outdated
Comment threadlib/path.js Outdated
@anonriganonrig added the needs-benchmark-ci PR that need a benchmark CI run. label Apr 27, 2024
@LiviaMedeiros

LiviaMedeiros commented Apr 27, 2024

Copy link
Copy Markdown
Member

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1534/

Results
00:16:45 confidence improvement accuracy (*) (**) (***)
00:16:45 path/basename-posix.js n=100000 pathext='' *** -19.60 % ±0.67% ±0.89% ±1.14%
00:16:45 path/basename-posix.js n=100000 pathext='/' *** -18.94 % ±0.76% ±1.00% ±1.28%
00:16:45 path/basename-posix.js n=100000 pathext='/foo' *** -14.02 % ±1.07% ±1.42% ±1.83%
00:16:45 path/basename-posix.js n=100000 pathext='/foo/.bar.baz' *** -5.28 % ±1.29% ±1.70% ±2.18%
00:16:45 path/basename-posix.js n=100000 pathext='/foo/.bar.baz|.baz' *** -11.79 % ±1.59% ±2.10% ±2.70%
00:16:45 path/basename-posix.js n=100000 pathext='/foo/bar/baz/asdf/quux.html' *** -7.71 % ±1.31% ±1.73% ±2.23%
00:16:45 path/basename-posix.js n=100000 pathext='/foo/bar/baz/asdf/quux.html|.html' *** -11.62 % ±1.37% ±1.81% ±2.33%
00:16:45 path/basename-posix.js n=100000 pathext='foo' *** -19.75 % ±0.65% ±0.86% ±1.10%
00:16:45 path/basename-posix.js n=100000 pathext='foo/bar.' *** -13.49 % ±0.85% ±1.12% ±1.44%
00:16:45 path/basename-posix.js n=100000 pathext='foo/bar.|.' *** -8.65 % ±1.16% ±1.54% ±1.97%
00:16:45 path/basename-win32.js n=100000 pathext='' *** -18.07 % ±0.91% ±1.20% ±1.54%
00:16:45 path/basename-win32.js n=100000 pathext='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html' *** -13.61 % ±1.06% ±1.40% ±1.79%
00:16:45 path/basename-win32.js n=100000 pathext='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html|.html' -0.32 % ±1.15% ±1.52% ±1.96%
00:16:45 path/basename-win32.js n=100000 pathext='C:\\\\' *** -17.50 % ±1.13% ±1.49% ±1.91%
00:16:45 path/basename-win32.js n=100000 pathext='C:\\\\foo' *** -10.95 % ±0.85% ±1.12% ±1.44%
00:16:45 path/basename-win32.js n=100000 pathext='D:\\\\foo\\\\.bar.baz' *** -15.08 % ±0.85% ±1.13% ±1.45%
00:16:45 path/basename-win32.js n=100000 pathext='E:\\\\foo\\\\.bar.baz|.baz' *** -8.39 % ±0.84% ±1.11% ±1.43%
00:16:45 path/basename-win32.js n=100000 pathext='foo' *** -17.80 % ±0.57% ±0.75% ±0.97%
00:16:45 path/basename-win32.js n=100000 pathext='foo\\\\bar.' *** -13.31 % ±0.79% ±1.04% ±1.34%
00:16:45 path/basename-win32.js n=100000 pathext='foo\\\\bar.|.' *** -6.67 % ±0.97% ±1.28% ±1.65%
00:16:45 path/dirname-posix.js n=100000 path='' *** -21.31 % ±1.19% ±1.58% ±2.03%
00:16:45 path/dirname-posix.js n=100000 path='/' *** -18.26 % ±0.71% ±0.94% ±1.21%
00:16:45 path/dirname-posix.js n=100000 path='/foo' *** -16.69 % ±0.51% ±0.68% ±0.87%
00:16:45 path/dirname-posix.js n=100000 path='/foo/bar' *** -13.29 % ±0.61% ±0.80% ±1.03%
00:16:45 path/dirname-posix.js n=100000 path='/foo/bar/baz/asdf/quux' *** -11.64 % ±0.98% ±1.30% ±1.67%
00:16:45 path/dirname-posix.js n=100000 path='foo' *** -12.39 % ±0.76% ±1.00% ±1.28%
00:16:45 path/dirname-posix.js n=100000 path='foo/bar' *** -18.93 % ±0.38% ±0.51% ±0.65%
00:16:45 path/dirname-win32.js n=100000 path='' *** -15.76 % ±0.98% ±1.29% ±1.65%
00:16:45 path/dirname-win32.js n=100000 path='\\\\' *** -14.66 % ±0.96% ±1.27% ±1.63%
00:16:45 path/dirname-win32.js n=100000 path='\\\\foo' *** -16.40 % ±0.67% ±0.89% ±1.14%
00:16:45 path/dirname-win32.js n=100000 path='C:\\\\foo\\\\bar' *** -11.84 % ±0.85% ±1.12% ±1.43%
00:16:45 path/dirname-win32.js n=100000 path='D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux' *** -7.00 % ±1.08% ±1.43% ±1.85%
00:16:45 path/dirname-win32.js n=100000 path='foo' *** -14.32 % ±0.93% ±1.23% ±1.59%
00:16:45 path/dirname-win32.js n=100000 path='foo\\\\bar' *** -13.92 % ±0.39% ±0.52% ±0.66%
00:16:45 path/extname-posix.js n=100000 path='' *** -26.90 % ±1.79% ±2.36% ±3.05%
00:16:45 path/extname-posix.js n=100000 path='/' *** -24.56 % ±1.12% ±1.48% ±1.90%
00:16:45 path/extname-posix.js n=100000 path='/foo' *** -13.80 % ±0.90% ±1.19% ±1.53%
00:16:45 path/extname-posix.js n=100000 path='/foo/bar/baz/asdf/quux.foobarbazasdfquux' *** -9.01 % ±1.24% ±1.63% ±2.10%
00:16:45 path/extname-posix.js n=100000 path='/foo/bar/baz/asdf/quux' *** -6.55 % ±0.97% ±1.28% ±1.64%
00:16:45 path/extname-posix.js n=100000 path='foo/.bar.baz' *** -6.49 % ±1.16% ±1.53% ±1.97%
00:16:45 path/extname-posix.js n=100000 path='foo/bar/...baz.quux' *** -4.33 % ±1.57% ±2.07% ±2.66%
00:16:45 path/extname-posix.js n=100000 path='foo/bar/..baz.quux' *** -4.49 % ±0.90% ±1.19% ±1.53%
00:16:45 path/extname-posix.js n=100000 path='index.html' *** -6.88 % ±1.44% ±1.90% ±2.45%
00:16:45 path/extname-posix.js n=100000 path='index' *** -16.85 % ±0.70% ±0.93% ±1.19%
00:16:45 path/extname-win32.js n=100000 path='' *** -20.19 % ±1.03% ±1.36% ±1.75%
00:16:45 path/extname-win32.js n=100000 path='\\\\' *** -19.16 % ±1.13% ±1.49% ±1.92%
00:16:45 path/extname-win32.js n=100000 path='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.foobarbazasdfquux' *** -6.78 % ±1.58% ±2.08% ±2.68%
00:16:45 path/extname-win32.js n=100000 path='C:\\\\foo' *** -12.76 % ±0.91% ±1.20% ±1.55%
00:16:45 path/extname-win32.js n=100000 path='D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux' *** -9.55 % ±1.16% ±1.54% ±1.98%
00:16:45 path/extname-win32.js n=100000 path='foo\\\\.bar.baz' *** -8.78 % ±0.82% ±1.08% ±1.38%
00:16:45 path/extname-win32.js n=100000 path='foo\\\\bar\\\\...baz.quux' *** -6.27 % ±0.77% ±1.02% ±1.32%
00:16:45 path/extname-win32.js n=100000 path='foo\\\\bar\\\\..baz.quux' *** -6.64 % ±1.05% ±1.39% ±1.79%
00:16:45 path/extname-win32.js n=100000 path='index.html' *** -9.13 % ±1.00% ±1.32% ±1.70%
00:16:45 path/extname-win32.js n=100000 path='index' *** -11.56 % ±0.97% ±1.28% ±1.65%
00:16:45 path/format-posix.js n=1000000 props='/|/home/user/dir|index.html|.html|index' 0.24 % ±0.41% ±0.54% ±0.70%
00:16:45 path/format-win32.js n=1000000 props='C:\\\\|C:\\\\path\\\\dir|index.html|.html|index' -0.17 % ±0.44% ±0.58% ±0.74%
00:16:45 path/isAbsolute-posix.js n=100000 path='.' *** -23.85 % ±0.61% ±0.81% ±1.04%
00:16:45 path/isAbsolute-posix.js n=100000 path='' *** -28.10 % ±1.22% ±1.62% ±2.08%
00:16:45 path/isAbsolute-posix.js n=100000 path='/baz/..' *** -23.75 % ±0.43% ±0.57% ±0.73%
00:16:45 path/isAbsolute-posix.js n=100000 path='/foo/bar' *** -24.11 % ±0.63% ±0.84% ±1.07%
00:16:45 path/isAbsolute-posix.js n=100000 path='bar/baz' *** -23.61 % ±0.52% ±0.69% ±0.88%
00:16:45 path/isAbsolute-win32.js n=100000 path='.' *** -21.81 % ±0.67% ±0.88% ±1.14%
00:16:45 path/isAbsolute-win32.js n=100000 path='' *** -22.27 % ±0.89% ±1.17% ±1.51%
00:16:45 path/isAbsolute-win32.js n=100000 path='//server' *** -22.59 % ±0.95% ±1.25% ±1.61%
00:16:45 path/isAbsolute-win32.js n=100000 path='bar\\\\baz' *** -22.66 % ±0.47% ±0.62% ±0.79%
00:16:45 path/isAbsolute-win32.js n=100000 path='C:\\\\baz\\\\..' *** -15.54 % ±0.53% ±0.71% ±0.91%
00:16:45 path/isAbsolute-win32.js n=100000 path='C:baz\\\\..' *** -15.36 % ±0.58% ±0.77% ±0.99%
00:16:45 path/join-posix.js n=100000 paths='/foo|bar||baz/asdf|quux|..' *** -11.15 % ±0.48% ±0.64% ±0.82%
00:16:45 path/join-win32.js n=100000 paths='C:\\\\foo|bar||baz\\\\asdf|quux|..' *** -8.50 % ±0.44% ±0.58% ±0.74%
00:16:45 path/makeLong-win32.js n=100000 path='\\\\\\\\?\\\\foo' *** -8.99 % ±0.74% ±0.97% ±1.25%
00:16:45 path/makeLong-win32.js n=100000 path='\\\\\\\\foo\\\\bar' *** -9.08 % ±0.66% ±0.87% ±1.12%
00:16:45 path/makeLong-win32.js n=100000 path='C:\\\\foo' *** -8.34 % ±1.02% ±1.35% ±1.74%
00:16:45 path/makeLong-win32.js n=100000 path='foo\\\\bar' *** -4.10 % ±0.33% ±0.44% ±0.56%
00:16:45 path/normalize-posix.js n=100000 path='.' *** -12.86 % ±0.88% ±1.16% ±1.49%
00:16:45 path/normalize-posix.js n=100000 path='' *** -17.22 % ±1.32% ±1.75% ±2.25%
00:16:45 path/normalize-posix.js n=100000 path='/../' *** -11.03 % ±1.08% ±1.42% ±1.83%
00:16:45 path/normalize-posix.js n=100000 path='/foo' *** -12.47 % ±0.46% ±0.60% ±0.78%
00:16:45 path/normalize-posix.js n=100000 path='/foo/bar' *** -5.82 % ±0.74% ±0.98% ±1.26%
00:16:45 path/normalize-posix.js n=100000 path='/foo/bar//baz/asdf/quux/..' * -0.73 % ±0.58% ±0.76% ±0.98%
00:16:45 path/normalize-win32.js n=100000 path='.' *** -9.51 % ±1.40% ±1.85% ±2.38%
00:16:45 path/normalize-win32.js n=100000 path='' *** -15.65 % ±1.36% ±1.80% ±2.31%
00:16:45 path/normalize-win32.js n=100000 path='C:\\\\..\\\\' *** -6.19 % ±1.01% ±1.33% ±1.71%
00:16:45 path/normalize-win32.js n=100000 path='C:\\\\foo' *** -6.86 % ±0.96% ±1.26% ±1.62%
00:16:45 path/normalize-win32.js n=100000 path='C:\\\\foo\\\\bar' *** -5.55 % ±0.88% ±1.16% ±1.49%
00:16:45 path/normalize-win32.js n=100000 path='C:\\\\foo\\\\bar\\\\\\\\baz\\\\asdf\\\\quux\\\\..' *** -2.20 % ±0.62% ±0.81% ±1.04%
00:16:45 path/parse-posix.js n=100000 path='' *** -15.52 % ±1.31% ±1.73% ±2.23%
00:16:45 path/parse-posix.js n=100000 path='/' *** -13.87 % ±0.94% ±1.25% ±1.60%
00:16:45 path/parse-posix.js n=100000 path='/foo' *** -12.18 % ±0.83% ±1.10% ±1.42%
00:16:45 path/parse-posix.js n=100000 path='/foo/bar.baz' *** -3.80 % ±1.35% ±1.79% ±2.31%
00:16:45 path/parse-posix.js n=100000 path='/foo/bar/baz/asdf/.quux' *** -7.80 % ±1.31% ±1.73% ±2.22%
00:16:45 path/parse-posix.js n=100000 path='foo/.bar.baz' -0.84 % ±1.43% ±1.89% ±2.44%
00:16:45 path/parse-posix.js n=100000 path='foo/bar' *** -9.94 % ±0.77% ±1.01% ±1.31%
00:16:45 path/parse-win32.js n=100000 path='' *** -14.42 % ±1.28% ±1.69% ±2.17%
00:16:45 path/parse-win32.js n=100000 path='\\\\foo' *** -7.94 % ±0.93% ±1.22% ±1.57%
00:16:45 path/parse-win32.js n=100000 path='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\.quux' *** -8.25 % ±1.60% ±2.12% ±2.72%
00:16:45 path/parse-win32.js n=100000 path='C:\\\\' *** -11.99 % ±1.17% ±1.55% ±1.99%
00:16:45 path/parse-win32.js n=100000 path='C:\\\\foo' *** -11.01 % ±0.96% ±1.27% ±1.64%
00:16:45 path/parse-win32.js n=100000 path='E:\\\\foo\\\\bar.baz' *** -7.39 % ±1.20% ±1.59% ±2.04%
00:16:45 path/parse-win32.js n=100000 path='foo\\\\.bar.baz' *** -6.38 % ±1.15% ±1.52% ±1.95%
00:16:45 path/parse-win32.js n=100000 path='foo\\\\bar' *** -8.37 % ±1.34% ±1.76% ±2.27%
00:16:45 path/relative-posix.js n=100000 paths='/|/' *** -26.78 % ±0.95% ±1.25% ±1.61%
00:16:45 path/relative-posix.js n=100000 paths='/|/var' *** -20.41 % ±0.59% ±0.78% ±1.00%
00:16:45 path/relative-posix.js n=100000 paths='/data/orandea/test/aaa|/data/orandea/impl/bbb' *** -7.77 % ±0.26% ±0.35% ±0.45%
00:16:45 path/relative-posix.js n=100000 paths='/foo/bar/baz/quux|/' *** -8.79 % ±0.23% ±0.30% ±0.38%
00:16:45 path/relative-posix.js n=100000 paths='/foo/bar/baz/quux|/foo/bar/baz/quux' *** -18.01 % ±1.98% ±2.62% ±3.37%
00:16:45 path/relative-posix.js n=100000 paths='/foo/bar/baz/quux|/var/log' *** -8.13 % ±0.24% ±0.31% ±0.40%
00:16:45 path/relative-posix.js n=100000 paths='/var|/bin' *** -14.60 % ±0.35% ±0.46% ±0.60%
00:16:45 path/relative-win32.js n=100000 paths='C:\\\\|D:\\\\' *** -15.54 % ±0.79% ±1.05% ±1.34%
00:16:45 path/relative-win32.js n=100000 paths='C:\\\\foo\\\\bar\\\\baz\\\\quux|C:\\\\' *** -4.45 % ±0.54% ±0.71% ±0.91%
00:16:45 path/relative-win32.js n=100000 paths='C:\\\\foo\\\\bar\\\\baz|C:\\\\foo\\\\bar\\\\baz' *** -13.06 % ±2.18% ±2.88% ±3.71%
00:16:45 path/relative-win32.js n=100000 paths='C:\\\\foo\\\\BAR\\\\BAZ|C:\\\\foo\\\\bar\\\\baz' *** -5.72 % ±0.33% ±0.44% ±0.56%
00:16:45 path/relative-win32.js n=100000 paths='C:\\\\orandea\\\\test\\\\aaa|C:\\\\orandea\\\\impl\\\\bbb' *** -3.19 % ±0.21% ±0.28% ±0.36%
00:16:45 path/resolve-posix.js n=100000 paths='' *** -4.00 % ±0.52% ±0.69% ±0.89%
00:16:45 path/resolve-posix.js n=100000 paths='|' *** -5.76 % ±0.53% ±0.69% ±0.89%
00:16:45 path/resolve-posix.js n=100000 paths='a/b/c/|../../..' -0.26 % ±0.48% ±0.63% ±0.81%
00:16:45 path/resolve-posix.js n=100000 paths='foo/bar|/tmp/file/|..|a/../subfile' *** -3.02 % ±0.65% ±0.86% ±1.11%
00:16:45 path/resolve-win32.js n=100000 paths='' *** -3.72 % ±0.52% ±0.69% ±0.89%
00:16:45 path/resolve-win32.js n=100000 paths='|' *** -5.68 % ±0.55% ±0.72% ±0.93%
00:16:45 path/resolve-win32.js n=100000 paths='c:/blah\\\\blah|d:/games|c:../a' *** -5.75 % ±0.47% ±0.63% ±0.81%
00:16:45 path/resolve-win32.js n=100000 paths='c:/ignore|d:\\\\a/b\\\\c/d|\\\\e.exe' *** -7.96 % ±1.01% ±1.33% ±1.71%
00:16:45 path/toNamespacedPath-posix.js n=100000 path='.' *** -79.53 % ±0.88% ±1.16% ±1.50%
00:16:45 path/toNamespacedPath-posix.js n=100000 path='' *** -79.71 % ±1.12% ±1.49% ±1.92%
00:16:45 path/toNamespacedPath-posix.js n=100000 path='/home/node/..' *** -79.21 % ±0.66% ±0.87% ±1.13%
00:16:45 path/toNamespacedPath-posix.js n=100000 path='/tmp/bar' *** -79.35 % ±0.54% ±0.72% ±0.92%
00:16:45 path/toNamespacedPath-posix.js n=100000 path='/w/bnch-comp/node/benchmark' *** -79.31 % ±0.64% ±0.85% ±1.10%
00:16:45 path/toNamespacedPath-posix.js n=100000 path='bar/baz' *** -78.92 % ±1.15% ±1.52% ±1.96%
00:16:45 path/toNamespacedPath-win32.js n=100000 path='' *** -68.07 % ±1.45% ±1.91% ±2.47%
00:16:45 path/toNamespacedPath-win32.js n=100000 path='\\\\e.exe' *** -10.52 % ±1.16% ±1.53% ±1.97%
00:16:45 path/toNamespacedPath-win32.js n=100000 path='\\\\w\\\\bnch-comp\\\\node\\\\benchmark' *** -3.06 % ±0.56% ±0.74% ±0.95%
00:16:45 path/toNamespacedPath-win32.js n=100000 path='c:../a' *** -2.87 % ±0.31% ±0.40% ±0.52%
00:16:45 path/toNamespacedPath-win32.js n=100000 path='c:/blah\\\\blah' *** -7.68 % ±0.53% ±0.70% ±0.91%
00:16:45 path/toNamespacedPath-win32.js n=100000 path='c:/ignore' *** -10.49 % ±1.06% ±1.40% ±1.80%
00:16:45 path/toNamespacedPath-win32.js n=100000 path='d:/games' *** -12.96 % ±0.95% ±1.25% ±1.60%
00:16:45 path/toNamespacedPath-win32.js n=100000 path='d:\\\\a/b\\\\c/d' *** -6.92 % ±0.84% ±1.11% ±1.42%

@avivkeller

This comment was marked as duplicate.

@LiviaMedeiros

Copy link
Copy Markdown
Member

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1535/

Comment threaddoc/api/path.md Outdated
Comment threaddoc/api/path.md Outdated

@LiviaMedeirosLiviaMedeiros left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark CI results
confidenceimprovement accuracy (*) (**) (***)
path/basename-posix.jsn=100000pathext=''-0.55 % ±1.29% ±1.71% ±2.20%
path/basename-posix.jsn=100000pathext='/'***-1.38 % ±0.78% ±1.03% ±1.32%
path/basename-posix.jsn=100000pathext='/foo'*-1.16 % ±1.07% ±1.41% ±1.82%
path/basename-posix.jsn=100000pathext='/foo/.bar.baz'0.19 % ±1.27% ±1.68% ±2.16%
path/basename-posix.jsn=100000pathext='/foo/.bar.baz|.baz'-0.87 % ±1.35% ±1.79% ±2.30%
path/basename-posix.jsn=100000pathext='/foo/bar/baz/asdf/quux.html'-0.10 % ±1.34% ±1.77% ±2.28%
path/basename-posix.jsn=100000pathext='/foo/bar/baz/asdf/quux.html|.html'**-2.45 % ±1.68% ±2.22% ±2.85%
path/basename-posix.jsn=100000pathext='foo'***-1.88 % ±0.75% ±0.99% ±1.27%
path/basename-posix.jsn=100000pathext='foo/bar.'-0.59 % ±0.81% ±1.07% ±1.38%
path/basename-posix.jsn=100000pathext='foo/bar.|.'**-0.98 % ±0.72% ±0.94% ±1.22%
path/basename-win32.jsn=100000pathext=''**-1.11 % ±0.82% ±1.08% ±1.39%
path/basename-win32.jsn=100000pathext='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html'-0.05 % ±0.93% ±1.23% ±1.58%
path/basename-win32.jsn=100000pathext='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html|.html'-0.75 % ±1.16% ±1.52% ±1.96%
path/basename-win32.jsn=100000pathext='C:\\\\'-0.10 % ±0.81% ±1.06% ±1.37%
path/basename-win32.jsn=100000pathext='C:\\\\foo'***-2.59 % ±1.06% ±1.40% ±1.80%
path/basename-win32.jsn=100000pathext='D:\\\\foo\\\\.bar.baz'*-1.10 % ±0.85% ±1.12% ±1.44%
path/basename-win32.jsn=100000pathext='E:\\\\foo\\\\.bar.baz|.baz'**-1.50 % ±0.90% ±1.19% ±1.53%
path/basename-win32.jsn=100000pathext='foo'***-0.62 % ±0.35% ±0.46% ±0.59%
path/basename-win32.jsn=100000pathext='foo\\\\bar.'*-0.86 % ±0.75% ±0.99% ±1.28%
path/basename-win32.jsn=100000pathext='foo\\\\bar.|.'**-1.45 % ±1.02% ±1.35% ±1.73%
path/dirname-posix.jsn=100000path=''**-1.35 % ±0.80% ±1.06% ±1.36%
path/dirname-posix.jsn=100000path='/'***-1.48 % ±0.73% ±0.97% ±1.24%
path/dirname-posix.jsn=100000path='/foo'***-1.54 % ±0.56% ±0.73% ±0.94%
path/dirname-posix.jsn=100000path='/foo/bar'***-1.23 % ±0.66% ±0.87% ±1.12%
path/dirname-posix.jsn=100000path='/foo/bar/baz/asdf/quux'*-1.59 % ±1.29% ±1.70% ±2.18%
path/dirname-posix.jsn=100000path='foo'***-1.81 % ±0.52% ±0.69% ±0.89%
path/dirname-posix.jsn=100000path='foo/bar'***-1.43 % ±0.35% ±0.46% ±0.60%
path/dirname-win32.jsn=100000path=''***-4.07 % ±1.21% ±1.59% ±2.04%
path/dirname-win32.jsn=100000path='\\\\'***-1.45 % ±0.81% ±1.07% ±1.38%
path/dirname-win32.jsn=100000path='\\\\foo'**-1.22 % ±0.86% ±1.13% ±1.45%
path/dirname-win32.jsn=100000path='C:\\\\foo\\\\bar'-0.82 % ±0.90% ±1.18% ±1.52%
path/dirname-win32.jsn=100000path='D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux'-0.65 % ±1.09% ±1.44% ±1.85%
path/dirname-win32.jsn=100000path='foo'***-1.90 % ±1.05% ±1.39% ±1.78%
path/dirname-win32.jsn=100000path='foo\\\\bar'-0.42 % ±0.59% ±0.78% ±1.01%
path/extname-posix.jsn=100000path=''***-4.55 % ±2.46% ±3.25% ±4.18%
path/extname-posix.jsn=100000path='/'***-2.05 % ±1.05% ±1.38% ±1.78%
path/extname-posix.jsn=100000path='/foo'***-4.64 % ±0.87% ±1.15% ±1.48%
path/extname-posix.jsn=100000path='/foo/bar/baz/asdf/quux.foobarbazasdfquux'-1.14 % ±1.64% ±2.16% ±2.77%
path/extname-posix.jsn=100000path='/foo/bar/baz/asdf/quux'0.24 % ±1.28% ±1.69% ±2.17%
path/extname-posix.jsn=100000path='foo/.bar.baz'0.77 % ±1.10% ±1.45% ±1.86%
path/extname-posix.jsn=100000path='foo/bar/...baz.quux'-1.58 % ±1.62% ±2.14% ±2.75%
path/extname-posix.jsn=100000path='foo/bar/..baz.quux'*-1.75 % ±1.72% ±2.27% ±2.92%
path/extname-posix.jsn=100000path='index.html'-0.62 % ±1.28% ±1.68% ±2.16%
path/extname-posix.jsn=100000path='index'0.13 % ±1.16% ±1.53% ±1.96%
path/extname-win32.jsn=100000path=''***-1.99 % ±1.03% ±1.36% ±1.75%
path/extname-win32.jsn=100000path='\\\\'***-3.51 % ±1.45% ±1.91% ±2.46%
path/extname-win32.jsn=100000path='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.foobarbazasdfquux'-0.54 % ±1.47% ±1.94% ±2.49%
path/extname-win32.jsn=100000path='C:\\\\foo'*-0.99 % ±0.83% ±1.10% ±1.41%
path/extname-win32.jsn=100000path='D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux'0.36 % ±1.02% ±1.34% ±1.72%
path/extname-win32.jsn=100000path='foo\\\\.bar.baz'0.18 % ±1.17% ±1.54% ±1.98%
path/extname-win32.jsn=100000path='foo\\\\bar\\\\...baz.quux'0.18 % ±1.07% ±1.41% ±1.82%
path/extname-win32.jsn=100000path='foo\\\\bar\\\\..baz.quux'*-1.29 % ±1.16% ±1.53% ±1.96%
path/extname-win32.jsn=100000path='index.html'*-1.05 % ±0.84% ±1.11% ±1.42%
path/extname-win32.jsn=100000path='index'***-2.03 % ±1.05% ±1.38% ±1.77%
path/format-posix.jsn=1000000props='/|/home/user/dir|index.html|.html|index'0.18 % ±0.46% ±0.61% ±0.78%
path/format-win32.jsn=1000000props='C:\\\\|C:\\\\path\\\\dir|index.html|.html|index'0.15 % ±0.39% ±0.51% ±0.65%
path/isAbsolute-posix.jsn=100000path='.'***-2.05 % ±0.85% ±1.12% ±1.44%
path/isAbsolute-posix.jsn=100000path=''***-3.72 % ±1.05% ±1.38% ±1.78%
path/isAbsolute-posix.jsn=100000path='/baz/..'***-1.40 % ±0.69% ±0.91% ±1.17%
path/isAbsolute-posix.jsn=100000path='/foo/bar'***-2.54 % ±0.82% ±1.08% ±1.39%
path/isAbsolute-posix.jsn=100000path='bar/baz'***-2.25 % ±0.71% ±0.93% ±1.20%
path/isAbsolute-win32.jsn=100000path='.'***-1.45 % ±0.64% ±0.84% ±1.08%
path/isAbsolute-win32.jsn=100000path=''-1.14 % ±1.15% ±1.52% ±1.95%
path/isAbsolute-win32.jsn=100000path='//server'***-6.37 % ±1.03% ±1.36% ±1.75%
path/isAbsolute-win32.jsn=100000path='bar\\\\baz'***-1.83 % ±0.46% ±0.61% ±0.78%
path/isAbsolute-win32.jsn=100000path='C:\\\\baz\\\\..'*-0.54 % ±0.43% ±0.57% ±0.73%
path/isAbsolute-win32.jsn=100000path='C:baz\\\\..'***-0.90 % ±0.44% ±0.58% ±0.75%
path/join-posix.jsn=100000paths='/foo|bar||baz/asdf|quux|..'0.29 % ±0.56% ±0.74% ±0.95%
path/join-win32.jsn=100000paths='C:\\\\foo|bar||baz\\\\asdf|quux|..'***-1.12 % ±0.48% ±0.64% ±0.82%
path/makeLong-win32.jsn=100000path='\\\\\\\\?\\\\foo'-0.40 % ±0.54% ±0.72% ±0.92%
path/makeLong-win32.jsn=100000path='\\\\\\\\foo\\\\bar'-0.49 % ±0.77% ±1.01% ±1.30%
path/makeLong-win32.jsn=100000path='C:\\\\foo'-0.49 % ±1.46% ±1.93% ±2.48%
path/makeLong-win32.jsn=100000path='foo\\\\bar'-0.30 % ±0.42% ±0.55% ±0.70%
path/normalize-posix.jsn=100000path='.'***-1.70 % ±0.61% ±0.81% ±1.04%
path/normalize-posix.jsn=100000path=''***-1.73 % ±0.92% ±1.22% ±1.57%
path/normalize-posix.jsn=100000path='/../'***-1.27 % ±0.45% ±0.60% ±0.77%
path/normalize-posix.jsn=100000path='/foo'**-1.62 % ±1.03% ±1.36% ±1.75%
path/normalize-posix.jsn=100000path='/foo/bar'***-2.05 % ±0.83% ±1.10% ±1.42%
path/normalize-posix.jsn=100000path='/foo/bar//baz/asdf/quux/..'***1.49 % ±0.67% ±0.88% ±1.13%
path/normalize-win32.jsn=100000path='.'-0.28 % ±1.13% ±1.49% ±1.91%
path/normalize-win32.jsn=100000path=''*-1.65 % ±1.29% ±1.71% ±2.19%
path/normalize-win32.jsn=100000path='C:\\\\..\\\\'*-1.15 % ±0.99% ±1.31% ±1.68%
path/normalize-win32.jsn=100000path='C:\\\\foo'***-1.48 % ±0.87% ±1.15% ±1.48%
path/normalize-win32.jsn=100000path='C:\\\\foo\\\\bar'***-1.85 % ±0.77% ±1.02% ±1.31%
path/normalize-win32.jsn=100000path='C:\\\\foo\\\\bar\\\\\\\\baz\\\\asdf\\\\quux\\\\..'0.02 % ±0.54% ±0.72% ±0.92%
path/parse-posix.jsn=100000path=''***-3.53 % ±1.20% ±1.59% ±2.04%
path/parse-posix.jsn=100000path='/'***-1.68 % ±0.70% ±0.92% ±1.18%
path/parse-posix.jsn=100000path='/foo'*-0.73 % ±0.57% ±0.75% ±0.97%
path/parse-posix.jsn=100000path='/foo/bar.baz'-0.59 % ±1.24% ±1.63% ±2.10%
path/parse-posix.jsn=100000path='/foo/bar/baz/asdf/.quux'1.10 % ±1.57% ±2.07% ±2.66%
path/parse-posix.jsn=100000path='foo/.bar.baz'*-1.90 % ±1.66% ±2.19% ±2.82%
path/parse-posix.jsn=100000path='foo/bar'-0.86 % ±0.91% ±1.20% ±1.55%
path/parse-win32.jsn=100000path=''0.16 % ±1.16% ±1.53% ±1.97%
path/parse-win32.jsn=100000path='\\\\foo'***-1.56 % ±0.84% ±1.11% ±1.42%
path/parse-win32.jsn=100000path='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\.quux'*-1.82 % ±1.66% ±2.19% ±2.81%
path/parse-win32.jsn=100000path='C:\\\\'***-2.60 % ±1.05% ±1.39% ±1.79%
path/parse-win32.jsn=100000path='C:\\\\foo'*-1.17 % ±1.08% ±1.43% ±1.84%
path/parse-win32.jsn=100000path='E:\\\\foo\\\\bar.baz'-0.56 % ±1.13% ±1.49% ±1.92%
path/parse-win32.jsn=100000path='foo\\\\.bar.baz'0.27 % ±1.07% ±1.42% ±1.82%
path/parse-win32.jsn=100000path='foo\\\\bar'*-1.44 % ±1.24% ±1.64% ±2.11%
path/relative-posix.jsn=100000paths='/|/'***-2.31 % ±1.21% ±1.59% ±2.05%
path/relative-posix.jsn=100000paths='/|/var'***-4.80 % ±1.05% ±1.39% ±1.79%
path/relative-posix.jsn=100000paths='/data/orandea/test/aaa|/data/orandea/impl/bbb'***-1.63 % ±0.32% ±0.43% ±0.55%
path/relative-posix.jsn=100000paths='/foo/bar/baz/quux|/'***-1.50 % ±0.25% ±0.34% ±0.43%
path/relative-posix.jsn=100000paths='/foo/bar/baz/quux|/foo/bar/baz/quux'-0.42 % ±2.42% ±3.19% ±4.10%
path/relative-posix.jsn=100000paths='/foo/bar/baz/quux|/var/log'***-1.23 % ±0.24% ±0.32% ±0.41%
path/relative-posix.jsn=100000paths='/var|/bin'***-0.92 % ±0.50% ±0.66% ±0.85%
path/relative-win32.jsn=100000paths='C:\\\\|D:\\\\'0.29 % ±0.87% ±1.15% ±1.48%
path/relative-win32.jsn=100000paths='C:\\\\foo\\\\bar\\\\baz\\\\quux|C:\\\\'0.33 % ±0.56% ±0.73% ±0.94%
path/relative-win32.jsn=100000paths='C:\\\\foo\\\\bar\\\\baz|C:\\\\foo\\\\bar\\\\baz'2.56 % ±2.64% ±3.48% ±4.47%
path/relative-win32.jsn=100000paths='C:\\\\foo\\\\BAR\\\\BAZ|C:\\\\foo\\\\bar\\\\baz'***1.07 % ±0.36% ±0.48% ±0.62%
path/relative-win32.jsn=100000paths='C:\\\\orandea\\\\test\\\\aaa|C:\\\\orandea\\\\impl\\\\bbb'-0.03 % ±0.27% ±0.36% ±0.46%
path/resolve-posix.jsn=100000paths=''0.24 % ±0.58% ±0.77% ±0.99%
path/resolve-posix.jsn=100000paths='|'0.08 % ±0.82% ±1.08% ±1.39%
path/resolve-posix.jsn=100000paths='a/b/c/|../../..'-0.03 % ±0.43% ±0.56% ±0.72%
path/resolve-posix.jsn=100000paths='foo/bar|/tmp/file/|..|a/../subfile'0.43 % ±0.50% ±0.65% ±0.84%
path/resolve-win32.jsn=100000paths=''0.56 % ±0.60% ±0.79% ±1.01%
path/resolve-win32.jsn=100000paths='|'-0.11 % ±0.76% ±1.01% ±1.30%
path/resolve-win32.jsn=100000paths='c:/blah\\\\blah|d:/games|c:../a'0.00 % ±0.49% ±0.65% ±0.84%
path/resolve-win32.jsn=100000paths='c:/ignore|d:\\\\a/b\\\\c/d|\\\\e.exe'-0.27 % ±0.86% ±1.13% ±1.45%
path/toNamespacedPath-posix.jsn=100000path='.'***-20.23 % ±0.97% ±1.28% ±1.65%
path/toNamespacedPath-posix.jsn=100000path=''***-21.27 % ±1.36% ±1.79% ±2.30%
path/toNamespacedPath-posix.jsn=100000path='/home/node/..'***-19.78 % ±0.80% ±1.06% ±1.36%
path/toNamespacedPath-posix.jsn=100000path='/tmp/bar'***-19.69 % ±0.91% ±1.20% ±1.54%
path/toNamespacedPath-posix.jsn=100000path='/w/bnch-comp/node/benchmark'***-19.40 % ±0.89% ±1.17% ±1.51%
path/toNamespacedPath-posix.jsn=100000path='bar/baz'***-20.50 % ±1.00% ±1.32% ±1.70%
path/toNamespacedPath-win32.jsn=100000path=''***-5.66 % ±1.83% ±2.41% ±3.10%
path/toNamespacedPath-win32.jsn=100000path='\\\\e.exe'0.48 % ±1.46% ±1.93% ±2.48%
path/toNamespacedPath-win32.jsn=100000path='\\\\w\\\\bnch-comp\\\\node\\\\benchmark'0.20 % ±0.70% ±0.92% ±1.19%
path/toNamespacedPath-win32.jsn=100000path='c:../a'*-0.38 % ±0.36% ±0.48% ±0.61%
path/toNamespacedPath-win32.jsn=100000path='c:/blah\\\\blah'**-1.70 % ±1.17% ±1.54% ±1.98%
path/toNamespacedPath-win32.jsn=100000path='c:/ignore'-0.33 % ±1.46% ±1.93% ±2.48%
path/toNamespacedPath-win32.jsn=100000path='d:/games'*1.40 % ±1.23% ±1.63% ±2.09%
path/toNamespacedPath-win32.jsn=100000path='d:\\\\a/b\\\\c/d'-0.78 % ±0.84% ±1.10% ±1.42%

Looks good overall, the only benchmark with significant performance impact is path/toNamespacedPath-posix.js.
IMHO it's good enough to land as is.

For context, originally path.posix.toNamespacedPath() is a noop. I doubt that this function is popular in userspace, but it's heavily used in Node.js internals (mostly in pair with getValidatedPath(), and in internal/modules).
There is an opened PR moving most of it to c++ (#52135) that should disconnect fs from performance impacts for js version of toNamespacedPath().

Alternatively on js side, I think we can reduce performance penalty in internals by introducing two versions of toNamespacedPath(): internal API without URL support and as public API with it.
@anonrig WDYT?

@avivkeller

avivkeller commented Apr 28, 2024

Copy link
Copy Markdown
MemberAuthor

While the performance impact seems significant, I don't think it'll be noticeable (but I haven't tested it yet for the actual MS timings). It's just an extra typeof check.

As @LiviaMedeiros said, I could also make an internal function for it to be used in the internals.

@avivkeller
avivkeller requested a review from anonrigApril 28, 2024 14:14
@anonrig

Copy link
Copy Markdown
Member

Can we run the benchmarks of file system operations? I don't think 20% is a cost that is acceptable.

@LiviaMedeiros

LiviaMedeiros commented Apr 28, 2024

Copy link
Copy Markdown
Member

fs benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1541/

Results
confidenceimprovement accuracy (*) (**) (***)
fs/bench_fdatasyncSync.jsn=10000type='existing'-1.23 % ±3.32% ±4.47% ±5.91%
fs/bench_fdatasyncSync.jsn=10000type='non-existing'1.53 % ±3.01% ±4.01% ±5.21%
fs/bench-accessSync.jsn=100000type='existing'**-0.55 % ±0.37% ±0.50% ±0.65%
fs/bench-accessSync.jsn=100000type='non-existing'**-0.59 % ±0.39% ±0.51% ±0.67%
fs/bench-accessSync.jsn=100000type='non-flat-existing'***-1.19 % ±0.54% ±0.72% ±0.94%
fs/bench-chmodSync.jsn=1000type='existing'-0.77 % ±1.32% ±1.75% ±2.27%
fs/bench-chmodSync.jsn=1000type='non-existing'-0.65 % ±3.54% ±4.71% ±6.13%
fs/bench-chownSync.jsn=10000method='chownSync'type='existing'-0.83 % ±3.07% ±4.09% ±5.32%
fs/bench-chownSync.jsn=10000method='chownSync'type='non-existing'-0.46 % ±1.19% ±1.58% ±2.05%
fs/bench-chownSync.jsn=10000method='lchownSync'type='existing'-1.61 % ±3.33% ±4.43% ±5.76%
fs/bench-chownSync.jsn=10000method='lchownSync'type='non-existing'0.17 % ±1.90% ±2.52% ±3.28%
fs/bench-copyFileSync.jsn=10000type='invalid'-0.21 % ±1.47% ±1.95% ±2.54%
fs/bench-copyFileSync.jsn=10000type='valid'-0.12 % ±2.17% ±2.89% ±3.77%
fs/bench-existsSync.jsn=1000000type='existing'0.01 % ±0.62% ±0.82% ±1.07%
fs/bench-existsSync.jsn=1000000type='non-existing'-0.46 % ±0.58% ±0.77% ±1.00%
fs/bench-existsSync.jsn=1000000type='non-flat-existing'-0.34 % ±0.53% ±0.71% ±0.92%
fs/bench-fchmodSync.jsn=1000type='existing'-4.09 % ±10.26% ±13.77% ±18.16%
fs/bench-fchmodSync.jsn=1000type='non-existing'-0.21 % ±4.23% ±5.63% ±7.32%
fs/bench-fsyncSync.jsn=10000type='existing'-0.22 % ±0.25% ±0.33% ±0.43%
fs/bench-fsyncSync.jsn=10000type='non-existing'-1.67 % ±3.15% ±4.18% ±5.44%
fs/bench-ftruncateSync.jsn=10000type='invalid'-0.37 % ±1.65% ±2.19% ±2.85%
fs/bench-ftruncateSync.jsn=10000type='valid'1.48 % ±3.99% ±5.30% ±6.89%
fs/bench-linkSync.jsn=1000type='invalid'1.10 % ±4.59% ±6.10% ±7.94%
fs/bench-linkSync.jsn=1000type='valid'*4.18 % ±3.58% ±4.76% ±6.20%
fs/bench-mkdirp.jsn=10000*-22.03 % ±18.58% ±24.70% ±32.12%
fs/bench-mkdirSync.jsn=1000recursive='false'type='existing'1.05 % ±3.25% ±4.33% ±5.64%
fs/bench-mkdirSync.jsn=1000recursive='false'type='non-existing'-5.59 % ±12.80% ±17.01% ±22.12%
fs/bench-mkdirSync.jsn=1000recursive='true'type='existing'1.87 % ±3.08% ±4.10% ±5.34%
fs/bench-mkdirSync.jsn=1000recursive='true'type='non-existing'1.90 % ±11.93% ±15.87% ±20.62%
fs/bench-mkdtempSync.jsn=10000type='invalid'-1.67 % ±1.81% ±2.42% ±3.17%
fs/bench-mkdtempSync.jsn=10000type='valid-buffer'-2.31 % ±9.48% ±12.60% ±16.38%
fs/bench-mkdtempSync.jsn=10000type='valid-string'2.64 % ±9.75% ±12.96% ±16.85%
fs/bench-opendir.jsbufferSize=1024mode='async'dir='lib'n=100-3.89 % ±4.22% ±5.62% ±7.33%
fs/bench-opendir.jsbufferSize=1024mode='async'dir='test/parallel'n=100-0.20 % ±0.63% ±0.84% ±1.10%
fs/bench-opendir.jsbufferSize=1024mode='callback'dir='lib'n=100-0.20 % ±2.08% ±2.76% ±3.59%
fs/bench-opendir.jsbufferSize=1024mode='callback'dir='test/parallel'n=100-2.80 % ±14.35% ±19.08% ±24.80%
fs/bench-opendir.jsbufferSize=1024mode='sync'dir='lib'n=100-1.28 % ±1.56% ±2.07% ±2.70%
fs/bench-opendir.jsbufferSize=1024mode='sync'dir='test/parallel'n=100-0.04 % ±0.64% ±0.85% ±1.11%
fs/bench-opendir.jsbufferSize=32mode='async'dir='lib'n=100-1.60 % ±3.19% ±4.24% ±5.53%
fs/bench-opendir.jsbufferSize=32mode='async'dir='test/parallel'n=1000.18 % ±0.64% ±0.85% ±1.10%
fs/bench-opendir.jsbufferSize=32mode='callback'dir='lib'n=100-0.37 % ±1.77% ±2.35% ±3.06%
fs/bench-opendir.jsbufferSize=32mode='callback'dir='test/parallel'n=1001.98 % ±2.52% ±3.35% ±4.36%
fs/bench-opendir.jsbufferSize=32mode='sync'dir='lib'n=100-0.29 % ±1.13% ±1.50% ±1.95%
fs/bench-opendir.jsbufferSize=32mode='sync'dir='test/parallel'n=100**-0.64 % ±0.43% ±0.57% ±0.75%
fs/bench-opendir.jsbufferSize=4mode='async'dir='lib'n=1000.11 % ±1.61% ±2.14% ±2.79%
fs/bench-opendir.jsbufferSize=4mode='async'dir='test/parallel'n=100-0.09 % ±0.64% ±0.85% ±1.10%
fs/bench-opendir.jsbufferSize=4mode='callback'dir='lib'n=100-0.80 % ±1.39% ±1.85% ±2.41%
fs/bench-opendir.jsbufferSize=4mode='callback'dir='test/parallel'n=100-0.71 % ±0.80% ±1.06% ±1.38%
fs/bench-opendir.jsbufferSize=4mode='sync'dir='lib'n=100-1.55 % ±1.61% ±2.15% ±2.79%
fs/bench-opendir.jsbufferSize=4mode='sync'dir='test/parallel'n=100***-1.67 % ±0.34% ±0.46% ±0.60%
fs/bench-opendirSync.jsn=1000type='existing'-0.04 % ±0.42% ±0.57% ±0.74%
fs/bench-opendirSync.jsn=1000type='non-existing'0.75 % ±4.46% ±5.93% ±7.71%
fs/bench-openSync.jsn=100000type='existing'*0.76 % ±0.68% ±0.91% ±1.18%
fs/bench-openSync.jsn=100000type='non-existing'-0.06 % ±0.43% ±0.57% ±0.74%
fs/bench-readdir.jswithFileTypes='false'dir='lib'n=103.13 % ±10.01% ±13.30% ±17.29%
fs/bench-readdir.jswithFileTypes='false'dir='test/parallel'n=102.55 % ±8.29% ±11.03% ±14.34%
fs/bench-readdir.jswithFileTypes='true'dir='lib'n=101.98 % ±11.27% ±15.01% ±19.57%
fs/bench-readdir.jswithFileTypes='true'dir='test/parallel'n=100.25 % ±6.94% ±9.24% ±12.03%
fs/bench-readdirSync.jswithFileTypes='false'dir='lib'n=10-0.12 % ±3.13% ±4.16% ±5.41%
fs/bench-readdirSync.jswithFileTypes='false'dir='test/parallel'n=101.63 % ±3.17% ±4.23% ±5.51%
fs/bench-readdirSync.jswithFileTypes='true'dir='lib'n=10-0.85 % ±3.30% ±4.39% ±5.71%
fs/bench-readdirSync.jswithFileTypes='true'dir='test/parallel'n=10-0.68 % ±2.87% ±3.82% ±4.98%
fs/bench-readlinkSync.jsn=1000type='invalid'-2.76 % ±4.17% ±5.54% ±7.21%
fs/bench-readlinkSync.jsn=1000type='valid'0.77 % ±1.93% ±2.57% ±3.36%
fs/bench-readSync.jsn=10000type='existing'2.56 % ±2.78% ±3.73% ±4.90%
fs/bench-readSync.jsn=10000type='non-existing'0.01 % ±0.92% ±1.23% ±1.60%
fs/bench-readvSync.jsn=100000type='invalid'0.33 % ±0.57% ±0.75% ±0.98%
fs/bench-readvSync.jsn=100000type='valid'0.51 % ±1.20% ±1.60% ±2.08%
fs/bench-realpath.jspathType='relative'n=100000.42 % ±1.07% ±1.42% ±1.85%
fs/bench-realpath.jspathType='resolved'n=100000.48 % ±1.01% ±1.34% ±1.75%
fs/bench-realpathSync.jspathType='relative'n=10000**-1.27 % ±0.85% ±1.14% ±1.48%
fs/bench-realpathSync.jspathType='resolved'n=10000-0.48 % ±1.72% ±2.28% ±2.97%
fs/bench-renameSync.jsn=2000type='invalid'*-3.82 % ±3.66% ±4.88% ±6.37%
fs/bench-renameSync.jsn=2000type='valid'0.34 % ±0.51% ±0.68% ±0.88%
fs/bench-rmdirSync.jsn=10000type='existing'-3.30 % ±4.38% ±5.87% ±7.73%
fs/bench-rmdirSync.jsn=10000type='non-existing'0.83 % ±1.15% ±1.53% ±1.99%
fs/bench-stat-promise.jsstatType='fstat'n=200000-0.54 % ±0.99% ±1.32% ±1.72%
fs/bench-stat-promise.jsstatType='lstat'n=200000-0.23 % ±1.09% ±1.45% ±1.89%
fs/bench-stat-promise.jsstatType='stat'n=200000-0.56 % ±0.97% ±1.29% ±1.68%
fs/bench-stat.jsstatType='fstat'n=2000000.61 % ±1.37% ±1.83% ±2.38%
fs/bench-stat.jsstatType='lstat'n=200000-0.69 % ±1.09% ±1.45% ±1.88%
fs/bench-stat.jsstatType='stat'n=2000000.42 % ±1.23% ±1.63% ±2.12%
fs/bench-statSync-failure.jsthrowType='noThrow'statSyncType='lstatSync'n=10000-1.31 % ±3.03% ±4.03% ±5.24%
fs/bench-statSync-failure.jsthrowType='noThrow'statSyncType='statSync'n=100000.34 % ±3.84% ±5.11% ±6.67%
fs/bench-statSync-failure.jsthrowType='throw'statSyncType='fstatSync'n=100000.17 % ±0.96% ±1.28% ±1.67%
fs/bench-statSync-failure.jsthrowType='throw'statSyncType='lstatSync'n=100000.68 % ±2.20% ±2.93% ±3.81%
fs/bench-statSync-failure.jsthrowType='throw'statSyncType='statSync'n=100001.51 % ±1.61% ±2.14% ±2.79%
fs/bench-statSync.jsstatSyncType='fstatSync'n=10000-0.34 % ±2.33% ±3.09% ±4.02%
fs/bench-statSync.jsstatSyncType='lstatSync'n=100000.95 % ±3.20% ±4.26% ±5.54%
fs/bench-statSync.jsstatSyncType='statSync'n=100000.07 % ±1.57% ±2.09% ±2.73%
fs/bench-symlinkSync.jsn=1000type='invalid'0.71 % ±3.61% ±4.80% ±6.24%
fs/bench-symlinkSync.jsn=1000type='valid'-0.80 % ±3.41% ±4.53% ±5.89%
fs/bench-timesSync.jsn=1000func='futimes'type='existing'-0.50 % ±5.05% ±6.71% ±8.72%
fs/bench-timesSync.jsn=1000func='futimes'type='non-existing'1.75 % ±3.37% ±4.49% ±5.83%
fs/bench-timesSync.jsn=1000func='lutimes'type='existing'0.10 % ±1.07% ±1.42% ±1.84%
fs/bench-timesSync.jsn=1000func='lutimes'type='non-existing'-0.47 % ±3.27% ±4.35% ±5.66%
fs/bench-timesSync.jsn=1000func='utimes'type='existing'0.06 % ±0.94% ±1.25% ±1.63%
fs/bench-timesSync.jsn=1000func='utimes'type='non-existing'-1.15 % ±4.98% ±6.62% ±8.61%
fs/bench-unlinkSync.jsn=1000type='existing'-0.46 % ±0.66% ±0.88% ±1.15%
fs/bench-unlinkSync.jsn=1000type='non-existing'1.24 % ±2.60% ±3.47% ±4.54%
fs/bench-writeFileSync.jsn=1000func='writeFile'useBuffer='false'length=1024useFd='false'encoding='utf8'-13.17 % ±17.59% ±23.39% ±30.44%
fs/bench-writeFileSync.jsn=1000func='writeFile'useBuffer='false'length=1024useFd='true'encoding='utf8'-0.09 % ±2.82% ±3.76% ±4.90%
fs/bench-writeFileSync.jsn=1000func='writeFile'useBuffer='false'length=102400useFd='false'encoding='utf8'1.16 % ±10.89% ±14.49% ±18.86%
fs/bench-writeFileSync.jsn=1000func='writeFile'useBuffer='false'length=102400useFd='true'encoding='utf8'0.57 % ±0.99% ±1.31% ±1.71%
fs/bench-writeFileSync.jsn=1000func='writeFile'useBuffer='false'length=1048576useFd='false'encoding='utf8'0.75 % ±17.97% ±23.89% ±31.06%
fs/bench-writeFileSync.jsn=1000func='writeFile'useBuffer='false'length=1048576useFd='true'encoding='utf8'0.21 % ±0.28% ±0.38% ±0.50%
fs/bench-writevSync.jsn=100000type='invalid'-0.04 % ±0.52% ±0.69% ±0.90%
fs/bench-writevSync.jsn=100000type='valid'*0.73 % ±0.67% ±0.90% ±1.17%
fs/read-stream-throughput.jsn=1024highWaterMark=1024filesize=1024000encodingType='asc'*-0.74 % ±0.72% ±0.96% ±1.24%
fs/read-stream-throughput.jsn=1024highWaterMark=1024filesize=1024000encodingType='buf'-0.43 % ±0.98% ±1.30% ±1.69%
fs/read-stream-throughput.jsn=1024highWaterMark=1024filesize=1024000encodingType='utf'0.01 % ±0.48% ±0.64% ±0.84%
fs/read-stream-throughput.jsn=1024highWaterMark=1048576filesize=1024000encodingType='asc'0.82 % ±3.71% ±4.93% ±6.41%
fs/read-stream-throughput.jsn=1024highWaterMark=1048576filesize=1024000encodingType='buf'-20.09 % ±24.06% ±32.01% ±41.67%
fs/read-stream-throughput.jsn=1024highWaterMark=1048576filesize=1024000encodingType='utf'0.02 % ±0.08% ±0.10% ±0.14%
fs/read-stream-throughput.jsn=1024highWaterMark=4096filesize=1024000encodingType='asc'*-0.63 % ±0.57% ±0.76% ±0.99%
fs/read-stream-throughput.jsn=1024highWaterMark=4096filesize=1024000encodingType='buf'-0.68 % ±1.41% ±1.87% ±2.44%
fs/read-stream-throughput.jsn=1024highWaterMark=4096filesize=1024000encodingType='utf'0.11 % ±0.60% ±0.80% ±1.04%
fs/read-stream-throughput.jsn=1024highWaterMark=65535filesize=1024000encodingType='asc'0.06 % ±2.06% ±2.73% ±3.55%
fs/read-stream-throughput.jsn=1024highWaterMark=65535filesize=1024000encodingType='buf'-2.08 % ±11.12% ±14.79% ±19.23%
fs/read-stream-throughput.jsn=1024highWaterMark=65535filesize=1024000encodingType='utf'-0.11 % ±0.40% ±0.53% ±0.69%
fs/readfile-partitioned.jsconcurrent=1len=1024encoding=''duration=50.00 % ±0.35% ±0.46% ±0.60%
fs/readfile-partitioned.jsconcurrent=1len=1024encoding='utf-8'duration=5-0.19 % ±0.30% ±0.40% ±0.52%
fs/readfile-partitioned.jsconcurrent=1len=16777216encoding=''duration=50.10 % ±0.24% ±0.31% ±0.41%
fs/readfile-partitioned.jsconcurrent=1len=16777216encoding='utf-8'duration=50.77 % ±0.79% ±1.06% ±1.38%
fs/readfile-partitioned.jsconcurrent=10len=1024encoding=''duration=50.03 % ±0.74% ±0.99% ±1.29%
fs/readfile-partitioned.jsconcurrent=10len=1024encoding='utf-8'duration=5-0.12 % ±1.01% ±1.34% ±1.74%
fs/readfile-partitioned.jsconcurrent=10len=16777216encoding=''duration=5-0.54 % ±1.07% ±1.42% ±1.85%
fs/readfile-partitioned.jsconcurrent=10len=16777216encoding='utf-8'duration=5-0.07 % ±0.23% ±0.30% ±0.39%
fs/readfile-permission-enabled.jsconcurrent=1len=1024encoding=''duration=50.61 % ±1.36% ±1.81% ±2.36%
fs/readfile-permission-enabled.jsconcurrent=1len=1024encoding='utf-8'duration=50.06 % ±1.52% ±2.02% ±2.63%
fs/readfile-permission-enabled.jsconcurrent=1len=16777216encoding=''duration=524.73 % ±34.05% ±45.33% ±59.05%
fs/readfile-permission-enabled.jsconcurrent=1len=16777216encoding='utf-8'duration=5-0.01 % ±0.46% ±0.62% ±0.80%
fs/readfile-permission-enabled.jsconcurrent=10len=1024encoding=''duration=5-0.20 % ±0.68% ±0.90% ±1.17%
fs/readfile-permission-enabled.jsconcurrent=10len=1024encoding='utf-8'duration=50.06 % ±0.77% ±1.02% ±1.33%
fs/readfile-permission-enabled.jsconcurrent=10len=16777216encoding=''duration=50.06 % ±1.96% ±2.61% ±3.39%
fs/readfile-permission-enabled.jsconcurrent=10len=16777216encoding='utf-8'duration=5-0.26 % ±0.30% ±0.39% ±0.51%
fs/readfile-promises.jsconcurrent=1len=1024encoding=''duration=5-0.14 % ±0.52% ±0.69% ±0.90%
fs/readfile-promises.jsconcurrent=1len=1024encoding='utf-8'duration=5*0.61 % ±0.51% ±0.68% ±0.88%
fs/readfile-promises.jsconcurrent=1len=16777216encoding=''duration=54.94 % ±20.11% ±26.75% ±34.78%
fs/readfile-promises.jsconcurrent=1len=16777216encoding='utf-8'duration=50.15 % ±0.26% ±0.34% ±0.44%
fs/readfile-promises.jsconcurrent=1len=33554432encoding=''duration=5-0.23 % ±0.37% ±0.49% ±0.64%
fs/readfile-promises.jsconcurrent=1len=33554432encoding='utf-8'duration=50.06 % ±0.21% ±0.28% ±0.37%
fs/readfile-promises.jsconcurrent=1len=4194304encoding=''duration=5-18.61 % ±27.55% ±36.68% ±47.76%
fs/readfile-promises.jsconcurrent=1len=4194304encoding='utf-8'duration=50.03 % ±0.26% ±0.34% ±0.44%
fs/readfile-promises.jsconcurrent=1len=524288encoding=''duration=55.79 % ±6.33% ±8.42% ±10.94%
fs/readfile-promises.jsconcurrent=1len=524288encoding='utf-8'duration=5-0.14 % ±0.49% ±0.65% ±0.84%
fs/readfile-promises.jsconcurrent=1len=8388608encoding=''duration=5-12.68 % ±26.92% ±35.79% ±46.52%
fs/readfile-promises.jsconcurrent=1len=8388608encoding='utf-8'duration=50.05 % ±0.23% ±0.31% ±0.41%
fs/readfile-promises.jsconcurrent=10len=1024encoding=''duration=51.19 % ±1.78% ±2.37% ±3.08%
fs/readfile-promises.jsconcurrent=10len=1024encoding='utf-8'duration=51.47 % ±2.34% ±3.11% ±4.04%
fs/readfile-promises.jsconcurrent=10len=16777216encoding=''duration=5-0.44 % ±0.91% ±1.21% ±1.57%
fs/readfile-promises.jsconcurrent=10len=16777216encoding='utf-8'duration=5*0.21 % ±0.20% ±0.26% ±0.34%
fs/readfile-promises.jsconcurrent=10len=33554432encoding=''duration=50.46 % ±0.77% ±1.03% ±1.34%
fs/readfile-promises.jsconcurrent=10len=33554432encoding='utf-8'duration=5*0.17 % ±0.16% ±0.21% ±0.27%
fs/readfile-promises.jsconcurrent=10len=4194304encoding=''duration=5-1.46 % ±1.72% ±2.29% ±2.98%
fs/readfile-promises.jsconcurrent=10len=4194304encoding='utf-8'duration=50.11 % ±0.32% ±0.43% ±0.56%
fs/readfile-promises.jsconcurrent=10len=524288encoding=''duration=53.15 % ±6.62% ±8.80% ±11.44%
fs/readfile-promises.jsconcurrent=10len=524288encoding='utf-8'duration=5-0.12 % ±0.48% ±0.63% ±0.82%
fs/readfile-promises.jsconcurrent=10len=8388608encoding=''duration=5-0.19 % ±0.99% ±1.32% ±1.72%
fs/readfile-promises.jsconcurrent=10len=8388608encoding='utf-8'duration=5-0.07 % ±0.21% ±0.28% ±0.37%
fs/readfile.jsconcurrent=1len=1024encoding=''duration=5-0.44 % ±1.49% ±1.98% ±2.58%
fs/readfile.jsconcurrent=1len=1024encoding='utf-8'duration=5*-1.77 % ±1.48% ±1.97% ±2.56%
fs/readfile.jsconcurrent=1len=16777216encoding=''duration=5-2.92 % ±29.62% ±39.38% ±51.19%
fs/readfile.jsconcurrent=1len=16777216encoding='utf-8'duration=5-0.13 % ±0.49% ±0.65% ±0.85%
fs/readfile.jsconcurrent=10len=1024encoding=''duration=5*-0.84 % ±0.78% ±1.04% ±1.35%
fs/readfile.jsconcurrent=10len=1024encoding='utf-8'duration=50.10 % ±0.87% ±1.16% ±1.51%
fs/readfile.jsconcurrent=10len=16777216encoding=''duration=5*2.32 % ±1.79% ±2.38% ±3.09%
fs/readfile.jsconcurrent=10len=16777216encoding='utf-8'duration=5-0.06 % ±0.22% ±0.29% ±0.38%
fs/readFileSync.jsn=10000hasFileDescriptor='false'path='existing'encoding='undefined'0.42 % ±0.97% ±1.30% ±1.69%
fs/readFileSync.jsn=10000hasFileDescriptor='false'path='existing'encoding='utf8'-1.26 % ±2.09% ±2.78% ±3.64%
fs/readFileSync.jsn=10000hasFileDescriptor='false'path='non-existing'encoding='undefined'0.13 % ±1.35% ±1.80% ±2.34%
fs/readFileSync.jsn=10000hasFileDescriptor='false'path='non-existing'encoding='utf8'0.10 % ±1.07% ±1.43% ±1.86%
fs/readFileSync.jsn=10000hasFileDescriptor='true'path='existing'encoding='undefined'0.58 % ±2.54% ±3.38% ±4.40%
fs/readFileSync.jsn=10000hasFileDescriptor='true'path='existing'encoding='utf8'-2.21 % ±2.60% ±3.46% ±4.50%
fs/readFileSync.jsn=10000hasFileDescriptor='true'path='non-existing'encoding='undefined'-1.05 % ±2.48% ±3.29% ±4.28%
fs/readFileSync.jsn=10000hasFileDescriptor='true'path='non-existing'encoding='utf8'0.24 % ±2.06% ±2.74% ±3.56%
fs/write-stream-throughput.jssize=1024encodingType='asc'dur=5-8.63 % ±11.00% ±14.63% ±19.02%
fs/write-stream-throughput.jssize=1024encodingType='buf'dur=5-4.67 % ±12.79% ±17.01% ±22.11%
fs/write-stream-throughput.jssize=1024encodingType='utf'dur=5-0.07 % ±0.64% ±0.85% ±1.11%
fs/write-stream-throughput.jssize=1048576encodingType='asc'dur=5-16.13 % ±17.05% ±22.93% ±30.33%
fs/write-stream-throughput.jssize=1048576encodingType='buf'dur=50.08 % ±21.28% ±28.30% ±36.79%
fs/write-stream-throughput.jssize=1048576encodingType='utf'dur=50.13 % ±0.76% ±1.02% ±1.32%
fs/write-stream-throughput.jssize=2encodingType='asc'dur=5-0.14 % ±0.55% ±0.74% ±0.96%
fs/write-stream-throughput.jssize=2encodingType='buf'dur=5-0.04 % ±0.23% ±0.30% ±0.39%
fs/write-stream-throughput.jssize=2encodingType='utf'dur=5-0.17 % ±0.63% ±0.84% ±1.10%
fs/write-stream-throughput.jssize=65535encodingType='asc'dur=5-4.76 % ±6.39% ±8.50% ±11.06%
fs/write-stream-throughput.jssize=65535encodingType='buf'dur=51.00 % ±6.70% ±8.92% ±11.59%
fs/write-stream-throughput.jssize=65535encodingType='utf'dur=50.55 % ±0.76% ±1.02% ±1.32%
fs/writefile-promises.jsconcurrent=1size=1024encodingType='asc'duration=5-0.23 % ±1.16% ±1.54% ±2.01%
fs/writefile-promises.jsconcurrent=1size=1024encodingType='buf'duration=5-0.37 % ±1.53% ±2.03% ±2.65%
fs/writefile-promises.jsconcurrent=1size=1024encodingType='utf'duration=50.18 % ±1.10% ±1.46% ±1.89%
fs/writefile-promises.jsconcurrent=1size=1048576encodingType='asc'duration=515.51 % ±31.87% ±42.38% ±55.11%
fs/writefile-promises.jsconcurrent=1size=1048576encodingType='buf'duration=5-7.46 % ±15.13% ±20.11% ±26.15%
fs/writefile-promises.jsconcurrent=1size=1048576encodingType='utf'duration=5-1.27 % ±1.36% ±1.81% ±2.37%
fs/writefile-promises.jsconcurrent=1size=2encodingType='asc'duration=5-0.67 % ±1.23% ±1.64% ±2.13%
fs/writefile-promises.jsconcurrent=1size=2encodingType='buf'duration=5-0.67 % ±1.53% ±2.03% ±2.64%
fs/writefile-promises.jsconcurrent=1size=2encodingType='utf'duration=5*1.19 % ±1.09% ±1.44% ±1.88%
fs/writefile-promises.jsconcurrent=1size=65535encodingType='asc'duration=5-1.15 % ±15.51% ±20.62% ±26.81%
fs/writefile-promises.jsconcurrent=1size=65535encodingType='buf'duration=5*-11.01 % ±10.43% ±13.88% ±18.07%
fs/writefile-promises.jsconcurrent=1size=65535encodingType='utf'duration=5-0.21 % ±1.22% ±1.63% ±2.12%
fs/writefile-promises.jsconcurrent=10size=1024encodingType='asc'duration=5-0.29 % ±1.21% ±1.61% ±2.10%
fs/writefile-promises.jsconcurrent=10size=1024encodingType='buf'duration=50.56 % ±1.09% ±1.45% ±1.89%
fs/writefile-promises.jsconcurrent=10size=1024encodingType='utf'duration=5-0.13 % ±1.03% ±1.37% ±1.79%
fs/writefile-promises.jsconcurrent=10size=1048576encodingType='asc'duration=5-0.28 % ±2.32% ±3.08% ±4.01%
fs/writefile-promises.jsconcurrent=10size=1048576encodingType='buf'duration=51.13 % ±2.44% ±3.24% ±4.21%
fs/writefile-promises.jsconcurrent=10size=1048576encodingType='utf'duration=5-0.61 % ±1.26% ±1.68% ±2.20%
fs/writefile-promises.jsconcurrent=10size=2encodingType='asc'duration=50.72 % ±1.07% ±1.42% ±1.85%
fs/writefile-promises.jsconcurrent=10size=2encodingType='buf'duration=50.31 % ±1.28% ±1.71% ±2.23%
fs/writefile-promises.jsconcurrent=10size=2encodingType='utf'duration=50.16 % ±1.16% ±1.54% ±2.01%
fs/writefile-promises.jsconcurrent=10size=65535encodingType='asc'duration=50.44 % ±1.60% ±2.12% ±2.76%
fs/writefile-promises.jsconcurrent=10size=65535encodingType='buf'duration=50.24 % ±1.94% ±2.58% ±3.36%
fs/writefile-promises.jsconcurrent=10size=65535encodingType='utf'duration=5**-2.09 % ±1.42% ±1.89% ±2.46%

FWIW 20% was observed only for path.posix.toNamespacedPath() which is literally noop.

@avivkeller

Copy link
Copy Markdown
MemberAuthor

@anonrig, given the benchmark results, do you think a secondary system w/o URL support should be added for the toNamespacedPath function?

@anonrig

Copy link
Copy Markdown
Member

I'm -0.5 since benchmarks seem to have some impact, but I'd like to raise awareness before unblocking.

cc @nodejs/tsc @nodejs/performance wdyt?

@GeoffreyBoothGeoffreyBooth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add an error hint where if someone calls a path method with a string URL (not an URL instance) we suggest that they wrap their input in new URL? Basically if we’re about to throw anyway, if the input started with file: then enrich the error with this suggestion.

Comment threaddoc/api/path.md Outdated
Comment threaddoc/api/path.md Outdated
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Should we add an error hint where if someone calls a path method with a string URL (not an URL instance) we suggest that they wrap their input in new URL? Basically if we’re about to throw anyway, if the input started with file: then enrich the error with this suggestion.

IIRC It's possible for a file path to contain the file: string, so an error like that would be unnecessary

@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2024
@RafaelGSS

Copy link
Copy Markdown
Member

I suspect this will thrown when using permission model

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

@avivkeller

Copy link
Copy Markdown
MemberAuthor

I suspect this will thrown when using permission model

I don't think that'll happen, as it's doing all the changes before any path processing really occurs

@anonriganonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2 referenced issues are 2 years old and 9 months old that didn't receive as much traffic to warrant for this change in an extremely important part of the Node.js core.

I prefer this function to not change the signature of the existing functions, if the request/need for the change is not warranting the cost of touching an extremely hot path. We might expose a different function that we don't rely on (internally), but I'm not sure.

I'm -1 on the proposed change as it is.

cc @nodejs/url

Comment threadtest/parallel/test-path-join.js Outdated

@tniessentniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced that this is a step in the right direction.

For example, isn't the second argument of the URL constructor supposed to effectively do resolve() (with the typical URL caveats)?

On a related note, normalize() has a variety of issues because the resulting path is not necessary equivalent to the original path. Explicitly propagating these quirks to file URLs feels odd to me.

Comment threadlib/path.js
validateString,
} = require('internal/validators');
const {
toPathIfFileURL: URLToPathIfFileURL,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preferably, a variable with camelCasing. I can see its little complex as the existing functions already reserved the names.

However, something like getPathFromFileURL or something similar would be nice to have.

@zthriggerzthrigger left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left open

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-benchmark-ciPR that need a benchmark CI run.needs-ciPRs that need a full CI run.pathIssues and PRs related to the path subsystem.semver-minorPRs that contain new features and should be released in the next minor version.whatwg-urlIssues and PRs related to the WHATWG URL implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

12 participants

@avivkeller@nodejs-github-bot@targos@LiviaMedeiros@anonrig@RafaelGSS@jasnell@GeoffreyBooth@tniessen@MrJithil@zthrigger@aduh95