Skip to content

buffer: Fix dataview-set benchmark. - #6922

Closed
RReverser wants to merge 1 commit into
nodejs:masterfrom
RReverser:buffer-dataview-bench
Closed

buffer: Fix dataview-set benchmark.#6922
RReverser wants to merge 1 commit into
nodejs:masterfrom
RReverser:buffer-dataview-bench

Conversation

@RReverser

@RReverserRReverser commented May 22, 2016

Copy link
Copy Markdown
Member
Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

Just fixes a benchmark code itself to provide proper measurement.

Improves numbers up to 4x by avoiding repetitive dynamic method lookup.

Before the change:

buffers\dataview-set.js type=Uint8 millions=1: 4.52066
buffers\dataview-set.js type=Uint16LE millions=1: 5.03784
buffers\dataview-set.js type=Uint16BE millions=1: 5.36833
buffers\dataview-set.js type=Uint32LE millions=1: 4.49819
buffers\dataview-set.js type=Uint32BE millions=1: 4.46667
buffers\dataview-set.js type=Int8 millions=1: 5.52648
buffers\dataview-set.js type=Int16LE millions=1: 5.54321
buffers\dataview-set.js type=Int16BE millions=1: 5.48971
buffers\dataview-set.js type=Int32LE millions=1: 5.61572
buffers\dataview-set.js type=Int32BE millions=1: 5.48682
buffers\dataview-set.js type=Float32LE millions=1: 5.43213
buffers\dataview-set.js type=Float32BE millions=1: 5.49020
buffers\dataview-set.js type=Float64LE millions=1: 5.41688
buffers\dataview-set.js type=Float64BE millions=1: 5.15072

After the change:

buffers\dataview-set.js type=Uint8 millions=1: 19.16623
buffers\dataview-set.js type=Uint16LE millions=1: 17.47599
buffers\dataview-set.js type=Uint16BE millions=1: 17.82669
buffers\dataview-set.js type=Uint32LE millions=1: 11.06974
buffers\dataview-set.js type=Uint32BE millions=1: 11.06411
buffers\dataview-set.js type=Int8 millions=1: 19.36784
buffers\dataview-set.js type=Int16LE millions=1: 13.45155
buffers\dataview-set.js type=Int16BE millions=1: 11.97407
buffers\dataview-set.js type=Int32LE millions=1: 18.88772
buffers\dataview-set.js type=Int32BE millions=1: 18.31484
buffers\dataview-set.js type=Float32LE millions=1: 18.33773
buffers\dataview-set.js type=Float32BE millions=1: 15.50081
buffers\dataview-set.js type=Float64LE millions=1: 16.86952
buffers\dataview-set.js type=Float64BE millions=1: 17.21089

(Just noticed this when going through #6893 and couldn't pass by.)

@nodejs-github-botnodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label May 22, 2016
@addaleaxaddaleax added the buffer Issues and PRs related to the buffer subsystem. label May 22, 2016
addaleax
addaleax reviewed May 22, 2016
View reviewed changes
Comment threadbenchmark/buffers/dataview-set.js Outdated

@addaleaxaddaleaxMay 22, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

const?

@RReverserRReverserMay 22, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Um... well, maybe, but in that case makes sense to change other places in this file as well. And, generally, unless already consistent with a file styling, I'm personally not a big fan of using block-scoped variables in benchmark code at least until V8 fixes their deopts (it does affect numbers quite a bit at the moment), but I can change if you really wish.

@addaleaxaddaleaxMay 22, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it affects the numbers (I know it can when it comes to let), feel free to leave it, but it’s become pretty standard around here to always use const when it makes sense… personally, I don’t care much, though. 😄

@RReverserRReverserMay 22, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well, if changing only those to const and not touching var i -> let i, then it looks fine, within the limtis.

@RReverserRReverserMay 22, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Updated.

@addaleax

addaleax commented May 22, 2016

Copy link
Copy Markdown
Member

Btw, if you want to refer to github issues/PRs/etc. in the commit message, you can do that, but it’s strongly preferred to use the full URL… I’d maybe drop that line anyway. :)

@RReverser

RReverser commented May 22, 2016

Copy link
Copy Markdown
MemberAuthor

Oh ok. Wasn't aware of that.

@addaleax

addaleax commented May 22, 2016

Copy link
Copy Markdown
Member

LGTM

@Fishrock123

Fishrock123 commented May 23, 2016

Copy link
Copy Markdown
Contributor

cc @nodejs/buffer

@trevnorris

trevnorris commented May 24, 2016

Copy link
Copy Markdown
Contributor

nits: git message title, don't cap "Fix" and don't end with period. In git message body want to place URL's in Ref:. e.g.

Improves numbers up to 4x by avoiding repetitive dynamic method lookup.
Ref: https://github.com/nodejs/node/pull/6893

Though as @addaleax mentioned, probably best to just drop that line since the ref isn't directly applicable to this change.

Change itself LGTM

@RReverser
buffer: fix dataview-set benchmark
0c664e9
Improves numbers up to 4x by avoiding repetitive dynamic method lookup.
@RReverser

RReverser commented May 25, 2016

Copy link
Copy Markdown
MemberAuthor

Changed the commit message.

@trevnorris

trevnorris commented May 26, 2016

Copy link
Copy Markdown
Contributor

Great. LGTM.

@jasnell

jasnell commented May 26, 2016

Copy link
Copy Markdown
Member

LGTM
@mscdex might have some thoughts tho.

@mscdex

mscdex commented May 26, 2016

Copy link
Copy Markdown
Contributor

lint passes: https://ci.nodejs.org/job/node-test-linter/2672/

LGTM

addaleax pushed a commit that referenced this pull request May 28, 2016
@RReverser@addaleax
buffer: fix dataview-set benchmark
4a56e89
Improves numbers up to 4x by avoiding repetitive dynamic method lookup.
PR-URL: #6922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@addaleax

addaleax commented May 28, 2016

Copy link
Copy Markdown
Member

Landed in 4a56e89 … Thanks!

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
@RReverser@Fishrock123
buffer: fix dataview-set benchmark
62dead9
Improves numbers up to 4x by avoiding repetitive dynamic method lookup.
PR-URL: nodejs#6922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
@RReverser@rvagg
buffer: fix dataview-set benchmark
1bcc226
Improves numbers up to 4x by avoiding repetitive dynamic method lookup.
PR-URL: #6922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@MylesBorinsMylesBorins mentioned this pull request Jul 11, 2016
Closed
@MylesBorins

MylesBorins commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

@RReverser lts?

@RReverser

RReverser commented Jul 12, 2016

Copy link
Copy Markdown
MemberAuthor

@thealphanerd Can do. Does it require backporting PR?

@MylesBorins

MylesBorins commented Jul 12, 2016

Copy link
Copy Markdown
Contributor

nope landed cleanly

MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@RReverserMyles Borins
buffer: fix dataview-set benchmark
957505a
Improves numbers up to 4x by avoiding repetitive dynamic method lookup.
PR-URL: #6922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@RReverserMyles Borins
buffer: fix dataview-set benchmark
182f731
Improves numbers up to 4x by avoiding repetitive dynamic method lookup.
PR-URL: #6922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@MylesBorinsMylesBorins mentioned this pull request Jul 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@RReverserMyles Borins
buffer: fix dataview-set benchmark
1c410bf
Improves numbers up to 4x by avoiding repetitive dynamic method lookup.
PR-URL: #6922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@RReverserMyles Borins
buffer: fix dataview-set benchmark
7dbb0d0
Improves numbers up to 4x by avoiding repetitive dynamic method lookup.
PR-URL: #6922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

benchmarkIssues and PRs related to the benchmark subsystem.bufferIssues and PRs related to the buffer subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@RReverser@addaleax@Fishrock123@trevnorris@jasnell@mscdex@MylesBorins@nodejs-github-bot