Conversation
nodejs-github-bot
added
assert
benchmark/assert/deepequal-buffer.js
Outdated
There was a problem hiding this comment.
This doesn't look correct? For the not versions it needs to use expectedWrong, right?
There was a problem hiding this comment.
You are absolutely correct. While looking over the file I missed that.
benchmark/assert/deepequal-object.js
Outdated
There was a problem hiding this comment.
Could this be more similar to the change above but just assign expected/expectedWrong to a diff variable as necessary. Then the for loops, etc. don't need to be part of the switch.
Same for all of the similar use cases below.
There was a problem hiding this comment.
I don't think this change is necessary since the let is not declared within the for loop and for general usage it was optimized quite a while ago.
There was a problem hiding this comment.
I don't think this change is quite right. It means the Buffer is always the first element of strings even though it shouldn't be. I would prefer to leave this benchmark as is. The only change I can see is not allocating the empty array in strings and instead allocating it within the else branch.
There was a problem hiding this comment.
millions * 1e6 will happen on every iteration of the loop. I would prefer it assigned to a const.
There was a problem hiding this comment.
I did not check but I am pretty certain that the compiler creates a constant internally after evaluating the computation.
There was a problem hiding this comment.
I know that's sort of the case with length but I wonder in this case... ping @bmeurer
There was a problem hiding this comment.
TurboFan should sort that out, yes.
There was a problem hiding this comment.
@apapirovski I personally would like to get rid of "millions" and use "n" instead. But for now I would just stick to the way I changed it.
But I am also fine with removing that change again as it is not important at all. What do you prefer?
There was a problem hiding this comment.
Same as above. There are a few more of these in the files below too...
benchmark/path/basename-win32.js
Outdated
benchmark/tls/convertprotocols.js
Outdated
There was a problem hiding this comment.
Can we store the array in a variable in addition to this change? The results here are probably dominated by the array creation.
|
@apapirovski PTAL |
|
@BridgeAR looks like one of the assert benchmarks is broken, or if not then at the very least the test that runs them. |
BridgeAR
force-pushed
the
improve-benchmarks
branch
from
982cf6d to
3f238eb
Compare
|
Fixed New CI: https://ci.nodejs.org/job/node-test-pull-request/12766/ |
BridgeAR
added
the
author ready
It should say `win32` and not `posix`.
Due to the destructuring the outer variables were not set anymore.
BridgeAR
force-pushed
the
improve-benchmarks
branch
from
3f238eb to
f7d9fac
Compare
|
Rebased due to conflicts. New CI https://ci.nodejs.org/job/node-test-pull-request/12872/ |
|
Should this be backported to |
It should say `win32` and not `posix`. PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
Due to the destructuring the outer variables were not set anymore. PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
|
@MylesBorins looks like this one's pending a backport? I'll love to give it a shot. Also, do I still need to backport it to v6? When it landed, v6 wasn't in maintenance, but it is now. |
|
It does not need to be backported to 6. Lmk of you need help with 8 at all |
|
@MylesBorins this looks super tricky because there's a change that landed later (but got backported before), that seems to have refactored almost everything. For example, take a look at:
in here, which change do I accept? Accepting the current change looks like a nice thing to do but I'll have to manually remove the last 4 lines (is that okay?). Accepting the incoming change won't work because it needs the |
|
@ryzokuken I would suggest trying to get a result that is closest to what is upstream (in master) (if that makes sense) |
|
P.S. Do I accept both and modify the solutions (mix and match)? That doesn't sound like something that should happen without the consent of the PR owner, given that such a thing might completely go against what they initially intended. |
Only keep stuff from the PR (incoming changes) that has 0 conflicts and is absolutely necessary, you mean? |
|
@MylesBorins there are files that were removed later. Should I re-add them with the changed version? |
It should say `win32` and not `posix`. PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
Due to the destructuring the outer variables were not set anymore. PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#18320 Reviewed-By: James M Snell <jasnell@gmail.com>
8 participants
Just some refactoring of some benchmarks to reduce code overhead and improve readability.
I some times changed
letin a loop tovarbecause it may theoretically still lead to a deopt / prevent a opt.I fixed two benchmarks that regressed before and what I realized when going over all files again.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
benchmark