New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: simplify stack compare function #24744

Closed
wants to merge 2 commits into
base: master
from

Conversation

@BridgeAR
Copy link
Member

BridgeAR commented Nov 30, 2018

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
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
events: simplify stack compare function
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 BridgeAR requested a review from addaleax Nov 30, 2018

Show resolved Hide resolved lib/events.js Outdated
}
if (matches)
return [ len, i, j ];

This comment has been minimized.

@addaleax

addaleax Nov 30, 2018

Member

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.

This comment has been minimized.

@BridgeAR

BridgeAR Nov 30, 2018

Member

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.

Show resolved Hide resolved lib/events.js
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 2, 2018

Ping @addaleax

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 5, 2018

This needs some reviews.

@BridgeAR

This comment has been minimized.

@mcollina
Copy link
Member

mcollina left a comment

Can you do a CITGM run?

// 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++) {

This comment has been minimized.

@mcollina

mcollina Dec 7, 2018

Member

I would cache a.length - 3 in a variable.

This comment has been minimized.

@targos

targos Dec 7, 2018

Member

why?

This comment has been minimized.

@BridgeAR

BridgeAR Dec 7, 2018

Member

I expect the value to be constant fold (but I did not check).

This comment has been minimized.

@mcollina

mcollina Dec 7, 2018

Member

a could be quite big, and it used to be slightly faster to cache the value if it's computed.

This comment has been minimized.

@BridgeAR

BridgeAR Dec 7, 2018

Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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.

@mcollina
Copy link
Member

mcollina left a comment

LGTM

@Trott

This comment has been minimized.

Copy link
Member

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 @nodejs/events team, so that list is assembled from very imperfect and superficial use of git shortlog.)

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 19, 2018

events: simplify stack compare function
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>
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 19, 2018

Landed in a76750b

@BridgeAR BridgeAR closed this Dec 19, 2018

MylesBorins added a commit that referenced this pull request Dec 25, 2018

events: simplify stack compare function
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>

@MylesBorins MylesBorins referenced this pull request Dec 25, 2018

Merged

v11.6.0 proposal #25175

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

events: simplify stack compare function
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment