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

buffer: fix performance regression #8754

Merged
merged 1 commit into from
Sep 26, 2016
Merged

Conversation

targos
Copy link
Member

@targos targos commented Sep 23, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

V8 5.4 changed the way that the default constructor of derived classes
is called. It introduced a significant performance regression in the
buffer module for the creation of pooled buffers. This commit forces the
definition back to how it was.

Ref: https://bugs.chromium.org/p/v8/issues/detail?id=4890

Benchmark results using the script from #8738:

6.6.0:
m.js n=1024 len=10 source="array": 2940.0523751381074
m.js n=1024 len=2048 source="array": 243.34844074199046
m.js n=1024 len=10 source="arraybuffer": 5469.801373510926
m.js n=1024 len=2048 source="arraybuffer": 5897.122441515577
m.js n=1024 len=10 source="arraybuffer-middle": 5715.82551047164
m.js n=1024 len=2048 source="arraybuffer-middle": 5651.568910575927
m.js n=1024 len=10 source="buffer": 2627.6210998395145
m.js n=1024 len=2048 source="buffer": 659.2472687131358
m.js n=1024 len=10 source="uint8array": 2598.5564648340537
m.js n=1024 len=2048 source="uint8array": 270.0350801781541
m.js n=1024 len=10 source="string": 1847.099925407467
m.js n=1024 len=2048 source="string": 335.8807107007938
m.js n=1024 len=10 source="string-base64": 1628.3220387388276
m.js n=1024 len=2048 source="string-base64": 293.5605053750516

Current master:
m.js n=1024 len=10 source="array": 1105.7778268274712
m.js n=1024 len=2048 source="array": 262.61636308568154
m.js n=1024 len=10 source="arraybuffer": 1463.1770393435074
m.js n=1024 len=2048 source="arraybuffer": 1438.3519347799318
m.js n=1024 len=10 source="arraybuffer-middle": 1289.6443262079608
m.js n=1024 len=2048 source="arraybuffer-middle": 1465.8917985524079
m.js n=1024 len=10 source="buffer": 1154.3281747402962
m.js n=1024 len=2048 source="buffer": 714.9198271277221
m.js n=1024 len=10 source="uint8array": 1188.4816701911184
m.js n=1024 len=2048 source="uint8array": 312.9837225134168
m.js n=1024 len=10 source="string": 1021.2184715065113
m.js n=1024 len=2048 source="string": 415.72725788425214
m.js n=1024 len=10 source="string-base64": 746.3019883767605
m.js n=1024 len=2048 source="string-base64": 272.66673662077363

This PR:
m.js n=1024 len=10 source="array": 4360.13359704818
m.js n=1024 len=2048 source="array": 210.9601264473078
m.js n=1024 len=10 source="arraybuffer": 6096.237297560867
m.js n=1024 len=2048 source="arraybuffer": 6204.587073036097
m.js n=1024 len=10 source="arraybuffer-middle": 6931.443006081272
m.js n=1024 len=2048 source="arraybuffer-middle": 5500.454679186303
m.js n=1024 len=10 source="buffer": 3741.9594463683297
m.js n=1024 len=2048 source="buffer": 782.6012936368813
m.js n=1024 len=10 source="uint8array": 4495.957363782579
m.js n=1024 len=2048 source="uint8array": 188.2792093087443
m.js n=1024 len=10 source="string": 2287.6058600223178
m.js n=1024 len=2048 source="string": 461.90557855442626
m.js n=1024 len=10 source="string-base64": 2026.5954755341668
m.js n=1024 len=2048 source="string-base64": 298.25954061659525

V8 5.4 changed the way that the default constructor of derived classes
is called. It introduced a significant performance regression in the
buffer module for the creation of pooled buffers. This commit forces the
definition back to how it was.

Ref: https://bugs.chromium.org/p/v8/issues/detail?id=4890
@targos targos added buffer Issues and PRs related to the buffer subsystem. performance Issues and PRs related to the performance of Node.js. labels Sep 23, 2016
@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Sep 23, 2016
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Yup, I can reproduce the benchmarking results.

Thanks and LGTM!

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@targos
Copy link
Member Author

targos commented Sep 24, 2016

@targos
Copy link
Member Author

targos commented Sep 24, 2016

/cc @indutny @trevnorris

@trevnorris
Copy link
Contributor

LGTM

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

@indutny
Copy link
Member

indutny commented Sep 24, 2016

Ahaha! Nice

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@Fishrock123
Copy link
Contributor

Wow, interesting.

@jbergstroem
Copy link
Member

Wow, silly. LGTM 👍

@targos targos merged commit 558a884 into nodejs:master Sep 26, 2016
@targos targos deleted the fix-buffer-perf branch September 26, 2016 18:14
@targos
Copy link
Member Author

targos commented Sep 26, 2016

Landed in 558a884

jasnell pushed a commit that referenced this pull request Sep 29, 2016
V8 5.4 changed the way that the default constructor of derived classes
is called. It introduced a significant performance regression in the
buffer module for the creation of pooled buffers. This commit forces the
definition back to how it was implicitly before.

Ref: https://bugs.chromium.org/p/v8/issues/detail?id=4890

PR-URL: #8754
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
V8 5.4 changed the way that the default constructor of derived classes
is called. It introduced a significant performance regression in the
buffer module for the creation of pooled buffers. This commit forces the
definition back to how it was implicitly before.

Ref: https://bugs.chromium.org/p/v8/issues/detail?id=4890

PR-URL: #8754
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants