Bug 165820

Summary: BytecodeBasicBlock::computeImpl() should not keep iterating blocks if all jump targets have already been found.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, jfbastien, keith_miller, msaboff, saam
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
mark.lam: review-
proposed patch.
saam: review+
patch for landing. none

Mark Lam
Reported 2016-12-13 13:52:13 PST
Currently, if an opcode is a branch type opcode. BytecodeBasicBlock::computeImpl() will iterate over all basic blocks looking for the block containing the jump target, and it will continue to do this even when all the jump targets have been found. This is wasted work, and all the more so given that most branch type opcodes only have a single jump target.
Attachments
proposed patch. (2.38 KB, patch)
2016-12-13 13:55 PST, Mark Lam 		mark.lam: review-
DetailsFormatted DiffDiff
proposed patch. (2.81 KB, patch)
2016-12-13 14:52 PST, Mark Lam 		saam: review+
DetailsFormatted DiffDiff
patch for landing. (2.87 KB, patch)
2016-12-14 11:57 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2016-12-13 13:55:35 PST
Created attachment 297039 [details] proposed patch.
Mark Lam
Comment 2 2016-12-13 13:58:52 PST
Comment on attachment 297039 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=297039&action=review > Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:163 > + ASSERT(!numberOfJumpTargets); This assertion is firing. I will need to check why.
Mark Lam
Comment 3 2016-12-13 14:52:01 PST
Created attachment 297042 [details] proposed patch.
Saam Barati
Comment 4 2016-12-14 11:45:30 PST
Comment on attachment 297042 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=297042&action=review > Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:160 > + --numberOfJumpTargets; Style suggestion: Instead of having the numberOfJumpTarget inside the "if" statement's conditional, you could instead check "if (!numberOfJumpTargets) break" right here.
Mark Lam
Comment 5 2016-12-14 11:47:25 PST
Comment on attachment 297042 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=297042&action=review >> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:160 >> + --numberOfJumpTargets; > > Style suggestion: Instead of having the numberOfJumpTarget inside the "if" statement's conditional, you could instead check "if (!numberOfJumpTargets) break" right here. I agree. That also makes it such that we only test numberOfJumpTarget if we've found a jump target instead of on every iteration. I'll change the code to do this before landing.
Mark Lam
Comment 6 2016-12-14 11:57:05 PST
Created attachment 297108 [details] patch for landing.
Mark Lam
Comment 7 2016-12-14 12:00:02 PST
Thanks for the review. Landed in r209820: <http://trac.webkit.org/r209820>.
Note You need to log in before you can comment on or make changes to this bug.