Bug 152255 - We should not employ the snippet code in the DFG if no OSR exit was previously encountered.
Summary: We should not employ the snippet code in the DFG if no OSR exit was previousl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-14 09:27 PST by Mark Lam
Modified: 2015-12-14 11:54 PST (History)
4 users (show)

See Also:


Attachments
proposed patch. (2.62 KB, patch)
2015-12-14 09:29 PST, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
x86_64 benchmark result. (66.54 KB, text/plain)
2015-12-14 09:33 PST, Mark Lam
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-12-14 09:27:05 PST
We already tried to do this but was doing it wrong.  The is what we do now:

     if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node())
         || m_graph.hasExitSite(node->origin.semantic, BadType)) {
         fixEdge<UntypedUse>(leftChild);
         fixEdge<UntypedUse>(rightChild);
         ...

The || should be a &&.  We only want to employ the snippet code if we have actually encountered an OSR exit due to untyped operands not being supported.
Comment 1 Mark Lam 2015-12-14 09:29:59 PST
Created attachment 267301 [details]
proposed patch.
Comment 2 Mark Lam 2015-12-14 09:33:12 PST
Created attachment 267302 [details]
x86_64 benchmark result.
Comment 3 Mark Lam 2015-12-14 09:33:58 PST
With this change, we see the following progression in JSRegress:

   arguments-out-of-bounds                           12.9152+-1.3205     ^     10.2297+-0.3642        ^ definitely 1.2625x faster
Comment 4 Saam Barati 2015-12-14 11:35:48 PST
Comment on attachment 267301 [details]
proposed patch.

r=me
Comment 5 Mark Lam 2015-12-14 11:41:04 PST
Thanks for the review.  Landed in r194040: <http://trac.webkit.org/r194040>.
Comment 6 Filip Pizlo 2015-12-14 11:52:59 PST
Comment on attachment 267301 [details]
proposed patch.

This is the wrong policy.  We usually only rely on exit sites as a last resort.

Why did you do this?
Comment 7 Mark Lam 2015-12-14 11:54:46 PST
(In reply to comment #6)
> Comment on attachment 267301 [details]
> proposed patch.
> 
> This is the wrong policy.  We usually only rely on exit sites as a last
> resort.
> 
> Why did you do this?

This was suggested by Geoff, and was implemented this way since the first op_sub snippet.  The only issue was that I had a bug in how I implemented it.