Bug 82986 – jsr/sret should be removed

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

[Filip Pizlo]

And replaced by inlining of finally blocks wherever we would have done jsr.

That makes the code easier to compile, should have no effect on performance or memory usage, and should allow us to kill off a lot of code.

Not to mention, it will preempt any bugs from OSR during execution of a finally block.  LLInt uses the virtual PC for the subroutine return, while the JIT uses the machine PC.

[Filip Pizlo]

Created attachment 135240 [details]
the patch

[Filip Pizlo]

Comment on attachment 135240 [details]
the patch

Reviewed by Geoff in person.

[Filip Pizlo]

Created attachment 135495 [details]
the patch

The previous patch failed to save/restore the correct state for finally inlining.  For example:

try {
   switch (foo) {
   case blah:
      return bar;
   }
} finally {
    break;
}

Since the finally block was emitted inline in the return, the break would break us out of the switch.

Other similarly horrible things would happen with scope depth and the finally depth, etc.

[Geoffrey Garen]

Comment on attachment 135495 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135495&action=review

emitComplexJumpScopes is maybe not my favorite function.

r=me

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2177
> +                while (m_labelScopes.size() > finallyContext.labelScopesSize)
> +                    m_labelScopes.removeLast();

m_labelScopes.shrink(finallyContext.labelScopesSize) should suffice here.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 135495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135495&action=review
> 
> emitComplexJumpScopes is maybe not my favorite function.
> 
> r=me
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2177
> > +                while (m_labelScopes.size() > finallyContext.labelScopesSize)
> > +                    m_labelScopes.removeLast();
> 
> m_labelScopes.shrink(finallyContext.labelScopesSize) should suffice here.

SegmentedVector is shrinkless!

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/113136

[Yong Li]

This commit gives >4% penalty on sunspider for me, tested on QNX without DFG turned on. Any idea?

[Filip Pizlo]

(In reply to comment #7)
> This commit gives >4% penalty on sunspider for me, tested on QNX without DFG turned on. Any idea?

I don't remember this being a regression. I will retest.

[Filip Pizlo]

(In reply to comment #8)
> (In reply to comment #7)
> > This commit gives >4% penalty on sunspider for me, tested on QNX without DFG turned on. Any idea?
> 
> I don't remember this being a regression. I will retest.

I reran all benchmarks on this change set.

There is no difference for me.