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: use FastBuffer when fill is set to 0 #21989

Closed
wants to merge 1 commit into from

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Jul 26, 2018

A large number of libraries seem to use Buffer.alloc(size, 0) instead of just Buffer.alloc(size).

We don't need to follow the «create unsafe buffer and fill it» path (i.e. actually allocate and perform fill) in that situation, that is better handled by Uint8Array constructor.

Buffer.alloc(size) and Buffer.alloc(size, 0) are equivalent, so use the same code path.

Not performing the zero-fill manually and having the underlying memory allocator do it for us can improve speed and reduce the memory usage for situations where Buffer.alloc(size, 0) is used.

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

Tests/benchmarks are not included as whether or not there would be improvement depends on the underlying memory allocator behavior (and probably also operating system memory management).

This is the difference on the current Node.js version (v10.7.0) and Linux 4.17.9:

> x = Buffer.alloc(1e9); x.length
1000000000
> `${process.memoryUsage().rss / 2**20} MiB`
'31.16796875 MiB'
> x.fill(0x42, 10, 20)
<Buffer 00 00 00 00 00 00 00 00 00 00 42 42 42 42 42 42 42 42 42 42 00 00 00 00 00 ... >
> `${process.memoryUsage().rss / 2**20} MiB`
'31.9921875 MiB'
> x = Buffer.alloc(1e9, 0); x.length
1000000000
> `${process.memoryUsage().rss / 2**20} MiB`
'985.9296875 MiB'

As it can be seen, Buffer.alloc(size) does not actually consume and/or zero-fill physical memory on the time of construction. Underlying memory allocation mechanism is able to track that the memory is supposed to be zero-filled (as calloc behaves), and zero-filled pages could be returned on first read.

That also affects speed in cases when memory is not read but is just overwritten:

> x = process.hrtime(); Buffer.alloc(2e9).fill(2); process.hrtime(x)
[ 0, 916665006 ]
> x = process.hrtime(); Buffer.alloc(2e9, 0).fill(2); process.hrtime(x)
[ 1, 4762215 ]
> x = process.hrtime(); Buffer.alloc(2e9).fill(2); process.hrtime(x)
[ 0, 970604205 ]
> x = process.hrtime(); Buffer.alloc(2e9, 0).fill(2); process.hrtime(x)
[ 1, 19652513 ]
> x = process.hrtime(); Buffer.alloc(2e9).fill(2); process.hrtime(x)
[ 0, 874969612 ]
> x = process.hrtime(); Buffer.alloc(2e9, 0).fill(2); process.hrtime(x)
[ 1, 30765432 ]

This change makes Buffer.alloc(size, 0) follow the same code path as more performant Buffer.alloc(size), which should increase speed in some cases where the buffer is overwritten and reduce memory usage in some cases where part of the buffer is later thrown away without being used (i.e. where more memory than needed is allocated temporarily).

I have seen usage of Buffer.alloc(size, 0) in many packages, including, but not limited to mysql2, hdkey, secp256k1 and more.

/cc @addaleax @bnoordhuis

A large number of libraries seem to use Buffer.alloc(size, 0)
instead of just Buffer.alloc(size).

We don't need to follow the "create unsafe buffer and fill it" path
(i.e. actually allocate and perform fill) in that situation, that is
better handled by Uint8Array constructor.

Buffer.alloc(size) and Buffer.alloc(size, 0) are equivalent, so
use the same code path.

Not performing the zero-fill manually and having the underlying memory
allocator do it for us can improve speed and reduce the memory usage
for situations where Buffer.alloc(size, 0) is used.
@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Jul 26, 2018
@gireeshpunathil
Copy link
Member

@ChALkeR - can you please show me where the zero-filling happens with the proposed changes?
[ On a side note, Ben is out of office ]

@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 27, 2018

@gireeshpunathil It calls new FastBuffer(size) on line 282, which inherits from Uint8Array, which is zero-filled by default. That is the same code path that is followed when fill is not defined, see https://nodejs.org/api/buffer.html#buffer_class_method_buffer_alloc_size_fill_encoding:

If fill is undefined, the Buffer will be zero-filled.

This change makes fill === 0 case follow the same code path as fill === undefined case.

@trivikr
Copy link
Member

trivikr commented Aug 1, 2018

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

@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 1, 2018
@addaleax
Copy link
Member

addaleax commented Aug 2, 2018

@trivikr
Copy link
Member

trivikr commented Aug 3, 2018

@trivikr
Copy link
Member

trivikr commented Aug 5, 2018

Landed in b07852d

@trivikr trivikr closed this Aug 5, 2018
trivikr pushed a commit that referenced this pull request Aug 5, 2018
A large number of libraries seem to use Buffer.alloc(size, 0)
instead of just Buffer.alloc(size).

We don't need to follow the "create unsafe buffer and fill it" path
(i.e. actually allocate and perform fill) in that situation, that is
better handled by Uint8Array constructor.

Buffer.alloc(size) and Buffer.alloc(size, 0) are equivalent, so
use the same code path.

Not performing the zero-fill manually and having the underlying memory
allocator do it for us can improve speed and reduce the memory usage
for situations where Buffer.alloc(size, 0) is used.

PR-URL: #21989
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this pull request Aug 6, 2018
A large number of libraries seem to use Buffer.alloc(size, 0)
instead of just Buffer.alloc(size).

We don't need to follow the "create unsafe buffer and fill it" path
(i.e. actually allocate and perform fill) in that situation, that is
better handled by Uint8Array constructor.

Buffer.alloc(size) and Buffer.alloc(size, 0) are equivalent, so
use the same code path.

Not performing the zero-fill manually and having the underlying memory
allocator do it for us can improve speed and reduce the memory usage
for situations where Buffer.alloc(size, 0) is used.

PR-URL: #21989
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.