buffer: Fix dataview-set benchmark. #6922
Conversation
nodejs-github-bot
added
the
benchmark
| @@ -41,17 +41,19 @@ function main(conf) { | |||
|
|
|||
| function benchInt(dv, fn, len, le) { | |||
| var m = mod[fn]; | |||
| var method = dv[fn]; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
Well, if changing only those to const and not touching var i -> let i, then it looks fine, within the limtis.
|
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. :) |
|
Oh ok. Wasn't aware of that. |
|
LGTM |
|
cc @nodejs/buffer |
|
nits: git message title, don't cap "Fix" and don't end with period. In git message body want to place URL's in 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.
|
Changed the commit message. |
|
Great. LGTM. |
|
LGTM |
|
lint passes: https://ci.nodejs.org/job/node-test-linter/2672/ LGTM |
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>
|
Landed in 4a56e89 … Thanks! |
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>
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>
|
@RReverser lts? |
|
@thealphanerd Can do. Does it require backporting PR? |
|
nope landed cleanly |
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>
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>
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>
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>
Checklist
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:
After the change:
(Just noticed this when going through #6893 and couldn't pass by.)