events: simplify stack compare function - #24744
Conversation
Sorry, something went wrong.
This simplifies the `longestSeqContainedIn()` logic by checking for the first identical occurance of at least three frames instead of the longest one. It also removes an unused argument.
|
@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1738/pipeline |
Sorry, something went wrong.
| return [len, i]; | ||
| } | ||
| if (matches) | ||
| return [ len, i, j ]; |
There was a problem hiding this comment.
And for the same reason, returning j might not be used in this specific setup, but it’s part of having this be a more generic function.
Sorry, something went wrong.
There was a problem hiding this comment.
pos translates to the former j but I recommend to change the signature when necessary and not to keep code in here that is currently unused.
Sorry, something went wrong.
|
This needs some reviews. |
Sorry, something went wrong.
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
Can you do a CITGM run?
Sorry, something went wrong.
| // Returns the length and line number of the first sequence of `a` that fully | ||
| // appears in `b` with a length of at least 4. | ||
| function identicalSequenceRange(a, b) { | ||
| for (var i = 0; i < a.length - 3; i++) { |
There was a problem hiding this comment.
I would cache a.length - 3 in a variable.
Sorry, something went wrong.
There was a problem hiding this comment.
why?
Sorry, something went wrong.
There was a problem hiding this comment.
I expect the value to be constant fold (but I did not check).
Sorry, something went wrong.
There was a problem hiding this comment.
a could be quite big, and it used to be slightly faster to cache the value if it's computed.
Sorry, something went wrong.
There was a problem hiding this comment.
a should normally be small (we currently only use this for stack frames) and this implementation should also be faster than the one before. If it's about performance, I could save a couple comparisons by using a simple for loop instead of indexOf (currently I check until the last entry but the last three entries are not interesting).
@bmeurer do values like these get constant fold?
Sorry, something went wrong.
Sorry, something went wrong.
|
@mcollina nothing showed up in CITGM (most failures are related due to some windows issues and others to removed V8 functions, the rest is also known). |
Sorry, something went wrong.
|
I know this is nothing important but it would still be great to get some reviews here. This PR is open since 14 days and there was neither a +1, nor a -1. |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
Not strictly required, but it would be great to get another review on this one. @addaleax @bnoordhuis @apapirovski @mscdex (There's not @nodejs/events team, so that list is assembled from very imperfect and superficial use of git shortlog.) |
Sorry, something went wrong.
This simplifies the `longestSeqContainedIn()` logic by checking for the first identical occurance of at least three frames instead of the longest one. It also removes an unused argument. PR-URL: nodejs#24744 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This simplifies the `longestSeqContainedIn()` logic by checking for the first identical occurance of at least three frames instead of the longest one. It also removes an unused argument. PR-URL: #24744 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This simplifies the `longestSeqContainedIn()` logic by checking for the first identical occurance of at least three frames instead of the longest one. It also removes an unused argument. PR-URL: nodejs#24744 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This simplifies the `longestSeqContainedIn()` logic by checking for the first identical occurance of at least three frames instead of the longest one. It also removes an unused argument. PR-URL: #24744 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This simplifies the `longestSeqContainedIn()` logic by checking for the first identical occurance of at least three frames instead of the longest one. It also removes an unused argument. PR-URL: #24744 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This simplifies the `longestSeqContainedIn()` logic by checking for the first identical occurance of at least three frames instead of the longest one. It also removes an unused argument. PR-URL: #24744 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This simplifies the `longestSeqContainedIn()` logic by checking for the first identical occurance of at least three frames instead of the longest one. It also removes an unused argument. PR-URL: #24744 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This simplifies the longestSeqContainedIn() logic by checking for
the first identical occurance of at least three frames instead of
the longest one.
It also removes an unused argument.
Checklist