buffer: add Buffer.from(), Buffer.alloc() and Buffer.allocUnsafe(), soft-deprecate Buffer(num) by jasnell · Pull Request #4682 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation231 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Node-EPS: nodejs/node-eps#7
Ref: #4660,

It soft deprecates Buffer()
It adds Buffer.allocUnsafe(size) as a direct replacement for Buffer(number)
It adds Buffer.alloc(size[, fill[, encoding]]) to create a new initialized Buffer.
It adds Buffer.from(data[, encoding) as a direct replacement for Buffer(data[, encoding]) (for every variant)
It adds a --zero-fill-buffers command line flag that forces Buffer(number), Buffer.alloc(), and SlowBuffer(number) to zero fill the initialized Buffers.
It adds documentation, test cases and new benchmarks for each of these.
It modifies every existing call to Buffer() currently in core in order to demonstrate the extent of the changes required by these edits. It's not a small change by any measure.

Note: PR description updated to reflect new naming and implementation details

jasnell added wip

Issues and PRs that are still a work in progress.

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

Jan 14, 2016

+1 on the current approach, but can we please find another name? safe and unsafe might be immediately clear in the context of the current issue, but imagine someone who is unfamiliar reading the docs. How about something like

Buffer.allocate(num[, unsafe])

where unsafe is a boolean. Maybe even find a better name for the argument.

-1 on Buffer.allocate(num[, unsafe])

Part of the motivation for this change is to make the choice between using
a prefilled buffer or not more explicit and obvious. We don't achieve that
using a single method with an optional Boolean.

We can bikeshed the names but safe and unsafe are concise and to the
point.
On Jan 13, 2016 9:57 PM, "silverwind" notifications@github.com wrote:

+1 on the approach, but can we please find another name? safe and unsafe
might be immediately clear in the context of the current issue, but imagine
someone fresh reading the docs. How about something like

Buffer.allocate(num[, unsafe])

where unsafe is a boolean.

—
Reply to this email directly or view it on GitHub
#4682 (comment).

@jasnell I might miss the point of this a bit, but just to be sure:

This commit features hard deprecation and does not touch the docs at all. The proposed solution was soft (doc only) deprecation at first.
The deprecation message tells people to use «Buffer.safe or Buffer.unsafe» with no warnings against the latter. The latter should be either removed from this notice or reworded, see Buffer(number) is unsafe #4660 (comment) and Buffer(number) is unsafe #4660 (comment).
Buffer.safe(number) probably needs new tests, including those that make sure that it's zero-filled.

Just a quick note/question: with this approach a safe Buffer would be basically built with malloc + memset, right? (I'm looking here)

In order to make safe buffers faster it might be a good idea to use calloc, since the zeroing could essentially be free in some cases. Repeating the benchmarks @mscdex did afterwards would be nice.

Related question: is there a reason why the ArrayBuffer here gets the data allocated right above instead of just passing the size and letting it use its constructor which would already use calloc if the zero-fill flag was set? (see here).

PS: I just glanced throught the code out of curiosity, I might be totally off-base here. Sorry for the noise if that is the case.

@silverwind
-1 on Buffer.allocate(num[, unsafe]).

Several reasons:

That is a bit harder to grep and to automatically find all the places where you (or someone else) is using unitialized Buffers.
That flag changes the behaviour of a function in an unobvious way (I have seen people failing to understand all impications of using unitialized buffers).
The names for these functions should better be self-describing.
Merging two different methods with different behaviour into a one that changes it's behaviour depending on a flag isn't a good design move.
(Pretty important). The documentation on those methods should be separate, and it's very important. While Buffer.safe could be documented as usually, Buffer.unsafe documentation should describe all the dangers of using unitialized Buffers.

Once more notice: as it looks now, Buffer.safe completely ignores the pooled behaviour. What impications would that have on performance of Buffer.safe(number) vs Buffer.unsafe(number).fill(0)?

/cc @trevnorris

if someone wants to quickly do some benchmarks for us, you can grab N|Solid and run the buffer benchmarks. It's based on the latest v4.x and comes with a "policies" feature that you can use to zero-fill by default—it switches out the malloc with a calloc when enabled, see: https://docs.nodesource.com/nsolid/1.2/docs/policies. The performance profile for this would be very close to what we'd end up with by doing this by default in core because it's basically the same thing.

@ChALkeR ... This PR is still a work in process so some pieces are still missing or not quite complete. It's intended to facilitate more constructive conversation about what changes we need to make. I fully intend to iterate on it more and add the necessary docs.

@saghul ... with the approach currently implemented by this PR, calling Buffer.safe() allocates a new Buffer using calloc (see https://github.com/nodejs/node/pull/4682/files#diff-196d056a936b6d2649721eb639e0442bR90). Calling createBuffer() with flags[kNoZeroFill] set to 0 will cause the ArrayBufferAllocator::Allocate() function in node.cc to use calloc(size,1) as opposed to malloc.

Currently, when Node starts up, a large buffer pool is created. New Buffer instances created using Buffer(num) are allocated off that pool and the memory is NOT zeroed out each time. Calling Buffer.safe() bypasses this pool and ensures that a new zero-filled allocation is created each time. You don't get the performance benefits of the pool but it's safer overall. And calling calloc should be a bit faster than calling fill(0) after the memory is already allocated, but that needs to be benchmarked (I'm sure @trevnorris could say for sure).

I'll be looking for ways of making this more efficient but given that the intent is to provide a more obvious way of creating "safe" buffers, going straight to calloc each time likely makes the most sense. We'll see tho.

Looking at the implementation for the --safe-buffers command line flag now... there are a couple of design questions. The intent of --safe-buffers is to force all newly created Buffers to be zero filled automatically. Given that, there are three approaches I can take with this:

Still use the slab buffer pool and fill(0) all Buffers after they are created. This works ok for smallish Buffers but could have significant performance impact on large buffers.
Skip the slab buffer pool and simply calloc all new Buffers. This is more efficient than calling fill(0) but means we don't enjoy the benefits of the slab allocated pool.
Use a hybrid approach... for Buffers under a certain size, we allocate off the pool and fill(0), but for Buffers over that size we fall back to calloc.

Regardless of the option chosen, --safe-buffers is going to cause a significant performance degradation, it's just a matter of figuring out how to minimize it.

@jasnell Btw, about --safe-buffers — I don't like that name. Buffers are generally safe, careless (or accidential) usage of Buffer(number) isn't. Also, that flag probably should be there only until Buffer(number) is hard deprecated (or removed). Perhaps giving it a more explicit name, like --safe-buffer-from-number or --safe-buffer-number would be better.

@jasnell About the Buffer(number) speed under the flag:

Buffer.safe(number) should try best to be fast with various number values. That could involve calloc-ing for all number values, or callocing for large number values and using a pooled approach (like Buffer.unsafe(number).fill(0)) for smaller number values, whichever is faster.
Buffer(number) under the --safe-buffer-number flag (or however it would be called) should behave exactly like Buffer.safe(number) and reuse that code. That will guarantee that it's as fast as Buffer(number) is and minimize the codebase, lowering the probability of introducing bugs.

@trevnorris @silverwind @saghul @ChALkeR ... ok, pushed an update:

adds documentation
adds SlowBuffer.safe() and SlowBuffer.unsafe() also, deprecates SlowBuffer(size)
changes the impl of Buffer.safe(len) to simply call Buffer.unsafe(len).fill(0), benchmarks are showing that it's still faster than doing the calloc directly every time.
expands benchmark/buffers/buffer-creation.js to include the safe and unsafe options and larger buffer sizes
updates test cases and internal code bits to use SlowBuffer.unsafe.

Still todo: add the --safe-buffer command line flag... per @ChALkeR's comments I'll likely rename it to --zero-fill-buffers

This is moving a bit quick, but some general things:

If we are going to do this, Buffer(number) should be deprecated, probably first only in docs. I prefer runtime deprecations, since you can ignore them --no-deprecation, but I understand some users don't.
safe() and unsafe() are not actually that descriptive. These sound like you are turning something on or off. I suggest going for allocSafe() and allocUnsafe().
--zero-fill-buffers is a better naming, yes.

You mentioned the pooling aspect. Would it be worthwhile to have a safe pool, or does that defeat the purpose?

@jasnell Could you make sure to split up these commits, one replacing usage in core should be separate as it is quite large and makes this difficult to review.

@Fishrock123 ... I have absolutely no intention of landing this any time soon so there shouldn't be a worry about it moving too quickly :-) ... the intent right now is to get concrete options in the form of working code on the table for discussion as opposed to going around and around discussing whether or not it's a bug or not.

oh, another point, having a safe pool likely isn't worthwhile because you'd end up having to zero fill every time anyway to reset from the last time :-) An argument could be made that if --zero-fill-buffers is set, the pool should be zero filled on creation but given that we end up having to zero fill later anyway, it's a bit redundant and wouldn't buy us anything.

Also, the current implementation of Buffer.safe() simply does a Buffer.unsafe().fill(), which shouldn't be more efficient that simply doing a calloc but benchmarks are showing that it is... which baffles me a bit. I'd like to get that figured out.

@Fishrock123 ... I reswizzled the commits to make it easier to review.

Pushed a new commit to use the names zalloc() and alloc() instead

jasnell changed the title~~buffer: add Buffer.safe() and Buffer.unsafe(), deprecate Buffer(num)~~ buffer: add Buffer.zalloc() and Buffer.alloc(), deprecate Buffer(num)

Jan 14, 2016

zalloc == 'zeroed-allocation'

jasnell added a commit that referenced this pull request

Apr 26, 2016

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release.

Buffer
- New Buffer constructors have been added #4682
- Previously deprecated Buffer APIs are removed #5048, #4594
- Improved error handling #4514
Cluster
- Worker emitted as first argument in 'message' event #5361.
Crypto
- Improved error handling #3100, #5611
- Simplified Certificate class bindings #5382
- Improved control over FIPS mode #5181
- pbkdf2 digest overloading is deprecated #4047
Dependencies
- Reintroduce shared c-ares build support #5775.
- V8 updated to 5.0.71.31 #6111.
DNS
- Add resolvePtr API to query plain DNS PTR records #4921.
Domains
- Clear stack when no error handler #4659.
File System
- The fs.realpath() and fs.realpathSync() methods have been updated to use a more efficient libuv implementation. This change includes the removal of the cache argument and the method can throw new errors #3594
- FS apis can now accept and return paths as Buffers #5616.
- Error handling and type checking improvements #5616, #5590, #4518, #3917.
- fs.read's string interface is deprecated #4525
HTTP
- 'clientError' can now be used to return custom errors from an HTTP server #4557.
Modules
- Current directory is now prioritized for local lookups #5689
- Symbolic links are preserved when requiring modules #5950
Net
- DNS hints no longer implicitly set #6021.
- Improved error handling and type checking #5981, #5733, #2904
OS X
- MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 #6402.
Path
- Improved type checking #5348.
Process
- Introduce process warnings API #4782.
- Throw exception when non-function passed to nextTick #3860.
Readline
- Emit key info unconditionally #6024
REPL
- Assignment to _ will emit a warning. #5535
Timers
- Fail early when callback is not a function #4362
TLS
- Rename 'clientError' to 'tlsClientError' #4557
- SHA1 used for sessionIdContext #3866
TTY
- Previously deprecated setRawMode wrapper is removed #2528.
Util
- Changes to Error object formatting #4582.
Windows
- Windows XP and Vista are no longer supported #5167, #5167.

jasnell added a commit that referenced this pull request

Apr 26, 2016

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release.

Buffer
- New Buffer constructors have been added #4682
- Previously deprecated Buffer APIs are removed #5048, #4594
- Improved error handling #4514
Cluster
- Worker emitted as first argument in 'message' event #5361.
Crypto
- Improved error handling #3100, #5611
- Simplified Certificate class bindings #5382
- Improved control over FIPS mode #5181
- pbkdf2 digest overloading is deprecated #4047
Dependencies
- Reintroduce shared c-ares build support #5775.
- V8 updated to 5.0.71.31 #6111.
DNS
- Add resolvePtr API to query plain DNS PTR records #4921.
Domains
- Clear stack when no error handler #4659.
File System
- The fs.realpath() and fs.realpathSync() methods have been updated to use a more efficient libuv implementation. This change includes the removal of the cache argument and the method can throw new errors #3594
- FS apis can now accept and return paths as Buffers #5616.
- Error handling and type checking improvements #5616, #5590, #4518, #3917.
- fs.read's string interface is deprecated #4525
HTTP
- 'clientError' can now be used to return custom errors from an HTTP server #4557.
Modules
- Current directory is now prioritized for local lookups #5689
- Symbolic links are preserved when requiring modules #5950
Net
- DNS hints no longer implicitly set #6021.
- Improved error handling and type checking #5981, #5733, #2904
OS X
- MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 #6402.
Path
- Improved type checking #5348.
Process
- Introduce process warnings API #4782.
- Throw exception when non-function passed to nextTick #3860.
Readline
- Emit key info unconditionally #6024
REPL
- Assignment to _ will emit a warning. #5535
Timers
- Fail early when callback is not a function #4362
TLS
- Rename 'clientError' to 'tlsClientError' #4557
- SHA1 used for sessionIdContext #3866
TTY
- Previously deprecated setRawMode wrapper is removed #2528.
Util
- Changes to Error object formatting #4582.
Windows
- Windows XP and Vista are no longer supported #5167, #5167.

jasnell added a commit that referenced this pull request

Apr 26, 2016

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release.

Buffer
- New Buffer constructors have been added #4682
- Previously deprecated Buffer APIs are removed #5048, #4594
- Improved error handling #4514
Cluster
- Worker emitted as first argument in 'message' event #5361.
Crypto
- Improved error handling #3100, #5611
- Simplified Certificate class bindings #5382
- Improved control over FIPS mode #5181
- pbkdf2 digest overloading is deprecated #4047
Dependencies
- Reintroduce shared c-ares build support #5775.
- V8 updated to 5.0.71.31 #6111.
DNS
- Add resolvePtr API to query plain DNS PTR records #4921.
Domains
- Clear stack when no error handler #4659.
File System
- The fs.realpath() and fs.realpathSync() methods have been updated to use a more efficient libuv implementation. This change includes the removal of the cache argument and the method can throw new errors #3594
- FS apis can now accept and return paths as Buffers #5616.
- Error handling and type checking improvements #5616, #5590, #4518, #3917.
- fs.read's string interface is deprecated #4525
HTTP
- 'clientError' can now be used to return custom errors from an HTTP server #4557.
Modules
- Current directory is now prioritized for local lookups #5689
- Symbolic links are preserved when requiring modules #5950
Net
- DNS hints no longer implicitly set #6021.
- Improved error handling and type checking #5981, #5733, #2904
OS X
- MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 #6402.
Path
- Improved type checking #5348.
Process
- Introduce process warnings API #4782.
- Throw exception when non-function passed to nextTick #3860.
Readline
- Emit key info unconditionally #6024
REPL
- Assignment to _ will emit a warning. #5535
Timers
- Fail early when callback is not a function #4362
TLS
- Rename 'clientError' to 'tlsClientError' #4557
- SHA1 used for sessionIdContext #3866
TTY
- Previously deprecated setRawMode wrapper is removed #2528.
Util
- Changes to Error object formatting #4582.
Windows
- Windows XP and Vista are no longer supported #5167, #5167.

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.