Summary: | Jettison DFG code when neither breakpoints nor the profiler are active. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | fpizlo, ggaren, joepeck, mhahnenberg, msaboff, oliver, timothy | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Mark Lam
2014-01-28 00:47:57 PST
Created attachment 222418 [details]
the patch.
This patch has passed the layout tests.
Created attachment 222419 [details]
patch 2: fixed a bug in the test and test expectations.
Comment on attachment 222419 [details]
patch 2: fixed a bug in the test and test expectations.
r=me
Thanks for the review. Landed in r162940: <http://trac.webkit.org/r162940>. 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. 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. (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) |