buffer: replace SlowBuffer with Buffer.allocUnsafeSlow(size) by jasnell · Pull Request #5833 · nodejs/node

jasnell · 2016-03-21T19:54:04Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

buffer

Description of change

Adds Buffer.allocUnsafeSlow() as a replacement for SlowBuffer
Soft-deprecate SlowBuffer

Essentially, this aligns the functionality of SlowBuffer with the new constructor API

Refs: #5799

/cc @ChALkeR @trevnorris

jasnell · 2016-03-21T19:54:28Z

semver-major because of the docs-only deprecation.

ChALkeR · 2016-03-21T19:56:14Z

doc/api/buffer.markdown

There are two spaces in «the memory» for some reason.

ChALkeR · 2016-03-21T20:01:43Z

Hm. Overall looks good, but I'm not sure on the exact syntax.

It seems to me that Buffer.allocUnsafe(10, true) is not very clear to someone who does not use (or see) it often. Pehaps adding an options argument would be better? E.g. Buffer.allocUnsafe(10, {pooled: false})?

jasnell · 2016-03-21T22:10:45Z

The object would make sense if we'd ever expect there to be additional possible options. I don't think that's the case. Also, part of the point for adding it as a second parameter was to avoid making things unnecessarily complex for this case (as opposed to adding a new function, etc). I know it's less obvious what is happening, but I think it's appropriate here. But that's just me. What do you think @trevnorris ?

jasnell · 2016-03-22T18:47:35Z

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

trevnorris · 2016-03-22T19:40:24Z

Could also have Buffer.allocUnsafe(n, kUnpooled) as a bitwise flag. :P

Without writing any benchmarks, I think passing in the extra object isn't going to add much if any noticeable overhead. Though after watching the Buffer API evolve over the last several years I can't foresee needing to pass any other options. Possibly use the boolean, but also strict check that the argument is a boolean instead of coercing it.

trevnorris · 2016-03-22T19:42:59Z

lib/buffer.js

something like:

if (unpooled !== undefined && typeof unpooled !== 'boolean') throw new TypeError('unpooled must be a boolean');

jasnell · 2016-03-22T20:10:25Z

@trevnorris ... added the additional type check.

trevnorris · 2016-03-23T06:07:42Z

Thanks. I'm cool with the new API, but would also like other's opinions.

jasnell · 2016-03-23T16:12:08Z

@nodejs/ctc ... please weigh in :-)

jasnell · 2016-03-24T22:35:38Z

@nodejs/collaborators ...

Fishrock123 · 2016-03-29T14:36:12Z

I think I'd prefer the bitwise flag idea.

Pros:

more explicit
extensible if we need other booleans in the future

Cons:

bitwise flag

I'm not so for adding [, true] to the buffer apis, it's not very easy to tell what that is. Also, I can't imagine many people are actually using SlowBuffer.

@ChALkeR could you test npm to try to see who might be using this?

jasnell · 2016-03-30T14:05:59Z

A bitwise flag sounds like a good compromise on this. I can't imagine that we'd actually add any more flags to this but you never know.

jasnell · 2016-04-01T19:42:35Z

@Fishrock123 @trevnorris ... switched to using a Bitwise field. PTAL

ChALkeR · 2016-04-01T21:23:36Z

This approach looks better to me =).

@Fishrock123

@ChALkeR could you test npm to try to see who might be using this?

Using SlowBuffer or Buffer.allocUnsafe? The former won't be affected by this, and the latter isn't used yet because it just recently landed to master.

