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

stream: improve read() performance more #28989

Merged
merged 1 commit into from Aug 9, 2019

Conversation

@mscdex
Copy link
Contributor

commented Aug 6, 2019

Results:

                                                        confidence improvement accuracy (*)    (**)   (***)
 streams/readable-bigread.js n=1000                           ***     43.35 %       ±2.61%  ±3.50%  ±4.61%
 streams/readable-bigunevenread.js n=1000                     ***     26.71 %       ±8.11% ±10.83% ±14.17%
 streams/readable-unevenread.js n=1000                        ***      3.42 %       ±1.13%  ±1.50%  ±1.96%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@nodejs-github-bot

This comment has been minimized.

if (nb === buf.length)
ret.set(buf, ret.length - n);
else
ret.set(new Uint8Array(buf.buffer, buf.byteOffset, nb), ret.length - n);

This comment has been minimized.

Copy link
@ronag

ronag Aug 6, 2019

Contributor

how does this differ from copy?

This comment has been minimized.

Copy link
@mscdex

mscdex Aug 6, 2019

Author Contributor

It's a tad faster.

This comment has been minimized.

Copy link
@ronag

ronag Aug 6, 2019

Contributor

yes, just confused as two how it can be faster. Some V8 thingy?

This comment has been minimized.

Copy link
@devsnek

devsnek Aug 6, 2019

Member

yes. v8 can optimize the stuff it provides, but our buffer copy is basically a black box to it, since its written in c++. we could probably optimize our copy impl to use typedarray's set if there are no source offsets passed.

This comment has been minimized.

Copy link
@ronag

ronag Aug 6, 2019

Contributor

wouldn't it better to apply this there then?

@targos
targos approved these changes Aug 6, 2019
@trivikr
trivikr approved these changes Aug 6, 2019
@jasnell
jasnell approved these changes Aug 7, 2019
@lpinca
lpinca approved these changes Aug 7, 2019
@ronag

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Doesn't #29066 supersede this PR?

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@ronag Not really, buffer.copy() has to perform a lot of additional checks that we can skip because of some assumptions we can safely make inside this function.

stream: improve read() performance more
PR-URL: #28989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

@mscdex mscdex force-pushed the mscdex:streams-readable-perf2 branch from 1744bd1 to 53e467d Aug 9, 2019

@mscdex mscdex merged commit 53e467d into nodejs:master Aug 9, 2019

0 of 2 checks passed

Travis CI - Pull Request Build Errored
Details
Travis CI - Branch Build Canceled
Details

@mscdex mscdex deleted the mscdex:streams-readable-perf2 branch Aug 9, 2019

targos added a commit that referenced this pull request Aug 19, 2019
stream: improve read() performance more
PR-URL: #28989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos added a commit that referenced this pull request Aug 19, 2019
stream: improve read() performance more
PR-URL: #28989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos referenced this pull request Aug 19, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
stream: improve read() performance more
PR-URL: nodejs#28989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
stream: improve read() performance more
PR-URL: nodejs#28989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.