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

fs: speed up fs.WriteStream#_write #2944

Closed
wants to merge 1 commit into from
Closed

fs: speed up fs.WriteStream#_write #2944

Show file tree
Hide file tree
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
29 changes: 22 additions & 7 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1840,30 +1840,45 @@ fs.FileWriteStream = fs.WriteStream; // support the legacy name


WriteStream.prototype.open = function() {
const self = this;

fs.open(this.path, this.flags, this.mode, function(er, fd) {
Copy link

Choose a reason for hiding this comment

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

latest node supports arrow functions so why not use them?
Ref: http://jsperf.com/arrow-functions-vs-bind
Ref: http://jsperf.com/arrow-vs-bind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's my call to make. I would like to see the Node core members formally embrace it before throwing my warm and fuzzy ES6 feelings into each PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know about "embracing" it, but we're not going to enforce it. It's a stylistic call to save a few chars in a scenario like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just so we're clear on future PRs, are arrow functions permitted in core now? I know they're supported by V8, but is there any reason NOT to use them?

Copy link
Contributor

Choose a reason for hiding this comment

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

They're permitted. Performance is on par w/ normal functions. Only place I'd suggest they be used is if this needs to propagate. Other than that it's nothing more than a style thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to my benchmark, arrow functions actually seem to be reliably faster than regular functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brendanashworth Try reordering the runs. ;)

$ node /tmp/arrow-bench.js 
function took 87.64109233 ns / op       
regular took 93.22382201 ns / op        

$ node /tmp/arrow-bench.js 
regular took 87.63654512 ns / op        
function took 93.23333455 ns / op

Copy link
Contributor

Choose a reason for hiding this comment

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

sigh, v8 is confusing sometimes.

if (er) {
this.destroy();
this.emit('error', er);
self.destroy();
self.emit('error', er);
return;
}

this.fd = fd;
this.emit('open', fd);
}.bind(this));
self.fd = fd;
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a good place to put an arrow function. I'm curious about the perf vs this bind and const self = this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Afaik bind is notoriously slow (both at bind time and at call time), so wherever we can remove it (which is everywhere), we most definitely should. Arrow functions, I don't believe perform quite well enough (or prevents optimization?) to start using all over the place I believe. Perhaps @mraleph can chime in on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wary about them as well, but after a bunch of testing found that they're just as fast as normal functions in the same situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though, bind() is still the devil's tool. Fortunately arrow functions don't play by the same rules, thus aren't placed under the same performance constraints.

self.emit('open', fd);
});
};


function write(fd, buffer, position, callback) {
function wrapper(err, written) {
// Retain a reference to buffer so that it can't be GC'ed too soon.
callback(err, written || 0, buffer);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trevnorris Any news on the solution to these wrappers? If we could avoid a closure for each of these write calls everywhere, I think that would improve performance (or lower memory usage) a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

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

You won't see much if any gain in terms of performance. The change is more for code cleanliness. Will have to make the change of passing buffer to wrapper() so it can be hoisted. Though that shouldn't be difficult.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what the plan is, but are you saying we would still need a wrapper? Perhaps you can elaborate a bit on what this is really all about? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Once the buffer has left JS scope then GC will clean it up. Even though it's still being used. One alternative is that the buffer bubbles through the oncomplete callback. I still need to look into it. While the wrapper is ugly, it doesn't add any noticeable performance overhead in this scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, thanks.


var req = new FSReqWrap();
req.oncomplete = wrapper;
binding.writeBuffer(fd, buffer, 0, buffer.length, position, req);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is what this PR is all about: avoiding all the stuff in fs.write that we already know does not apply to this use-case.

}


WriteStream.prototype._write = function(data, encoding, cb) {
if (!(data instanceof Buffer))
return this.emit('error', new Error('Invalid data'));
return this.emit('error', new TypeError('Invalid data'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this isn't necessary anyways, as Writable guarantees data will be a Buffer. Perhaps, if the C++ throws on not-a-buffer, this could be removed entirely? (As to throw on evil ._write calls)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I think you are right! And C++ does check for this luckily.
That's great, more free perf :) I'll fix this.


if (typeof this.fd !== 'number')
return this.once('open', function() {
this._write(data, encoding, cb);
});

var self = this;
fs.write(this.fd, data, 0, data.length, this.pos, function(er, bytes) {

write(this.fd, data, this.pos, function(er, bytes) {
if (er) {
self.destroy();
return cb(er);
Expand Down
14 changes: 6 additions & 8 deletions test/parallel/test-fs-write-stream-err.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
var common = require('../common');
var assert = require('assert');
var fs = require('fs');
var binding = process.binding('fs');

common.refreshTmpDir();

Expand All @@ -10,21 +11,18 @@ var stream = fs.createWriteStream(common.tmpDir + '/out', {
});
var err = new Error('BAM');

var write = fs.write;
var writeBuffer = binding.writeBuffer;
var writeCalls = 0;
fs.write = function() {
binding.writeBuffer = function() {
switch (writeCalls++) {
case 0:
console.error('first write');
// first time is ok.
return write.apply(fs, arguments);
console.error('first write');
return writeBuffer.apply(this, arguments);
case 1:
// then it breaks
console.error('second write');
var cb = arguments[arguments.length - 1];
return process.nextTick(function() {
cb(err);
});
return process.nextTick(arguments[5].oncomplete, err);
default:
// and should not be called again!
throw new Error('BOOM!');
Expand Down