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

stream: skip chunk validation for Buffers #10580

Merged
merged 1 commit into from
Jan 5, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions benchmark/streams/writable-manywrites.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

const common = require('../common');
const Writable = require('stream').Writable;

const bench = common.createBenchmark(main, {
n: [2e6]
});

function main(conf) {
const n = +conf.n;
const b = Buffer.allocUnsafe(1024);
const s = new Writable();
s._write = function(chunk, encoding, cb) {
cb();
};

bench.start();
for (var k = 0; k < n; ++k) {
s.write(b);
}
bench.end(n);
}
37 changes: 17 additions & 20 deletions lib/_stream_writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,23 +194,18 @@ function writeAfterEnd(stream, cb) {
process.nextTick(cb, er);
}

// If we get something that is not a buffer, string, null, or undefined,
// and we're not in objectMode, then that's an error.
// Otherwise stream chunks are all considered to be of length=1, and the
// watermarks determine how many objects to keep in the buffer, rather than
// how many bytes or characters.
// Checks that a user-supplied chunk is valid, especially for the particular
// mode the stream is in. Currently this means that `null` is never accepted
// and undefined/non-string values are only allowed in object mode.
function validChunk(stream, state, chunk, cb) {
var valid = true;
Copy link
Member

Choose a reason for hiding this comment

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

You should add back some comment here, explaining this change. validChunk  now only checks if a chunk is valid when it is not a 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.

I removed the comments here because they either duplicated what the inline comments already said or had nothing to do with validChunk() ('Otherwise stream chunks ...').

Copy link
Contributor Author

@mscdex mscdex Jan 3, 2017

Choose a reason for hiding this comment

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

Even the inline comments are pretty redundant as they just explain what the simple code below it is doing, not worth keeping IMHO. I have added a new comment above the function definition now.

var er = false;
// Always throw error if a null is written
// if we are not in object mode then throw
// if it is not a buffer, string, or undefined.

if (chunk === null) {
er = new TypeError('May not write null values to stream');
} else if (!(chunk instanceof Buffer) &&
typeof chunk !== 'string' &&
chunk !== undefined &&
!state.objectMode) {
} else if (typeof chunk !== 'string' &&
Copy link
Member

Choose a reason for hiding this comment

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

Would switching to Buffer.isBuffer() have the same performance profile as instanceof? By removing the instanceof Buffer check here, this becomes a semver-major. If we can switch to Buffer.isBuffer() and still get a performance boost, then this can be semver-patch.

Copy link
Member

Choose a reason for hiding this comment

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

@jasnell this PR is not removing the check (see my comment). It is still doing it, just in a different way. I would flag it semver-minor, and it can even be backported to v4 and v6.

Copy link
Contributor Author

@mscdex mscdex Jan 3, 2017

Choose a reason for hiding this comment

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

Buffer.isBuffer() is just an instanceof check, so that would not help. Also, @mcollina is correct, we're just avoiding duplicate instanceofs since it was already being done in write().

Copy link
Member

Choose a reason for hiding this comment

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

Ah.. lol, forgot about that :-)

Copy link
Member

Choose a reason for hiding this comment

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

@mscdex that's my point on the review. "remove duplicate instanceof checks" should be a good commit message/pr title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcollina It's not just that though, for example it would have also checked for null before getting to the second instanceof check. Anyway, I've changed the commit message. Let me know if it's more suitable.

chunk !== undefined &&
!state.objectMode) {
er = new TypeError('Invalid non-string/buffer chunk');
}
if (er) {
Expand All @@ -224,13 +219,14 @@ function validChunk(stream, state, chunk, cb) {
Writable.prototype.write = function(chunk, encoding, cb) {
var state = this._writableState;
var ret = false;
var isBuf = (chunk instanceof Buffer);

if (typeof encoding === 'function') {
cb = encoding;
encoding = null;
}

if (chunk instanceof Buffer)
if (isBuf)
encoding = 'buffer';
else if (!encoding)
encoding = state.defaultEncoding;
Expand All @@ -240,9 +236,9 @@ Writable.prototype.write = function(chunk, encoding, cb) {

if (state.ended)
writeAfterEnd(this, cb);
else if (validChunk(this, state, chunk, cb)) {
else if (isBuf || validChunk(this, state, chunk, cb)) {
state.pendingcb++;
ret = writeOrBuffer(this, state, chunk, encoding, cb);
ret = writeOrBuffer(this, state, isBuf, chunk, encoding, cb);
}

return ret;
Expand Down Expand Up @@ -291,11 +287,12 @@ function decodeChunk(state, chunk, encoding) {
// if we're already writing something, then just put this
// in the queue, and wait our turn. Otherwise, call _write
// If we return false, then we need a drain event, so set that flag.
function writeOrBuffer(stream, state, chunk, encoding, cb) {
chunk = decodeChunk(state, chunk, encoding);

if (chunk instanceof Buffer)
encoding = 'buffer';
function writeOrBuffer(stream, state, isBuf, chunk, encoding, cb) {
if (!isBuf) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@mscdex could chunk have changed before this is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. writeOrBuffer() is only called by write() and validChunk() does not return a modified chunk or anything.

chunk = decodeChunk(state, chunk, encoding);
if (chunk instanceof Buffer)
encoding = 'buffer';
}
var len = state.objectMode ? 1 : chunk.length;

state.length += len;
Expand Down