benchmark: add benchmarks for `Buffer.from()` by addaleax · Pull Request #8738 · nodejs/node

addaleax · 2016-09-23T14:01:25Z

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.

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

Fishrock123 · 2016-09-23T14:22:01Z

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

lpinca

LGTM

addaleax · 2016-09-23T16:53:40Z

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 · 2016-09-23T17:48:16Z

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 · 2016-09-23T19:00:05Z

@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 · 2016-09-23T19:02:38Z

@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 · 2016-09-23T19:03:59Z

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 · 2016-09-23T20:41:48Z

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).

addaleax · 2016-09-23T21:12:23Z

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 · 2016-09-23T21:14:30Z

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
.

imyller

LGTM

addaleax · 2016-09-26T18:22:41Z

Landed in 289d862

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 · 2017-02-03T10:36:42Z

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 · 2017-02-03T11:11:53Z

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 · 2017-02-03T11:47:38Z

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.

benchmark: add benchmarks for Buffer.from()

cb4c7d2

Adds benchmarks for `Buffer.from()` and its various argument combinations. Ref: nodejs#8733

addaleax added buffer Issues and PRs related to the buffer subsystem. benchmark Issues and PRs related to the benchmark subsystem. labels Sep 23, 2016

addaleax mentioned this pull request Sep 23, 2016

Buffer performance degradation in 6.6.0 #8733

Closed

lpinca approved these changes Sep 23, 2016

View reviewed changes

ofrobots added the v8 engine Issues and PRs related to the V8 dependency. label Sep 23, 2016

targos mentioned this pull request Sep 23, 2016

buffer: fix performance regression #8754

Merged

2 tasks

imyller approved these changes Sep 26, 2016

View reviewed changes

targos mentioned this pull request Sep 26, 2016

Audit performance of master with V8 5.4 #8793

Closed

addaleax closed this Sep 26, 2016

addaleax deleted the buffer-from branch September 26, 2016 18:23

MylesBorins added the dont-land-on-v4.x label Oct 6, 2016

Uh oh!

Conversation

addaleax commented Sep 23, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123 commented Sep 23, 2016

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 23, 2016

Uh oh!

addaleax commented Sep 23, 2016

Uh oh!

jasnell commented Sep 23, 2016

Uh oh!

addaleax commented Sep 23, 2016

Uh oh!

jasnell commented Sep 23, 2016

Uh oh!

targos commented Sep 23, 2016

Uh oh!

addaleax commented Sep 23, 2016

Uh oh!

jasnell commented Sep 23, 2016

Uh oh!

imyller left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 26, 2016

Uh oh!

fhinkel commented Feb 3, 2017

Uh oh!

addaleax commented Feb 3, 2017

Uh oh!

fhinkel commented Feb 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants