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

not-an-aardvark · 2016-10-22T02:43:40Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

buffer

Description of change

Previously, the implementation of Buffer.alloc() called Buffer#fill()
with another Buffer as an argument. However, in v4.x, Buffer#fill does
not support a Buffer as an argument. As a workaround, call
binding.fill() directly in the Buffer.alloc() implementation.

Fixes: #9226

Previously, the implementation of Buffer.alloc() called Buffer#fill() with another Buffer as an argument. However, in v4.x, Buffer#fill does not support a Buffer as a parameter. As a workaround, call binding.fill() directly in the Buffer.alloc() implementation. Fixes: nodejs#9226

MylesBorins · 2016-10-22T02:53:08Z

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

/cc @jasnell @nodejs/buffer

mscdex · 2016-10-22T03:06:55Z

test/parallel/test-buffer-alloc.js

+// issue GH-9226
+{
+  const buf = Buffer.alloc(4, 'YQ==', 'base64');
+  const expectedBuf = new Buffer([97, 97, 97, 97]);


Can't this be Buffer.from() instead?

Good idea; I had been under the impression that Buffer.from() had different behavior in v4, but it seems like that's not an issue here.

mscdex · 2016-10-22T03:07:01Z

test/parallel/test-buffer-alloc.js

+}
+{
+  const buf = Buffer.alloc(4, 'ab', 'ascii');
+  const expectedBuf = new Buffer([97, 98, 97, 98]);


mscdex · 2016-10-22T03:07:37Z

test/parallel/test-buffer-alloc.js

@@ -1060,6 +1060,17 @@ assert.throws(function() {
  Buffer.allocUnsafe(0xFFFFFFFFF);
 }, RangeError);

+// issue GH-9226


I think it might be better to provide a URL instead (or at least in addition).

mscdex · 2016-10-22T03:12:52Z

It should be noted that this change will put the function over the default code inlining limit (600 characters). This may not be very desirable since Buffers get used so much, inside and outside of core.

not-an-aardvark · 2016-10-22T03:17:59Z

That's inconvenient. Should I try to reduce the size to under the limit, or extract the added code into a separate private function?

mscdex · 2016-10-22T03:20:26Z

If it's feasible, maybe we can just backport the required fill() change to support Buffers? shrug

Or maybe move both sets of comments before the function?

not-an-aardvark · 2016-10-22T03:22:37Z

I think backporting would be the more elegant fix, but for this PR I wanted to avoid touching the public API at all.

Moving the comments seems like it should work.

mscdex · 2016-10-22T04:27:04Z

lib/buffer.js

+ *
+ * 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.


I know this was copied, but there is a typo here: s/interpretted/interpreted/

mscdex · 2016-10-22T04:29:44Z

test/parallel/test-buffer-alloc.js

@@ -1060,6 +1060,8 @@ assert.throws(function() {
  Buffer.allocUnsafe(0xFFFFFFFFF);
 }, RangeError);

+assert(Buffer.alloc.toString().length < 600, 'Buffer.alloc is not inlineable');


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

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).

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

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.

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.

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.

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.)

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.

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

@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.

MylesBorins · 2016-10-24T15:24:51Z

so do we want to land this or backport the original change?

not-an-aardvark · 2016-10-24T20:11:33Z

If we backport the Buffer.fill change, it will be semver-minor. /cc @nodejs/lts

There is also a slight chance of breakage if someone was confused and passed a buffer to Buffer.fill (undocumented behavior in v4.x), and somehow expected later on that it was zero-filled. It seems unlikely that someone would be depending on this, but it might be worth considering.

I think it would be simpler to just use this fix and not backport the semver-minor change, but I would be fine with either.

MylesBorins · 2016-10-26T17:10:13Z

/cc @nodejs/ctc and @nodejs/lts Are you happy with this landing in the current state?

jasnell · 2016-10-26T17:11:11Z

SGTM

MylesBorins · 2016-10-26T17:12:26Z

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

MylesBorins · 2016-10-26T17:31:11Z

LGTM

MylesBorins · 2016-10-26T17:58:32Z

all failures are infra related

Previously, the implementation of Buffer.alloc() called Buffer#fill() with another Buffer as an argument. However, in v4.x, Buffer#fill does not support a Buffer as a parameter. As a workaround, call binding.fill() directly in the Buffer.alloc() implementation. Fixes: #9226 PR-URL: #9238 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2016-10-26T18:00:59Z

landed in dc3e45f

Previously, the implementation of Buffer.alloc() called Buffer#fill() with another Buffer as an argument. However, in v4.x, Buffer#fill does not support a Buffer as a parameter. As a workaround, call binding.fill() directly in the Buffer.alloc() implementation. Fixes: #9226 PR-URL: #9238 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: James M Snell <[email protected]>

