Skip to content

buffer: construct Buffers in JS#2866

Closed
trevnorris wants to merge 1 commit intonodejs:masterfrom
trevnorris:avoid-zero-fill
Closed

buffer: construct Buffers in JS#2866
trevnorris wants to merge 1 commit intonodejs:masterfrom
trevnorris:avoid-zero-fill

Conversation

@trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Sep 14, 2015

Overall construction time is faster in JS, but the problem is zero-fill
of memory. Get around this by using a flag in the ArrayBuffer::Allocator
to trigger when memory should or shouldn't be zero-filled.

R=@bnoordhuis
R=@indutny

Warning. There is some mess in here. Throwing it up for general review.

indutny
indutny reviewed Sep 14, 2015
View reviewed changes
lib/buffer.js
Copy link
Member

@indutny indutny Sep 14, 2015

Choose a reason for hiding this comment

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

What do you think about making this one-byte Uint8Array instead, and passing it to the C++?

Copy link
Member

@indutny indutny Sep 14, 2015

Choose a reason for hiding this comment

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

nvm, you did it this way :)

@brendanashworth brendanashworth added the buffer Issues and PRs related to the buffer subsystem. label Sep 14, 2015
indutny
indutny reviewed Sep 15, 2015
View reviewed changes
lib/buffer.js Outdated
Copy link
Member

@indutny indutny Sep 15, 2015

Choose a reason for hiding this comment

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

What do you think about getting rid of this thing and using class.. extends?

Copy link
Contributor Author

@trevnorris trevnorris Sep 15, 2015

Choose a reason for hiding this comment

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

super() cannot be optimized. using class.. extends was the first approach I took when reimplementing Buffer and it had significant overhead. Which is the initial reason I went the route of the C++ construction.

@trevnorris
buffer: construct Uint8Array in JS
d235fe2 
Overall construction time of Typed Arrays is faster in JS, but the
problem with using it normally is zero-fill of memory. Get around this
by using a flag in the ArrayBuffer::Allocator to trigger when memory
should or shouldn't be zero-filled.

Remove Buffer::Create() as it is no longer called.

The creation of the Uint8Array() was done at each callsite because at
the time of this patch there was a performance penalty for centralizing
the call in a single function.
indutny
indutny reviewed Sep 15, 2015
View reviewed changes
lib/buffer.js
Copy link
Member

@indutny indutny Sep 15, 2015

Choose a reason for hiding this comment

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

Hahaha, this one is better! :)

Are you afraid of class...extends? We can address it later, though.

Copy link
Contributor Author

@trevnorris trevnorris Sep 15, 2015

Choose a reason for hiding this comment

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

class is too slow. The call to super() can't be optimized by v8. I learned this b/c the initial implementation used class...extends and I never saw the constructor show up in IRHydra.

Copy link
Member

@indutny indutny Sep 15, 2015

Choose a reason for hiding this comment

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

You sure about this? I have seen the disassembly output, and it looked like the Buffer constructor was inlined and was called Uint8Array constructor directly

Copy link
Contributor Author

@trevnorris trevnorris Sep 15, 2015

Choose a reason for hiding this comment

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

This was when class was very first supported. So additional optimizations may have been done since then.

@indutny
Copy link
Member

indutny commented Sep 15, 2015

One naming nit, otherwise LGTM.

trevnorris added a commit that referenced this pull request Sep 15, 2015
@trevnorris
buffer: construct Uint8Array in JS
74178a5 
Overall construction time of Typed Arrays is faster in JS, but the
problem with using it normally is zero-fill of memory. Get around this
by using a flag in the ArrayBuffer::Allocator to trigger when memory
should or shouldn't be zero-filled.

Remove Buffer::Create() as it is no longer called.

The creation of the Uint8Array() was done at each callsite because at
the time of this patch there was a performance penalty for centralizing
the call in a single function.

PR-URL: #2866
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@trevnorris
Copy link
Contributor Author

trevnorris commented Sep 15, 2015

Suggested name changed. Landed in 74178a5.

