Faster buffer slice using subarray() #2777

skomski · 2015-09-09T16:49:48Z

src: use subarray() in Buffer.slice() for speedup
src: throw on out-of-range args in Buffer.slice

Only changed tests that passed out-of-range args

buffers/buffer-slice.js type=fast n=1024: ./node: 4201.9 ./node_slow: 2941.8 . 42.84%
buffers/buffer-slice.js type=slow n=1024: ./node: 4159.2 ./node_slow: 2957 ... 40.66%

buffers/buffer-creation.js type=fast len=10 n=1024: ./node: 3875.3 ./node_slow: 3094.6 .. 25.23%
buffers/buffer-creation.js type=fast len=1024 n=1024: ./node: 2061 ./node_slow: 1810.4 .. 13.84%

mscdex · 2015-09-09T16:54:50Z

/cc @trevnorris

ChALkeR · 2015-09-09T21:06:04Z

This changes the behaviour on non-numeric arguments. For example, start = '-10'.

trevnorris · 2015-09-10T20:53:49Z

Can you change up the commits so the first commit only contains the performance improvement? That is a minor change that we can land on v4. But the additions of throwing for out of range are a major change.

ChALkeR · 2015-09-10T21:02:36Z

Now the range checks are not valid (well, as they were not valid before). See #2668.
For example, ~~-1e30 === 0.

This shouldn't stop this PR, because it's a separate issue.

trevnorris · 2015-09-10T21:06:00Z

Yeah. If anything I'd like to get the performance fix in so it can land on the v4.x branch asap. we can discuss edge cases of handling range checks afterwards.

skomski · 2015-09-10T22:34:24Z

It was already separated but I added the new string tests to the first one. Should I make a new PR for out-of-range commits or push behind?

thefourtheye · 2015-09-13T05:03:07Z

lib/buffer.js

-
-  return binding.slice(this, start, end);
+  var buffer = this.subarray(start, end);
+  buffer.__proto__ = Buffer.prototype;


Please use Object.setPrototypeOf

trevnorris · 2015-09-14T09:29:39Z

New PR please. Will make it easier to track and merge to other branches.

trevnorris · 2015-09-15T18:30:06Z

LGTM

trevnorris · 2015-09-15T18:34:17Z

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

trevnorris · 2015-09-15T18:47:22Z

and again: https://ci.nodejs.org/job/node-test-pull-request/308/

ChALkeR · 2015-09-15T18:56:53Z

LGTM if passes CI.
One question: why removing this comment by @trevnorris? The behavior didn't seem to change.

// TODO(trevnorris): currently works like Array.prototype.slice(), which        
// doesn't follow the new standard for throwing on out of range indexes.

ChALkeR · 2015-09-15T18:59:49Z

lib/buffer.js

@@ -549,33 +549,10 @@ Buffer.prototype.toJSON = function() {
 };


-// TODO(trevnorris): currently works like Array.prototype.slice(), which
-// doesn't follow the new standard for throwing on out of range indexes.


Why removing this comment? The behavior wasn't changed.

Removed because initially the commit included the TODO.

Thanks for noticing this @ChALkeR

Looking back, I put this in over 2 years ago (3a2f273) when I was attempting to be more strict with how arguments were handled. Now that we've been forced to use typed arrays it's safe to say that the original intention of this comment is dead. It's fine to leave it removed.

Oop, and just saw that it was added back. That's cool to. Maybe we should get some feedback about whether it should throw. I dunno anymore.

@trevnorris That's out of scope of this PR =).
Btw, in that CI even the linter blew up on the Jenkins side.

Yeah. The tests look fine to me. Going to land it.

benchmark/buffer-slice: 40% benchmark/buffer-creation (pool): 25%

Use the built-in Typed Array method subarray() to improve performance of Buffer#slice(). Benchmark improvements: benchmark/buffer-slice: 40% benchmark/buffer-creation (pool): 25% Additional tests also added. PR-URL: #2777 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

trevnorris · 2015-09-15T21:51:27Z

Thanks. Landed in e7a3ca3 with minor commit message change.

Use the built-in Typed Array method subarray() to improve performance of Buffer#slice(). Benchmark improvements: benchmark/buffer-slice: 40% benchmark/buffer-creation (pool): 25% Additional tests also added. PR-URL: #2777 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

skomski · 2015-09-16T18:56:09Z

My performance patch for subarray landed in v8: https://codereview.chromium.org/1331993004
Performance improvement is 4200 ms -> 3500 ms (16.67%).

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

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

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Sep 9, 2015

skomski force-pushed the faster-buffer-slice branch from 20a591f to 1f91ea3 Compare September 10, 2015 22:27

thefourtheye reviewed Sep 13, 2015
View reviewed changes

Fishrock123 added the land-on-v4.x label Sep 13, 2015

Fishrock123 mentioned this pull request Sep 13, 2015

Release proposal: v4.1.0 #2844

Closed

7 tasks

skomski force-pushed the faster-buffer-slice branch from 1f91ea3 to 4867e3c Compare September 13, 2015 20:10

ChALkeR reviewed Sep 15, 2015
View reviewed changes

src: use subarray() in Buffer.slice() for speedup

47abd16

benchmark/buffer-slice: 40% benchmark/buffer-creation (pool): 25%

skomski force-pushed the faster-buffer-slice branch from 4867e3c to 47abd16 Compare September 15, 2015 19:16

trevnorris closed this Sep 15, 2015

rvagg mentioned this pull request Sep 22, 2015

Release proposal: v4.1.1 #2995

Merged

jasnell added the lts-landed-on-v4.x label Oct 7, 2015

MylesBorins removed the land-on-v4.x label Nov 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster buffer slice using subarray() #2777

Faster buffer slice using subarray() #2777

skomski commented Sep 9, 2015

mscdex commented Sep 9, 2015

ChALkeR commented Sep 9, 2015

trevnorris commented Sep 10, 2015

ChALkeR commented Sep 10, 2015

trevnorris commented Sep 10, 2015

skomski commented Sep 10, 2015

thefourtheye Sep 13, 2015

trevnorris commented Sep 14, 2015

trevnorris commented Sep 15, 2015

trevnorris commented Sep 15, 2015

trevnorris commented Sep 15, 2015

ChALkeR commented Sep 15, 2015

ChALkeR Sep 15, 2015

skomski Sep 15, 2015

trevnorris Sep 15, 2015

ChALkeR Sep 15, 2015

trevnorris Sep 15, 2015

trevnorris commented Sep 15, 2015

skomski commented Sep 16, 2015

Faster buffer slice using subarray() #2777

Faster buffer slice using subarray() #2777

Conversation

skomski commented Sep 9, 2015

mscdex commented Sep 9, 2015

ChALkeR commented Sep 9, 2015

trevnorris commented Sep 10, 2015

ChALkeR commented Sep 10, 2015

trevnorris commented Sep 10, 2015

skomski commented Sep 10, 2015

thefourtheye Sep 13, 2015

Choose a reason for hiding this comment

trevnorris commented Sep 14, 2015

trevnorris commented Sep 15, 2015

trevnorris commented Sep 15, 2015

trevnorris commented Sep 15, 2015

ChALkeR commented Sep 15, 2015

ChALkeR Sep 15, 2015

Choose a reason for hiding this comment

skomski Sep 15, 2015

Choose a reason for hiding this comment

trevnorris Sep 15, 2015

Choose a reason for hiding this comment

ChALkeR Sep 15, 2015

Choose a reason for hiding this comment

trevnorris Sep 15, 2015

Choose a reason for hiding this comment

trevnorris commented Sep 15, 2015

skomski commented Sep 16, 2015