Bug 165820 – BytecodeBasicBlock::computeImpl() should not keep iterating blocks if all jump targets have already been found.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Mark Lam]

Created attachment 297039 [details]
proposed patch.

[Mark Lam]

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]

Created attachment 297042 [details]
proposed patch.

[Saam Barati]

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 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]

Created attachment 297108 [details]
patch for landing.

[Mark Lam]

Thanks for the review.  Landed in r209820: <http://trac.webkit.org/r209820>.