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.

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

fs: refactor to use private fields #36443

Closed
wants to merge 1 commit into from

Conversation

Lxxyx
Copy link
Member

@Lxxyx Lxxyx commented Dec 8, 2020

Current thoughts are personal, feel free to point out problems if inappropriate!

Refactor lib/internal/fs/dir.js and lib/internal/fs/util.js to use private fields instead of symbols.

Reason:

  • Symbol used to emulate Private Fields (Using global symbols), but now Private Fields are available (proposal-class-fields#Private fields).
  • Symbol makes the code not easy to understand. We need to jump around between uses and definitions.
  • Symbols are used in Node.js in two ways.
    1. to implement private variables only.
    2. for variables that may be used by internal code, but do not want to be modified externally.

When I see a Symbol, I need to look at the context to determine if the variable is just private or shared by internal. I think Symbol only do option 2 in the project, which will make the code easier to read.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Dec 8, 2020
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

One of the reasons for not using private fields yet is performance. They're still not as fast as normal properties or even Symbols:

                                                                              confidence improvement accuracy (*)    (**)   (***)
fs/bench-opendir.js bufferSize=1024 mode='callback' dir='test/parallel' n=100        ***    -11.08 %       ±3.95%  ±5.26%  ±6.86%
fs/bench-opendir.js bufferSize=1024 mode='sync' dir='test/parallel' n=100            ***    -11.78 %       ±4.02%  ±5.41%  ±7.16%
fs/bench-opendir.js bufferSize=32 mode='callback' dir='test/parallel' n=100          ***    -11.43 %       ±5.15%  ±6.86%  ±8.93%
fs/bench-opendir.js bufferSize=32 mode='sync' dir='test/parallel' n=100              ***    -15.40 %       ±0.97%  ±1.29%  ±1.68%
fs/bench-opendir.js bufferSize=4 mode='callback' dir='test/parallel' n=100           ***     -6.42 %       ±3.65%  ±4.86%  ±6.33%
fs/bench-opendir.js bufferSize=4 mode='sync' dir='test/parallel' n=100               ***    -14.61 %       ±1.09%  ±1.45%  ±1.90%
                                                                       confidence improvement accuracy (*)   (**)   (***)
fs/bench-readdir.js withFileTypes='true' dir='test/parallel' n=10             ***    -12.21 %       ±2.64% ±3.52%  ±4.58%
fs/bench-readdirSync.js withFileTypes='true' dir='lib' n=10                   ***    -13.28 %       ±5.36% ±7.14%  ±9.33%
fs/bench-readdirSync.js withFileTypes='true' dir='test/parallel' n=10         ***     -9.81 %       ±3.70% ±4.92%  ±6.40%

@Lxxyx
Copy link
Member Author

Lxxyx commented Dec 8, 2020

One of the reasons for not using private fields yet is performance. They're still not as fast as normal properties or even Symbols:

                                                                              confidence improvement accuracy (*)    (**)   (***)
fs/bench-opendir.js bufferSize=1024 mode='callback' dir='test/parallel' n=100        ***    -11.08 %       ±3.95%  ±5.26%  ±6.86%
fs/bench-opendir.js bufferSize=1024 mode='sync' dir='test/parallel' n=100            ***    -11.78 %       ±4.02%  ±5.41%  ±7.16%
fs/bench-opendir.js bufferSize=32 mode='callback' dir='test/parallel' n=100          ***    -11.43 %       ±5.15%  ±6.86%  ±8.93%
fs/bench-opendir.js bufferSize=32 mode='sync' dir='test/parallel' n=100              ***    -15.40 %       ±0.97%  ±1.29%  ±1.68%
fs/bench-opendir.js bufferSize=4 mode='callback' dir='test/parallel' n=100           ***     -6.42 %       ±3.65%  ±4.86%  ±6.33%
fs/bench-opendir.js bufferSize=4 mode='sync' dir='test/parallel' n=100               ***    -14.61 %       ±1.09%  ±1.45%  ±1.90%
                                                                       confidence improvement accuracy (*)   (**)   (***)
fs/bench-readdir.js withFileTypes='true' dir='test/parallel' n=10             ***    -12.21 %       ±2.64% ±3.52%  ±4.58%
fs/bench-readdirSync.js withFileTypes='true' dir='lib' n=10                   ***    -13.28 %       ±5.36% ±7.14%  ±9.33%
fs/bench-readdirSync.js withFileTypes='true' dir='test/parallel' n=10         ***     -9.81 %       ±3.70% ±4.92%  ±6.40%

Thanks for the reply, I hadn't noticed the performance issue before, maybe we need to wait for the improvement from V8.

@Lxxyx Lxxyx closed this Dec 8, 2020
@Lxxyx Lxxyx deleted the refactor-fs-private-fields branch December 8, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants