Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: refactor realpath with Map and Set #43569

Merged
merged 1 commit into from
Jun 30, 2022
Merged

fs: refactor realpath with Map and Set #43569

merged 1 commit into from
Jun 30, 2022

Conversation

LiviaMedeiros
Copy link
Contributor

Affecting non-native implementations of fs.realpath(path[, options], callback) and fs.realpathSync(path[, options]).

Local benchmark, !isWindows, 256 runs:

                                                     confidence improvement accuracy (*)   (**)  (***)
fs/bench-realpath.js pathType='relative' n=10000            ***     13.34 %       ±2.29% ±3.01% ±3.86%
fs/bench-realpath.js pathType='resolved' n=10000                     1.12 %       ±4.04% ±5.32% ±6.81%
fs/bench-realpathSync.js pathType='relative' n=10000        ***      7.61 %       ±0.33% ±0.44% ±0.56%
fs/bench-realpathSync.js pathType='resolved' n=10000        ***     10.21 %       ±0.37% ±0.49% ±0.63%

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jun 25, 2022
@LiviaMedeiros LiviaMedeiros added request-ci Add this label to start a Jenkins CI on a PR. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. and removed fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jun 25, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 25, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Jun 26, 2022
Open
32 tasks
@aduh95
Copy link
Contributor

aduh95 commented Jun 26, 2022 •
edited by RaisinTen
Loading

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

                                                     confidence improvement accuracy (*)   (**)  (***)
fs/bench-realpath.js pathType='relative' n=10000            ***     11.99 %       ±4.55% ±6.06% ±7.88%
fs/bench-realpath.js pathType='resolved' n=10000                     2.50 %       ±3.29% ±4.38% ±5.70%
fs/bench-realpathSync.js pathType='relative' n=10000          *      5.08 %       ±3.90% ±5.19% ±6.76%
fs/bench-realpathSync.js pathType='resolved' n=10000          *      4.51 %       ±4.24% ±5.65% ±7.37%

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 26, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Jun 27, 2022
Open
33 tasks
jasnell
jasnell reviewed Jun 27, 2022
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Let's make sure we do a CITGM run on this. We've accidentally broken the ecosystem with changes to realpath before.

@LiviaMedeiros LiviaMedeiros added the needs-citgm PRs that need a CITGM CI run. label Jun 27, 2022
@LiviaMedeiros
Copy link
Contributor Author

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2965/

This was referenced Jun 28, 2022
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@F3n67u F3n67u mentioned this pull request Jun 29, 2022
Closed
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 30, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 30, 2022
@nodejs-github-bot nodejs-github-bot merged commit e0705be into nodejs:main Jun 30, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in e0705be

targos pushed a commit that referenced this pull request Jul 12, 2022
@LiviaMedeiros @targos
fs: refactor realpath with Map and Set
12a591a 
PR-URL: #43569
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 20, 2022
@LiviaMedeiros @targos
fs: refactor realpath with Map and Set
d29922a 
PR-URL: #43569
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@LiviaMedeiros @targos
fs: refactor realpath with Map and Set
fcf74c5 
PR-URL: #43569
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@LiviaMedeiros @guangwong
fs: refactor realpath with Map and Set
1b586e4 
PR-URL: nodejs/node#43569
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@aduh95 aduh95 aduh95 approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@LiviaMedeiros @nodejs-github-bot @aduh95 @jasnell @lpinca @BridgeAR @RaisinTen