SlowBuffer( usage: https://gist.github.com/ChALkeR/43f3376f46b3d71e1e62ef37f643fb06

Note that this doesn't include on purpose pure instanceof SlowBuffer (which is broken btw) and SlowBuffer method redifinitions (there were some, copied in various modules, they mostly add some methods from Buffer to SlowBuffer for older Node.js versions).

Note: mocha has its own SlowBuffer that is not related to Node.js SlowBuffer, ignore that match. It affects many lines in the grep result.

jasnell · 2016-04-02T01:16:09Z

Rebased, squashed. New CI: https://ci.nodejs.org/job/node-test-pull-request/2131/

jasnell · 2016-04-03T18:27:13Z

@trevnorris @Fishrock123 @ChALkeR ... ping

ChALkeR · 2016-04-03T18:54:10Z

lib/buffer.js

It looks like this is missing the

if (size > 0) flags[kNoZeroFill] = 1;

in the createBuffer case.

Good catch.

jasnell · 2016-04-03T20:12:43Z

@ChALkeR ... updated! PTAL

PR-URL: #5833 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

jasnell · 2016-04-15T17:37:27Z

Landed in 6275249, 3fe204c, and a0579c0

Aligns the functionality of SlowBuffer with the new Buffer constructor API. Next step is to docs-only deprecate SlowBuffer. Replace the internal uses of SlowBuffer with `Buffer.allocUnsafeSlow(size)` PR-URL: nodejs#5833 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

With the addition of `Buffer.allocUnsafeSlow(size)` `SlowBuffer` can be deprecated... but docs-only for now. PR-URL: nodejs#5833 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

PR-URL: nodejs#5833 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Aligns the functionality of SlowBuffer with the new Buffer constructor API. Next step is to docs-only deprecate SlowBuffer. Replace the internal uses of SlowBuffer with `Buffer.allocUnsafeSlow(size)` PR-URL: #5833 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

With the addition of `Buffer.allocUnsafeSlow(size)` `SlowBuffer` can be deprecated... but docs-only for now. PR-URL: #5833 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

PR-URL: #5833 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

ChALkeR · 2016-05-05T17:47:33Z

Note that this change wasn't backported to 5.x, unlike all the other new Buffer API methods, which were backported in #5763.

ChALkeR · 2016-06-05T23:25:29Z

@jasnell Backport to v5.x in #7169.

This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`, `Buffer.from()`, and `Buffer.allocUnsafeSlow()` APIs for v4. Some backported tests are disabled, but those are not related to the new API. Note that `Buffer.from(arrayBuffer[, byteOffset [, length]])` is not supported in v4.x, only `Buffer.from(arrayBuffer)` is. Refs: nodejs#4682 Refs: nodejs#5833 Refs: nodejs#7475 PR-URL: nodejs#7562 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`, `Buffer.from()`, and `Buffer.allocUnsafeSlow()` APIs for v4. Some backported tests are disabled, but those are not related to the new API. Note that `Buffer.from(arrayBuffer[, byteOffset [, length]])` is not supported in v4.x, only `Buffer.from(arrayBuffer)` is. Refs: #4682 Refs: #5833 Refs: #7475 PR-URL: #7562 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

jasnell added buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 21, 2016

ChALkeR reviewed Mar 21, 2016
View reviewed changes

doc/api/buffer.markdown Outdated

Copy link

Member

ChALkeR Mar 21, 2016

Choose a reason for hiding this comment

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

There are two spaces in «the memory» for some reason.

jasnell modified the milestone: 6.0.0 Mar 22, 2016

trevnorris reviewed Mar 22, 2016
View reviewed changes

jasnell force-pushed the dep-slow-buffer branch from 6343f1d to 15a09d0 Compare March 22, 2016 20:09

jasnell force-pushed the dep-slow-buffer branch 2 times, most recently from 2b73f9c to 7b885a6 Compare April 1, 2016 19:42

jasnell force-pushed the dep-slow-buffer branch 2 times, most recently from 0e21486 to 6f6a3e9 Compare April 2, 2016 01:15

jasnell changed the title ~~buffer: replace SlowBuffer with Buffer.allocUnsafe(size, true)~~ buffer: replace SlowBuffer with Buffer.allocUnsafe(size, Buffer.F_UNPOOLED) Apr 3, 2016

ChALkeR reviewed Apr 3, 2016
View reviewed changes

jasnell force-pushed the dep-slow-buffer branch from bd2c406 to 2ffad68 Compare April 3, 2016 20:04

jasnell closed this Apr 15, 2016

ChALkeR mentioned this pull request Apr 19, 2016

What about SlowBuffer? #5799

Closed

vitaly-t mentioned this pull request Apr 27, 2016

Doesn't work under Node.js 6.0.0 brianc/node-postgres#1000

Closed

ChALkeR mentioned this pull request May 5, 2016

buffer: backport new buffer constructor APIs #5763

Closed

4 tasks

addaleax mentioned this pull request May 29, 2016

buffer: ignore negative allocation lengths #7051

Merged

3 tasks

ChALkeR mentioned this pull request Jun 5, 2016

Backport important Buffer changes to v5.x #7169

Merged

Uh oh!

Conversation

jasnell commented Mar 21, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

ChALkeR Mar 21, 2016

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented Mar 21, 2016

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

trevnorris commented Mar 22, 2016

Uh oh!

trevnorris Mar 22, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

trevnorris commented Mar 23, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

jasnell commented Mar 24, 2016

Uh oh!

Fishrock123 commented Mar 29, 2016

Uh oh!

jasnell commented Mar 30, 2016

Uh oh!

jasnell commented Apr 1, 2016

Uh oh!

ChALkeR commented Apr 1, 2016

Uh oh!

jasnell commented Apr 2, 2016

Uh oh!

jasnell commented Apr 3, 2016

Uh oh!

ChALkeR Apr 3, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Apr 3, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 3, 2016

Uh oh!

jasnell commented Apr 15, 2016

Uh oh!

ChALkeR commented May 5, 2016

Uh oh!

ChALkeR commented Jun 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants