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: two minor optimizations #14055

Merged
merged 1 commit into from
Jul 9, 2017
Merged

fs: two minor optimizations #14055

merged 1 commit into from
Jul 9, 2017

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jul 3, 2017

  1. If the function in tryStatSync threw, it would never reach that code path to return any value.
  2. Only use try catch if necessary. lchmodSync does not need the second try catch in most cases.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jul 3, 2017
TimothyGu
TimothyGu reviewed Jul 4, 2017
View reviewed changes
lib/fs.js
@@ -552,10 +551,11 @@ fs.readFileSync = function(path, options) {
var isUserFd = isFd(path); // file descriptor ownership
var fd = isUserFd ? path : fs.openSync(path, options.flag || 'r', 0o666);

tryStatSync(fd, isUserFd);
Copy link
Member

Choose a reason for hiding this comment

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

Now that TF+I are in, can we start inlining all of these try* functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

At least in a recently run benchmark (v8 5.8) it still seemed like it's a tiny bit better to keep them around. I suggest to wait until 6.0 has landed and if it's good to inline them, I'd make a single PR for all of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack, since it's an patch level enhancement that should land in v8.x, although it will probably be relevant for 1 minor node8 release.

lpinca
lpinca reviewed Jul 4, 2017
View reviewed changes
lib/fs.js
@@ -520,7 +520,6 @@ function tryStatSync(fd, isUserFd) {
} finally {
Copy link
Member

Choose a reason for hiding this comment

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

I guess it was written like this for performance. If not, isn't cleaner to use catch and get rid of the threw variable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am about to open a separate PR that deals with lots of those :D

Copy link
Member

Choose a reason for hiding this comment

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

Get ready to run benchmarks then :) as I remember that some of these changes (finally -> catch) were rejected in the past. I think it was on timers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Out of my perspective the main issue is that the error has to be thrown again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, when running the benchmark it does not seem to be a significant change, so it would be more churn than anything else.

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR please note that the performance profile of try/catch/finally changed quite a bit since the code was added most likely :)

Copy link
Member Author

@BridgeAR BridgeAR Jul 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Of course but I ran the benchmarks with changing these to try catch and it did not show changed numbers. Therefore I think it's mainly churn to change it.

refack
refack approved these changes Jul 8, 2017
View reviewed changes
lib/fs.js
@@ -552,10 +551,11 @@ fs.readFileSync = function(path, options) {
var isUserFd = isFd(path); // file descriptor ownership
var fd = isUserFd ? path : fs.openSync(path, options.flag || 'r', 0o666);

tryStatSync(fd, isUserFd);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ack, since it's an patch level enhancement that should land in v8.x, although it will probably be relevant for 1 minor node8 release.

@refack
Copy link
Contributor

refack commented Jul 8, 2017

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

@refack refack self-assigned this Jul 8, 2017
refack
refack reviewed Jul 8, 2017
View reviewed changes
lib/fs.js
@@ -552,10 +551,11 @@ fs.readFileSync = function(path, options) {
var isUserFd = isFd(path); // file descriptor ownership
var fd = isUserFd ? path : fs.openSync(path, options.flag || 'r', 0o666);

tryStatSync(fd, isUserFd);
// Use stats array directly to avoid creating an fs.Stats instance just for
// our internal use.
var size;
Copy link
Contributor

Choose a reason for hiding this comment

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

does a trinary with assign to const perform better?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is not really a measurable difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷‍♂️ never mind then

@BridgeAR @refack
fs: two minor optimizations
1806952 
* tryStatSync should not return any value
  If the function threw, it would never reach that code path.

* only use try catch if necessary
  lchmodSync does not need the second try catch in most cases.

PR-URL: nodejs#14055
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack refack merged commit 1806952 into nodejs:master Jul 9, 2017
@refack
Copy link
Contributor

refack commented Jul 9, 2017

Extra quick sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/7168/

addaleax pushed a commit that referenced this pull request Jul 11, 2017
@BridgeAR @addaleax
fs: two minor optimizations
54c4ead 
* tryStatSync should not return any value
  If the function threw, it would never reach that code path.

* only use try catch if necessary
  lchmodSync does not need the second try catch in most cases.

PR-URL: #14055
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Jul 11, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@BridgeAR @addaleax
fs: two minor optimizations
2a02868 
* tryStatSync should not return any value
  If the function threw, it would never reach that code path.

* only use try catch if necessary
  lchmodSync does not need the second try catch in most cases.

PR-URL: #14055
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@BridgeAR @Fishrock123
fs: two minor optimizations
0a462fb 
* tryStatSync should not return any value
  If the function threw, it would never reach that code path.

* only use try catch if necessary
  lchmodSync does not need the second try catch in most cases.

PR-URL: #14055
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@MylesBorins
Copy link
Contributor

ping re: v6.x

@BridgeAR
Copy link
Member Author

I will open a PR to backport this soon.

@MylesBorins
Copy link
Contributor

ping @BridgeAR

@BridgeAR
Copy link
Member Author

When looking at it again there is little benefit for backporting, so I just changed the label accordingly.

@MylesBorins MylesBorins mentioned this pull request Feb 27, 2018
Closed
2 tasks
@refack refack removed their assignment Oct 12, 2018
@BridgeAR BridgeAR deleted the micro-optimize-fs branch April 1, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@refack refack refack approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BridgeAR @refack @MylesBorins @benjamingr @lpinca @TimothyGu @nodejs-github-bot