events: simplify stack compare function#24744
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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.
nodejs-github-bot added the events 
Uh oh!
There was an error while loading. Please reload this page.
| 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.
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.
Uh oh!
There was an error while loading. Please reload this page.
BridgeAR commented Dec 2, 2018
Ping @addaleax |
BridgeAR commented Dec 5, 2018
This needs some reviews. |
BridgeAR commented Dec 5, 2018
BridgeAR requested review from antsmartian, jasnell, joyeecheung, lundibundi, mcollina, refack and ryzokuken
| // Returns the length and line number of the first sequence of `a` that fully | ||
| // appears in `b` with a length of at least 4. | ||
| functionidenticalSequenceRange(a,b){ | ||
| for(vari=0;i<a.length-3;i++){ |
There was a problem hiding this comment.
I would cache a.length - 3 in a variable.
There was a problem hiding this comment.
I expect the value to be constant fold (but I did not check).
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.
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?
BridgeAR commented Dec 14, 2018
@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). |
BridgeAR commented Dec 14, 2018
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. |
BridgeAR added the author ready 
Trott commented Dec 16, 2018
Not strictly required, but it would be great to get another review on this one. @addaleax@bnoordhuis@apapirovski@mscdex (There's not |
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 <[email protected]>
BridgeAR commented Dec 19, 2018
Landed in a76750b |
BridgeAR added the performance 
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
6 participants
This simplifies the
longestSeqContainedIn()logic by checking forthe first identical occurance of at least three frames instead of
the longest one.
It also removes an unused argument.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes