FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: two minor optimizations by BridgeAR · Pull Request #14055 · nodejs/node · GitHub

/ node Public

fs: two minor optimizations - #14055

Merged
refack merged 1 commit into
nodejs:masterfrom
BridgeAR:micro-optimize-fs
Jul 9, 2017
ConversationCommits (1)ChecksFiles changed
Merged

fs: two minor optimizations#14055
refack merged 1 commit into
nodejs:masterfrom
BridgeAR:micro-optimize-fs

Conversation

BridgeAR commented Jul 3, 2017

Copy link
Copy Markdown
Member
  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 added the fs Issues and PRs related to the fs subsystem / file system. label Jul 3, 2017
TimothyGu reviewed Jul 4, 2017
View reviewed changes
Comment thread lib/fs.js Outdated

TimothyGu Jul 4, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

BridgeAR Jul 4, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

refack Jul 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 reviewed Jul 4, 2017
View reviewed changes
Comment thread lib/fs.js Outdated

lpinca Jul 4, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

BridgeAR Jul 4, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lpinca Jul 4, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

BridgeAR Jul 4, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

BridgeAR Jul 4, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

benjamingr Jul 6, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

BridgeAR Jul 8, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 approved these changes Jul 8, 2017
View reviewed changes
Comment thread lib/fs.js Outdated

refack Jul 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jul 8, 2017

Copy link
Copy Markdown
Contributor

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

refack self-assigned this Jul 8, 2017
refack reviewed Jul 8, 2017
View reviewed changes
Comment thread lib/fs.js Outdated

refack Jul 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

does a trinary with assign to const perform better?

BridgeAR Jul 9, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is not really a measurable difference.

refack Jul 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

🤷‍♂️ never mind then

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 force-pushed the micro-optimize-fs branch from ac7bd7c to 1806952 Compare July 9, 2017 17:42
refack merged commit 1806952 into nodejs:master Jul 9, 2017

refack commented Jul 9, 2017

Copy link
Copy Markdown
Contributor

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
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 mentioned this pull request Jul 11, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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
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 commented Aug 16, 2017

Copy link
Copy Markdown
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 commented Sep 19, 2017

Copy link
Copy Markdown
Contributor

ping re: v6.x

BridgeAR commented Sep 19, 2017

Copy link
Copy Markdown
Member Author

I will open a PR to backport this soon.

MylesBorins commented Nov 14, 2017

Copy link
Copy Markdown
Contributor

ping @BridgeAR

BridgeAR commented Dec 11, 2017

Copy link
Copy Markdown
Member Author

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

MylesBorins mentioned this pull request Feb 27, 2018
Closed
2 tasks
refack removed their assignment Oct 12, 2018
BridgeAR deleted the micro-optimize-fs branch April 1, 2019 23:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
+2 more reviewers
refack refack approved these changes
TimothyGu TimothyGu left review comments
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL