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.

Sign up for GitHub

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

Jump to bottom

Faster buffer slice using subarray() #2777

Closed
wants to merge 1 commit into from
Closed

Faster buffer slice using subarray() #2777

wants to merge 1 commit into from

Conversation

skomski
Copy link
Contributor

@skomski skomski commented Sep 9, 2015

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 mscdex added the buffer Issues and PRs related to the buffer subsystem. label Sep 9, 2015
@mscdex
Copy link
Contributor

mscdex commented Sep 9, 2015

/cc @trevnorris

@ChALkeR
Copy link
Member

ChALkeR commented Sep 9, 2015

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

@trevnorris
Copy link
Contributor

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

ChALkeR commented Sep 10, 2015

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
Copy link
Contributor

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 skomski force-pushed the faster-buffer-slice branch from 20a591f to 1f91ea3 Compare September 10, 2015 22:27
@skomski
Copy link
Contributor Author

skomski commented Sep 10, 2015

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
thefourtheye reviewed Sep 13, 2015
View reviewed changes
lib/buffer.js

return binding.slice(this, start, end);
var buffer = this.subarray(start, end);
buffer.__proto__ = Buffer.prototype;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use Object.setPrototypeOf

@Fishrock123 Fishrock123 mentioned this pull request Sep 13, 2015
Closed
7 tasks
@skomski skomski force-pushed the faster-buffer-slice branch from 1f91ea3 to 4867e3c Compare September 13, 2015 20:10
@trevnorris
Copy link
Contributor

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

@trevnorris
Copy link
Contributor

LGTM

@trevnorris
Copy link
Contributor

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

@trevnorris
Copy link
Contributor

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

@ChALkeR
Copy link
Member

ChALkeR commented Sep 15, 2015

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
ChALkeR reviewed Sep 15, 2015
View reviewed changes
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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed because initially the commit included the TODO.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@skomski
src: use subarray() in Buffer.slice() for speedup
47abd16 
benchmark/buffer-slice: 40%
benchmark/buffer-creation (pool): 25%
@skomski skomski force-pushed the faster-buffer-slice branch from 4867e3c to 47abd16 Compare September 15, 2015 19:16
trevnorris pushed a commit that referenced this pull request Sep 15, 2015
@skomski @trevnorris
src: use subarray() in Buffer#slice() for speedup
e7a3ca3 
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
Copy link
Contributor

Thanks. Landed in e7a3ca3 with minor commit message change.

@trevnorris trevnorris closed this Sep 15, 2015
Fishrock123 pushed a commit that referenced this pull request Sep 16, 2015
@skomski @Fishrock123
src: use subarray() in Buffer#slice() for speedup
0a5f80a 
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
Copy link
Contributor Author

skomski commented Sep 16, 2015

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

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
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.
8 participants
@skomski @mscdex @ChALkeR @trevnorris @thefourtheye @jasnell @MylesBorins @Fishrock123