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

buffer: fix fill with encoding in Buffer.alloc() #9238

Closed
Closed
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
18 changes: 13 additions & 5 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,21 +91,29 @@ Object.setPrototypeOf(Buffer, Uint8Array);
/**
* Creates a new filled Buffer instance.
* alloc(size[, fill[, encoding]])
*
* Only pay attention to encoding if it's a string. This
* prevents accidentally sending in a number that would
* be interpreted as a start offset.
* Also, don't apply encoding if fill is a number.
*
* These comments are placed before the function to keep the text length
* down, to ensure that it remains inlineable by V8.
**/
Buffer.alloc = function(size, fill, encoding) {
if (typeof size !== 'number')
throw new TypeError('"size" argument must be a number');
if (size <= 0)
return createBuffer(size);
if (fill !== undefined) {
// Only pay attention to encoding if it's a string. This
// prevents accidentally sending in a number that would
// be interpretted as a start offset.
// Also, don't apply encoding if fill is a number.
if (typeof fill !== 'number' && typeof encoding === 'string')
fill = Buffer.from(fill, encoding);

return createBuffer(size, true).fill(fill);
const buf = createBuffer(size, true);
// Buffer.prototype.fill does not support filling with other buffers in v4.
// Instead, call binding.fill directly.
binding.fill(buf, fill, 0, buf.length);
return buf;
}
return createBuffer(size);
};
Expand Down
13 changes: 13 additions & 0 deletions test/parallel/test-buffer-alloc.js
Original file line number Diff line number Diff line change
Expand Up @@ -1060,6 +1060,19 @@ assert.throws(function() {
Buffer.allocUnsafe(0xFFFFFFFFF);
}, RangeError);

assert(Buffer.alloc.toString().length < 600, 'Buffer.alloc is not inlineable');
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 not sure about this. There was some discussion awhile back about linting for this, but maybe it would be easier or just as easy to do it this way.

I'm wondering though since it's not really a test per se that we might just put all of these kinds of checks in a separate file where we check the lengths of various commonly used functions? /cc @nodejs/collaborators

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to do this for all functions, then I think linting would be easier. But I would assume we have a lot of longer functions that aren't worth shortening.

For what it's worth, apparently TurboFan doesn't take function length into account when inlining (source).

Copy link
Contributor

@mscdex mscdex Oct 22, 2016

Choose a reason for hiding this comment

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

No, it wouldn't be for all functions, just select ones.

That's interesting about TurboFan, but Crankshaft is still used quite a bit.

EDIT: It looks like that change was reverted shortly after? It was relanded after that heh...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, in that case linting for it might be kind of annoying, because we would have to explicitly state the list of functions somewhere.

Should we move this discussion to a separate issue? In terms of making sure Buffer.alloc can be inlined, I think this test works fine. Having a separate file for inlining checks might be a good idea, but it seems like it's out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, my main concern was about adding this particular instance now. Hopefully we can get some input from other @nodejs/collaborators and see what they think.

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 not opposed to having this somewhere, but I don't think it should be part of our standard test suite. This seems very coupled to V8, and potentially specific versions of V8.

Copy link
Contributor Author

@not-an-aardvark not-an-aardvark Oct 23, 2016

Choose a reason for hiding this comment

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

If we find out later on that function length is no longer a concern, then we can always remove this test. At the moment, though, while we apparently consider it worthwhile to keep commonly-used functions under 600 characters, we're relying on code review to make sure that happens. If it's worth making changes to the standard library's code to fulfill this requirement, I think it's also worth adding a test to make it easier to notice that the function is too long.

(Also, keep in mind that this fix is specific to v4.x, so it seems unlikely that it will make it into a different version of V8.)

Copy link
Member

Choose a reason for hiding this comment

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

Just in case if we want to lint against this, there is a ready ESLint plugin for that https://www.npmjs.com/package/eslint-plugin-v8-func-inline which we could enable for select functions using comment-based rules.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and yes, in TurboFan this should be fixed now - it has limit only on AST complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RReverser I tried out that ESLint plugin, but it seems to check the length of the entire file, rather than the length of a function itself. So if a file has many small functions, they will all get reported.


// https://github.com/nodejs/node/issues/9226
{
const buf = Buffer.alloc(4, 'YQ==', 'base64');
const expectedBuf = Buffer.from([97, 97, 97, 97]);
assert(buf.equals(expectedBuf));
}
{
const buf = Buffer.alloc(4, 'ab', 'ascii');
const expectedBuf = Buffer.from([97, 98, 97, 98]);
assert(buf.equals(expectedBuf));
}

// attempt to overflow buffers, similar to previous bug in array buffers
assert.throws(function() {
Expand Down