★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24744Nouvelle recherche | Portail wanayoo
Skip to content

/node

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 from

Conversation

Reviewers

@addaleax addaleax

@targos targos

@mcollina mcollina

@refack refack

@jasnell jasnell

@antsmartian antsmartian

@joyeecheung joyeecheung

@lundibundi lundibundi

@ryzokuken ryzokuken

Assignees
No one assigned
Labels
author ready events performance
Projects
None yet
Milestone
No milestone
6 participants
@BridgeAR @nodejs-github-bot @Trott @mcollina @addaleax @targos
@BridgeAR
Copy link
Member

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
@BridgeAR
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.
Unverified
The committer email address is not verified.
Learn about signing commits
Loading status checks…
6064046

@BridgeAR BridgeAR requested a review from addaleax Nov 30, 2018

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the events label Nov 30, 2018

@addaleax

addaleax reviewed Nov 30, 2018
View changes

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

This comment has been minimized.

Sign in to view
Copy link
@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.

Sign in to view
Copy link
@BridgeAR

BridgeAR Nov 30, 2018 •
edited

Author 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
fixup: improve comments
Unverified
The committer email address is not verified.
Learn about signing commits
Loading status checks…
11a6984
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 2, 2018

Ping @addaleax
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 5, 2018

This needs some reviews.
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 5, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/19238/

@BridgeAR BridgeAR requested review from antsmartian, jasnell, joyeecheung, lundibundi, mcollina, refack and ryzokuken Dec 6, 2018

@mcollina

mcollina reviewed Dec 7, 2018
View changes

Copy link
Member

left a comment

Can you do a CITGM run?
lib/events.js
// 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.

Sign in to view
Copy link
@mcollina

mcollina Dec 7, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@targos

targos Dec 7, 2018

Member

why?

This comment has been minimized.

Sign in to view
Copy link
@BridgeAR

BridgeAR Dec 7, 2018

Author Member

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

This comment has been minimized.

Sign in to view
Copy link
@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.

Sign in to view
Copy link
@BridgeAR

BridgeAR Dec 7, 2018

Author 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
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member Author

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.

Sign in to view
Copy link
Member Author

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

mcollina approved these changes Dec 14, 2018
View changes

Copy link
Member

left a comment

LGTM

@BridgeAR BridgeAR added the author ready label Dec 14, 2018

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

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

@BridgeAR
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>
Unverified
The committer email address is not verified.
Learn about signing commits
a76750b
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 19, 2018

Landed in a76750b

@BridgeAR BridgeAR closed this Dec 19, 2018

@BridgeAR BridgeAR added the performance label Dec 23, 2018

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

@BridgeAR @MylesBorins
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>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
d1a98a8

@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

@BridgeAR @refack
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>
52d7fa9

BethGriggs added a commit that referenced this pull request Apr 17, 2019

@BridgeAR @BethGriggs
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>
Verified
This commit was signed with a verified signature.
@BethGriggs BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
0d4c75b

BethGriggs added a commit that referenced this pull request Apr 28, 2019

@BridgeAR @BethGriggs
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>
Verified
This commit was signed with a verified signature.
@BethGriggs BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
98ef8a4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.