Bug 127766 – Jettison DFG code when neither breakpoints nor the profiler are active.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Instead of using speculation checks, we can just jettison the DFG CodeBlock if there are breakpoints set in it, or if the profiler is active.  This way, we can avoid the overhead of the speculation checks when neither breakpoints nor the profiler are active.

This patch improves the Octane score from ~2950 to ~3067.

[Mark Lam]

Created attachment 222418 [details]
the patch.

This patch has passed the layout tests.

[Mark Lam]

Created attachment 222419 [details]
patch 2: fixed a bug in the test and test expectations.

[Geoffrey Garen]

Comment on attachment 222419 [details]
patch 2: fixed a bug in the test and test expectations.

r=me

[Mark Lam]

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

[Filip Pizlo]

Comment on attachment 222419 [details]
patch 2: fixed a bug in the test and test expectations.

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

> Source/JavaScriptCore/dfg/DFGClobberize.h:93
> +    case Breakpoint:
> +    case ProfileWillCall:
> +    case ProfileDidCall:

This breaks the FTL so hard.  You're claiming that these three nodes have no effects, which means that they can be hoisted, moved, and CSE'd.  That causes madness and it causes LayoutTest failures.

Basically, if you add a new node, and that node can't be moved, then DFG::clobberize() must see the node as modifying some state.  You'll notice that there are a bunch of nodes that modify "SideState" which is our way of saying that they modify something not visible to the JS heap.  That means that they cannot be moved/CSEd, but it also doesn't inhibit optimizations of any other nodes.

> Source/JavaScriptCore/dfg/DFGClobberize.h:-623
> -    case Breakpoint:
> -    case ProfileWillCall:
> -    case ProfileDidCall:

It would have been far better to leave them here.  That would have avoided the bug.

[Mark Lam]

Comment on attachment 222419 [details]
patch 2: fixed a bug in the test and test expectations.

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

>> Source/JavaScriptCore/dfg/DFGClobberize.h:93
>> +    case ProfileDidCall:
> 
> This breaks the FTL so hard.  You're claiming that these three nodes have no effects, which means that they can be hoisted, moved, and CSE'd.  That causes madness and it causes LayoutTest failures.
> 
> Basically, if you add a new node, and that node can't be moved, then DFG::clobberize() must see the node as modifying some state.  You'll notice that there are a bunch of nodes that modify "SideState" which is our way of saying that they modify something not visible to the JS heap.  That means that they cannot be moved/CSEd, but it also doesn't inhibit optimizations of any other nodes.

Thanks for the clarification of what “SideState" means.  Perhaps that should have been documented in the comments in this function, or it should have been named “HasSideEffects” instead.

[Filip Pizlo]

(In reply to comment #6)
> (From update of attachment 222419 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222419&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGClobberize.h:93
> >> +    case ProfileDidCall:
> > 
> > This breaks the FTL so hard.  You're claiming that these three nodes have no effects, which means that they can be hoisted, moved, and CSE'd.  That causes madness and it causes LayoutTest failures.
> > 
> > Basically, if you add a new node, and that node can't be moved, then DFG::clobberize() must see the node as modifying some state.  You'll notice that there are a bunch of nodes that modify "SideState" which is our way of saying that they modify something not visible to the JS heap.  That means that they cannot be moved/CSEd, but it also doesn't inhibit optimizations of any other nodes.
> 
> Thanks for the clarification of what “SideState" means.  Perhaps that should have been documented in the comments in this function, or it should have been named “HasSideEffects” instead.

It doesn't mean HasSideEffects.  All of the possible abstract heaps involve side effects and saying anything like "write(FooBar)" in clobberize() means that the node has side effects.

The SideState abstract heap is clearly documented in DFGAbstractHeap.h:

    /* Use this for writes only, just to indicate that hoisting the node is invalid. This works because we don't hoist anything that has any side effects at all. */\
    macro(SideState)