Skip to content

Conversation

@RReverser
Copy link
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
Copy link
Member

Choose a reason for hiding this comment

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

const?

Copy link
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.

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

Copy link
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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Updated.

@addaleax
Copy link
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
Copy link
MemberAuthor

Oh ok. Wasn't aware of that.

@addaleax
Copy link
Member

LGTM

@Fishrock123
Copy link
Contributor

cc @nodejs/buffer

@trevnorris
Copy link
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

Improves numbers up to 4x by avoiding repetitive dynamic method lookup.
@RReverser
Copy link
MemberAuthor

Changed the commit message.

@trevnorris
Copy link
Contributor

Great. LGTM.

@jasnell
Copy link
Member

LGTM
@mscdex might have some thoughts tho.

@mscdex
Copy link
Contributor

addaleax pushed a commit that referenced this pull request May 28, 2016
Improves numbers up to 4x by avoiding repetitive dynamic method lookup. PR-URL: #6922 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
@addaleax
Copy link
Member

Landed in 4a56e89 … Thanks!

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
Improves numbers up to 4x by avoiding repetitive dynamic method lookup. PR-URL: nodejs#6922 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
Improves numbers up to 4x by avoiding repetitive dynamic method lookup. PR-URL: #6922 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
@MylesBorins
Copy link
Contributor

@RReverser lts?

@RReverser
Copy link
MemberAuthor

@thealphanerd Can do. Does it require backporting PR?

@MylesBorins
Copy link
Contributor

nope landed cleanly

MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Improves numbers up to 4x by avoiding repetitive dynamic method lookup. PR-URL: #6922 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Improves numbers up to 4x by avoiding repetitive dynamic method lookup. PR-URL: #6922 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Improves numbers up to 4x by avoiding repetitive dynamic method lookup. PR-URL: #6922 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Improves numbers up to 4x by avoiding repetitive dynamic method lookup. PR-URL: #6922 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

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