@trevnorris trevnorris closed this Sep 15, 2015
@trevnorris trevnorris deleted the avoid-zero-fill branch September 15, 2015 22:28
trevnorris added a commit that referenced this pull request Sep 16, 2015
@trevnorris @Fishrock123
buffer: construct Uint8Array in JS
7df018a 
Overall construction time of Typed Arrays is faster in JS, but the
problem with using it normally is zero-fill of memory. Get around this
by using a flag in the ArrayBuffer::Allocator to trigger when memory
should or shouldn't be zero-filled.

Remove Buffer::Create() as it is no longer called.

The creation of the Uint8Array() was done at each callsite because at
the time of this patch there was a performance penalty for centralizing
the call in a single function.

PR-URL: #2866
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@Fishrock123 Fishrock123 mentioned this pull request Sep 16, 2015
Closed
7 tasks
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 17, 2015
@Fishrock123
2015-09-17, Version 4.1.0 (Stable) Release
b4ec2c2 
Notable changes:

* buffer:
  - Buffers are now created in JavaScript, rather than C++. This increases the speed of buffer creation (Trevor Norris) nodejs#2866.
  - `Buffer#slice()` now uses `Uint8Array#subarray()` internally, increasing `slice()` performance (Karl Skomski) nodejs#2777.
* fs:
  - `fs.utimes()` now properly converts numeric strings, `NaN`, and `Infinity` (Yazhong Liu) nodejs#2387.
  - `fs.WriteStream` now implements `_writev`, allowing for super-fast bulk writes (Ron Korving) nodejs#2167.
* http: Fixed an issue with certain `write()` sizes causing errors when using `http.request()` (Fedor Indutny) nodejs#2824.
* npm: Upgrade to version 2.14.3, see https://github.com/npm/npm/releases/tag/v2.14.3 for more details (Kat Marchán) nodejs#2822.
* src: V8 cpu profiling no longer erroneously shows idle time (Oleksandr Chekhovskyi) nodejs#2324.
* v8: Lateral upgrade to 4.5.103.33 from 4.5.103.30, contains minor fixes (Ali Ijaz Sheikh) nodejs#2870.
  - This fixes a previously known bug where some computed object shorthand properties did not work correctly (nodejs#2507).

Refs: nodejs#2844
PR-URL: nodejs#2889
Fishrock123 added a commit that referenced this pull request Sep 17, 2015
@Fishrock123 @rvagg
2015-09-17, Version 4.1.0 (Stable) Release
e4f2952 
Notable changes:

* buffer:
  - Buffers are now created in JavaScript, rather than C++. This increases the speed of buffer creation (Trevor Norris) #2866.
  - `Buffer#slice()` now uses `Uint8Array#subarray()` internally, increasing `slice()` performance (Karl Skomski) #2777.
* fs:
  - `fs.utimes()` now properly converts numeric strings, `NaN`, and `Infinity` (Yazhong Liu) #2387.
  - `fs.WriteStream` now implements `_writev`, allowing for super-fast bulk writes (Ron Korving) #2167.
* http: Fixed an issue with certain `write()` sizes causing errors when using `http.request()` (Fedor Indutny) #2824.
* npm: Upgrade to version 2.14.3, see https://github.com/npm/npm/releases/tag/v2.14.3 for more details (Kat Marchán) #2822.
* src: V8 cpu profiling no longer erroneously shows idle time (Oleksandr Chekhovskyi) #2324.
* v8: Lateral upgrade to 4.5.103.33 from 4.5.103.30, contains minor fixes (Ali Ijaz Sheikh) #2870.
  - This fixes a previously known bug where some computed object shorthand properties did not work correctly (#2507).

Refs: #2844
PR-URL: #2889
@rvagg rvagg mentioned this pull request Sep 22, 2015
Merged
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 16, 2015

landed in lts-v4.x-staging as 7df018a

@ronag ronag mentioned this pull request Jul 14, 2019
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@trevnorris @indutny @MylesBorins @jasnell @brendanashworth