This LTS release comes with 219 commits. This includes 80 commits that are docs related, 58 commits that are test related, 20 commits that are build / tool related, and 9 commits that are updates to dependencies. Notable Changes * build: - It is now possible to build the documentation from the release tarball (Anna Henningsen) #8413 * buffer: - Buffer will no longer incorrectly return a zero filled buffer when an encoding is passed (Teddy Katz) #9238 * deps: - upgrade npm in LTS to 2.15.11 (Kat Marchán) #8928 * repl: - Enable tab completion for global properties (Lance Ball) #7369 * url: - `url.format()` will now encode all `#` in `search` (Ilkka Myller) #8072 PR-URL: #9298

This LTS release comes with 219 commits. This includes 80 commits that are docs related, 58 commits that are test related, 20 commits that are build / tool related, and 9 commits that are updates to dependencies. Notable Changes * build: - It is now possible to build the documentation from the release tarball (Anna Henningsen) #8413 * buffer: - Buffer.alloc() will no longer incorrectly return a zero filled buffer when an encoding is passed (Teddy Katz) #9238 * deps: - upgrade npm in LTS to 2.15.11 (Kat Marchán) #8928 * repl: - Enable tab completion for global properties (Lance Ball) #7369 * url: - `url.format()` will now encode all `#` in `search` (Ilkka Myller) #8072 PR-URL: #9298

This LTS release comes with 219 commits. This includes 80 commits that are docs related, 58 commits that are test related, 20 commits that are build / tool related, and 9 commits that are updates to dependencies. Notable Changes * build: - It is now possible to build the documentation from the release tarball (Anna Henningsen) nodejs/node#8413 * buffer: - Buffer.alloc() will no longer incorrectly return a zero filled buffer when an encoding is passed (Teddy Katz) nodejs/node#9238 * deps: - upgrade npm in LTS to 2.15.11 (Kat Marchan) nodejs/node#8928 * repl: - Enable tab completion for global properties (Lance Ball) nodejs/node#7369 * url: - `url.format()` will now encode all `#` in `search` (Ilkka Myller) nodejs/node#8072 PR-URL: nodejs/node#9298 Signed-off-by: Ilkka Myller <[email protected]>

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. v4.x labels Oct 22, 2016

mscdex reviewed Oct 22, 2016

View reviewed changes

not-an-aardvark added 2 commits October 21, 2016 23:15

squash: use Buffer.from

275b17b

squash: link to GitHub issue in comment

54c9dad

squash: ensure Buffer.alloc is inlineable

90af46a

mscdex reviewed Oct 22, 2016

View reviewed changes

squash: fix comment typo

baad1f6

MylesBorins self-assigned this Oct 24, 2016

MylesBorins added this to the v4.6.2 milestone Oct 24, 2016

addaleax approved these changes Oct 26, 2016

View reviewed changes

MylesBorins approved these changes Oct 26, 2016

View reviewed changes

MylesBorins closed this Oct 26, 2016

not-an-aardvark deleted the buf-alloc-encoding branch October 26, 2016 18:22

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

not-an-aardvark commented Oct 22, 2016

MylesBorins commented Oct 22, 2016

mscdex Oct 22, 2016

not-an-aardvark Oct 22, 2016

mscdex Oct 22, 2016

mscdex Oct 22, 2016

mscdex commented Oct 22, 2016 •

edited

Loading

not-an-aardvark commented Oct 22, 2016

mscdex commented Oct 22, 2016 •

edited

Loading

not-an-aardvark commented Oct 22, 2016 •

edited

Loading

mscdex Oct 22, 2016

mscdex Oct 22, 2016

not-an-aardvark Oct 22, 2016

mscdex Oct 22, 2016 •

edited

Loading

not-an-aardvark Oct 22, 2016

mscdex Oct 22, 2016

cjihrig Oct 22, 2016

not-an-aardvark Oct 23, 2016 •

edited

Loading

RReverser Oct 24, 2016

RReverser Oct 24, 2016

not-an-aardvark Oct 24, 2016

MylesBorins commented Oct 24, 2016

not-an-aardvark commented Oct 24, 2016

MylesBorins commented Oct 26, 2016

jasnell commented Oct 26, 2016

MylesBorins commented Oct 26, 2016

MylesBorins commented Oct 26, 2016

MylesBorins commented Oct 26, 2016

MylesBorins commented Oct 26, 2016

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

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

Conversation

not-an-aardvark commented Oct 22, 2016

Checklist

Affected core subsystem(s)

Description of change

MylesBorins commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Oct 22, 2016 • edited Loading

not-an-aardvark commented Oct 22, 2016

mscdex commented Oct 22, 2016 • edited Loading

not-an-aardvark commented Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark Oct 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Oct 24, 2016

not-an-aardvark commented Oct 24, 2016

MylesBorins commented Oct 26, 2016

jasnell commented Oct 26, 2016

MylesBorins commented Oct 26, 2016

MylesBorins commented Oct 26, 2016

MylesBorins commented Oct 26, 2016

MylesBorins commented Oct 26, 2016

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

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

mscdex commented Oct 22, 2016 •

edited

Loading

mscdex commented Oct 22, 2016 •

edited

Loading

not-an-aardvark commented Oct 22, 2016 •

edited

Loading

mscdex Oct 22, 2016 •

edited

Loading

not-an-aardvark Oct 23, 2016 •

edited

Loading