Skip to content

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
@addaleaxaddaleax 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

@lpincalpinca 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
MemberAuthor

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
MemberAuthor

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

@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
MemberAuthor

@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

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

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

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

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

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 [email protected]
wrote:

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

@jasnellhttps://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
.

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

@imyllerimyller 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
MemberAuthor

Landed in 289d862

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@addaleaxaddaleax 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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@fhinkel
Copy link
Member

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
MemberAuthor

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

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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmarkIssues and PRs related to the benchmark subsystem.bufferIssues and PRs related to the buffer subsystem.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@addaleax@Fishrock123@jasnell@targos@fhinkel@lpinca@imyller@ofrobots@MylesBorins