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
+11 −13
Merged

fs: two minor optimizations #14055

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,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.

if (threw && !isUserFd) fs.closeSync(fd);
}
return !threw;
}

function tryCreateBuffer(size, fd, isUserFd) {
Expand Down Expand Up @@ -553,10 +552,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.

// 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

if (tryStatSync(fd, isUserFd) && (statValues[1/*mode*/] & S_IFMT) === S_IFREG)
if ((statValues[1/*mode*/] & S_IFMT) === S_IFREG)
size = statValues[8/*size*/];
else
size = 0;
Expand Down Expand Up @@ -1085,7 +1085,7 @@ if (constants.O_SYMLINK !== undefined) {
callback(err);
return;
}
// prefer to return the chmod error, if one occurs,
// Prefer to return the chmod error, if one occurs,
// but still try to close, and report closing errors if they occur.
fs.fchmod(fd, mode, function(err) {
fs.close(fd, function(err2) {
Expand All @@ -1098,20 +1098,18 @@ if (constants.O_SYMLINK !== undefined) {
fs.lchmodSync = function(path, mode) {
var fd = fs.openSync(path, constants.O_WRONLY | constants.O_SYMLINK);

// prefer to return the chmod error, if one occurs,
// Prefer to return the chmod error, if one occurs,
// but still try to close, and report closing errors if they occur.
var err, err2, ret;
var ret;
try {
ret = fs.fchmodSync(fd, mode);
} catch (er) {
err = er;
}
try {
fs.closeSync(fd);
} catch (er) {
err2 = er;
} catch (err) {
try {
fs.closeSync(fd);
} catch (ignore) {}
throw err;
}
if (err || err2) throw (err || err2);
fs.closeSync(fd);
return ret;
};
}
Expand Down