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: replace SlowBuffer with Buffer.allocUnsafeSlow(size) #5833

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 21, 2016

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 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
@jasnell
Copy link
Member Author

jasnell commented Mar 21, 2016

semver-major because of the docs-only deprecation.


When calling `Buffer.allocUnsafe()`, the segment of allocated memory is
*uninitialized* (it is not zeroed-out). While this design makes the allocation
of memory quite fast, the allocated segment of memory might contain old data
that is potentially sensitive. Using a `Buffer` created by
`Buffer.allocUnsafe(size)` without *completely* overwriting the memory can
allow this old data to be leaked when the `Buffer` memory is read.
`Buffer.allocUnsafe()` without *completely* overwriting the memory can allow
Copy link
Member

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.

@ChALkeR
Copy link
Member

ChALkeR commented Mar 21, 2016

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
Copy link
Member Author

jasnell commented Mar 21, 2016

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 jasnell modified the milestone: 6.0.0 Mar 22, 2016
@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2016

@trevnorris
Copy link
Contributor

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.

@@ -118,12 +118,12 @@ Buffer.alloc = function(size, fill, encoding) {
* Equivalent to Buffer(num), by default creates a non-zero-filled Buffer
* instance. If `--zero-fill-buffers` is set, will zero-fill the buffer.
**/
Buffer.allocUnsafe = function(size) {
Buffer.allocUnsafe = function(size, unpooled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

something like:

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

@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2016

@trevnorris ... added the additional type check.

@trevnorris
Copy link
Contributor

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

@jasnell
Copy link
Member Author

jasnell commented Mar 23, 2016

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

@jasnell
Copy link
Member Author

jasnell commented Mar 24, 2016

@nodejs/collaborators ...

@Fishrock123
Copy link
Contributor

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
Copy link
Member Author

jasnell commented Mar 30, 2016

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 jasnell force-pushed the dep-slow-buffer branch 2 times, most recently from 2b73f9c to 7b885a6 Compare April 1, 2016 19:42
@jasnell
Copy link
Member Author

jasnell commented Apr 1, 2016

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

@ChALkeR
Copy link
Member

ChALkeR commented Apr 1, 2016

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 jasnell force-pushed the dep-slow-buffer branch 2 times, most recently from 0e21486 to 6f6a3e9 Compare April 2, 2016 01:15
@jasnell
Copy link
Member Author

jasnell commented Apr 2, 2016

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

@jasnell
Copy link
Member Author

jasnell commented Apr 3, 2016

@trevnorris @Fishrock123 @ChALkeR ... ping

@jasnell 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
if (typeof size !== 'number')
throw new TypeError('"size" argument must be a number');
return allocate(size);
return ((allocflags & Buffer.F_UNPOOLED) === Buffer.F_UNPOOLED) ?
createBuffer(size) : allocate(size);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is missing the

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

in the createBuffer case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

@jasnell
Copy link
Member Author

jasnell commented Apr 3, 2016

@ChALkeR ... updated! PTAL

jasnell added a commit that referenced this pull request Apr 15, 2016
PR-URL: #5833
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Apr 15, 2016

Landed in 6275249, 3fe204c, and a0579c0

@jasnell jasnell closed this Apr 15, 2016
@ChALkeR ChALkeR mentioned this pull request Apr 19, 2016
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
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>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
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>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
PR-URL: nodejs#5833
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell added a commit that referenced this pull request Apr 26, 2016
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>
jasnell added a commit that referenced this pull request Apr 26, 2016
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>
jasnell added a commit that referenced this pull request Apr 26, 2016
PR-URL: #5833
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@ChALkeR
Copy link
Member

ChALkeR commented May 5, 2016

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

@ChALkeR
Copy link
Member

ChALkeR commented Jun 5, 2016

@jasnell Backport to v5.x in #7169.

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Jul 8, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 8, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants