FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: Fix dataview-set benchmark. by RReverser · Pull Request #6922 · nodejs/node · GitHub

/ node Public

buffer: Fix dataview-set benchmark. - #6922

Closed
RReverser wants to merge 1 commit into
nodejs:masterfrom
RReverser:buffer-dataview-bench
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

RReverser 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-bot added the benchmark Issues and PRs related to the benchmark subsystem. label May 22, 2016
addaleax added the buffer Issues and PRs related to the buffer subsystem. label May 22, 2016
addaleax reviewed May 22, 2016
View reviewed changes
Comment thread benchmark/buffers/dataview-set.js Outdated

addaleax May 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

const?

RReverser May 22, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

addaleax May 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

RReverser May 22, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

RReverser May 22, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Updated.

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 commented May 22, 2016

Copy link
Copy Markdown
Member Author

Oh ok. Wasn't aware of that.

addaleax commented May 22, 2016

Copy link
Copy Markdown
Member

LGTM

Fishrock123 commented May 23, 2016

Copy link
Copy Markdown
Contributor

cc @nodejs/buffer

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

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

RReverser commented May 25, 2016

Copy link
Copy Markdown
Member Author

Changed the commit message.

trevnorris commented May 26, 2016

Copy link
Copy Markdown
Contributor

Great. LGTM.

jasnell commented May 26, 2016

Copy link
Copy Markdown
Member

LGTM
@mscdex might have some thoughts tho.

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
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 commented May 28, 2016

Copy link
Copy Markdown
Member

Landed in 4a56e89 … Thanks!

addaleax closed this May 28, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
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
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>
MylesBorins mentioned this pull request Jul 11, 2016
Closed

MylesBorins commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

@RReverser lts?

RReverser commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd Can do. Does it require backporting PR?

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
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
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>
MylesBorins mentioned this pull request Jul 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

benchmark Issues and PRs related to the benchmark subsystem. buffer Issues 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

Back | FazBrowse Home | New Git URL