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

benchmark: add benchmarks for Buffer.from() #8738

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmarks

Description of change

Adds benchmarks for Buffer.from() and its various argument combinations.

Ref: #8733

It also looks like there’s a performance regression on the current master that isn’t on v6 and which exceeds the one described in #8733 notably:

$ nvm use 6.6
Now using node v6.6.0
$ node benchmark/buffers/buffer-from.js
buffers/buffer-from.js n=1024 len=10 source="array": 1352.177972206885
buffers/buffer-from.js n=1024 len=2048 source="array": 108.78659817646022
buffers/buffer-from.js n=1024 len=10 source="arraybuffer": 2849.9419428697133
buffers/buffer-from.js n=1024 len=2048 source="arraybuffer": 3303.4248644402187
buffers/buffer-from.js n=1024 len=10 source="arraybuffer-middle": 3115.8790362447385
buffers/buffer-from.js n=1024 len=2048 source="arraybuffer-middle": 3143.4771954881257
buffers/buffer-from.js n=1024 len=10 source="buffer": 1362.2701961428957
buffers/buffer-from.js n=1024 len=2048 source="buffer": 367.766503317392
buffers/buffer-from.js n=1024 len=10 source="uint8array": 1480.231221483884
buffers/buffer-from.js n=1024 len=2048 source="uint8array": 139.1829921904438
buffers/buffer-from.js n=1024 len=10 source="string": 685.3303955985581
buffers/buffer-from.js n=1024 len=2048 source="string": 163.58717416397928
buffers/buffer-from.js n=1024 len=10 source="string-base64": 504.0783343717544
buffers/buffer-from.js n=1024 len=2048 source="string-base64": 110.29233159776292
$ ./node benchmark/buffers/buffer-from.js 
buffers/buffer-from.js n=1024 len=10 source="array": 522.2764604853
buffers/buffer-from.js n=1024 len=2048 source="array": 97.72530491842134
buffers/buffer-from.js n=1024 len=10 source="arraybuffer": 776.7268797100915
buffers/buffer-from.js n=1024 len=2048 source="arraybuffer": 658.5854603318716
buffers/buffer-from.js n=1024 len=10 source="arraybuffer-middle": 690.3042490225982
buffers/buffer-from.js n=1024 len=2048 source="arraybuffer-middle": 742.2573140765411
buffers/buffer-from.js n=1024 len=10 source="buffer": 496.9498820166972
buffers/buffer-from.js n=1024 len=2048 source="buffer": 296.6712882896401
buffers/buffer-from.js n=1024 len=10 source="uint8array": 561.3881977090617
buffers/buffer-from.js n=1024 len=2048 source="uint8array": 124.81186442783465
buffers/buffer-from.js n=1024 len=10 source="string": 386.8836136075654
buffers/buffer-from.js n=1024 len=2048 source="string": 173.50450305089902
buffers/buffer-from.js n=1024 len=10 source="string-base64": 370.7046939007373
buffers/buffer-from.js n=1024 len=2048 source="string-base64": 120.02970526090957

:(

Adds benchmarks for `Buffer.from()` and its various
argument combinations.

Ref: nodejs#8733
@addaleax addaleax added buffer Issues and PRs related to the buffer subsystem. benchmark Issues and PRs related to the benchmark subsystem. labels Sep 23, 2016
@Fishrock123
Copy link
Contributor

Ouch! Even if these "micro benchmarks" are not 100% accurate, they could still be good for discovering large deltas.

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member Author

Ouch! Even if these "micro benchmarks" are not 100% accurate, they could still be good for discovering large deltas.

Yeah. The difference actually seems pretty reproducible to me, so I’m going to starting bisecting now…

@addaleax
Copy link
Member Author

The first bad commit could be any of:
785506a1fc460323074dbeffb27b4518c404e9b4
fbfc15c51b7cb8de96d6719d3b2096c086530ee3
7292a1e954e0db348ff78c704df9e105ba5667ad
ec02b811a8a5c999bab4de312be2d732b7d9d50b
2187bd3b4a3430880ec1c2b99e4eff57f27afd89

Those are, unfortunately, the V8 5.4 update from #8317. /cc @nodejs/v8

(For completeness: benchmark results at all bisect points; bisected from the v6.0.0 release up to the current HEAD.)

@jasnell
Copy link
Member

jasnell commented Sep 23, 2016

@addaleax ... ouch. That's a big drop.

For the bench mark itself, can you perhaps move each case into a separate function then use common.v8ForceOptimization() to force V8 to optimize before doing the actual benchmark. I'm curious to see if that would have any tangible impact. (I doubt it, but worth checking)

@addaleax
Copy link
Member Author

@jasnell I can try that later, but since you’re obviously having something in specific in mind and I’ll be afk for a bit, feel free to use the Allow edits from maintainers. thingy to make any changes you’d like to see. :)

@jasnell
Copy link
Member

jasnell commented Sep 23, 2016

heh... yeah, considered that but I need to be away from the keyboard for a bit also (my two boys have a cross country track meet at school and they need a cheering section). When I get back on later today I'll see what I can work up.

@targos
Copy link
Member

targos commented Sep 23, 2016

I found the cause of the regression:

In the new FastBuffer(allocPool, poolOffset, size); call in allocate, V8 creates and uses an array iterator (for a yet unknown reason).

@ofrobots ofrobots added the v8 engine Issues and PRs related to the V8 dependency. label Sep 23, 2016
@addaleax
Copy link
Member Author

I'm curious to see if that would have any tangible impact.

@jasnell it doesn’t seem to have much of an effect. Would you still prefer that change?

@jasnell
Copy link
Member

jasnell commented Sep 23, 2016

No that's OK. Given what appears to be the cause of the regression, it's
not going to have much impact. Thank you for checking tho!

On Friday, September 23, 2016, Anna Henningsen notifications@github.com
wrote:

I'm curious to see if that would have any tangible impact.

@jasnell https://github.com/jasnell it doesn’t seem to have much of an
effect. Would you still prefer that change?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8738 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eXoEhGmffn7XJ5o8EY-AU6b9Q-ZTks5qtEC-gaJpZM4KFAv2
.

@targos targos mentioned this pull request Sep 23, 2016
2 tasks
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member Author

Landed in 289d862

@addaleax addaleax closed this Sep 26, 2016
addaleax added a commit that referenced this pull request Sep 26, 2016
Adds benchmarks for `Buffer.from()` and its various
argument combinations.

Ref: #8733
PR-URL: #8738
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@addaleax addaleax deleted the buffer-from branch September 26, 2016 18:23
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Adds benchmarks for `Buffer.from()` and its various
argument combinations.

Ref: #8733
PR-URL: #8738
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Adds benchmarks for `Buffer.from()` and its various
argument combinations.

Ref: #8733
PR-URL: #8738
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@fhinkel
Copy link
Member

fhinkel commented Feb 3, 2017

We improved spread-super performance (my microbenchmark is as fast in 5.8 as it was in 5.1 when it wasn't spec compliant). Can we revisit this, once we have 5.8 in Node?

@addaleax
Copy link
Member Author

addaleax commented Feb 3, 2017

Can we revisit this, once we have 5.8 in Node?

What exactly? The change in #8754? I would assume we could just revert that if we want and it doesn’t have an performance impact.

@fhinkel
Copy link
Member

fhinkel commented Feb 3, 2017

All I meant was to re-run the benchmarks. But we'll do that anyways. Sorry for the confusion. I got to excited by the performance improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. buffer Issues and PRs related to the buffer subsystem. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants