Bug 117385 - fourthTier: DFG should support op_in and it should use patching to make it fast
Summary: fourthTier: DFG should support op_in and it should use patching to make it fast
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-09 13:38 PDT by Filip Pizlo
Modified: 2013-06-13 13:54 PDT (History)
7 users (show)

See Also:


Attachments
very early work in progress (7.78 KB, patch)
2013-06-09 13:39 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it should just work (28.89 KB, patch)
2013-06-10 19:30 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it did just work (30.94 KB, patch)
2013-06-10 23:00 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (36.54 KB, patch)
2013-06-11 14:39 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (50.36 KB, patch)
2013-06-11 18:07 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (59.04 KB, patch)
2013-06-11 18:23 PDT, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2013-06-09 13:38:59 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2013-06-09 13:39:43 PDT
Created attachment 204122 [details]
very early work in progress

Still figuring out what the best way of doing this is.
Comment 2 Filip Pizlo 2013-06-10 19:30:56 PDT
Created attachment 204277 [details]
it should just work
Comment 3 Filip Pizlo 2013-06-10 23:00:12 PDT
Created attachment 204289 [details]
it did just work

Still need to implement 32-bit part, still need to run more tests.
Comment 4 Filip Pizlo 2013-06-11 10:58:19 PDT
Still debugging some LayoutTests/jquery crashes.  No perf numbers yet other than microbenchmarks which speed up by a *lot*.
Comment 5 Filip Pizlo 2013-06-11 14:39:08 PDT
Created attachment 204361 [details]
the patch
Comment 6 Filip Pizlo 2013-06-11 18:07:02 PDT
Created attachment 204375 [details]
the patch

32-bit now builds and runs clean.
Comment 7 Filip Pizlo 2013-06-11 18:23:32 PDT
Created attachment 204376 [details]
the patch

Added JSRegress tests.
Comment 8 Filip Pizlo 2013-06-12 12:12:05 PDT
This isn't a small speed-up.


   in-four-cases                                 297.3037+-2.8573     ^     21.8263+-0.1147        ^ definitely 13.6213x faster
   in-one-case-false                             178.0126+-2.6962     ^     10.4301+-0.1371        ^ definitely 17.0673x faster
   in-one-case-true                              156.9623+-2.0516     ^     10.1108+-0.0654        ^ definitely 15.5242x faster
   in-two-cases                                  166.7664+-1.6888     ^     10.5953+-0.1015        ^ definitely 15.7397x faster
Comment 9 Geoffrey Garen 2013-06-12 13:16:52 PDT
Comment on attachment 204376 [details]
the patch

Would be nice to take even more advantage and further optimize for cases like

    if (!("property" in object))
        throw "oops!";
    return object.property;

but I can't in good conscience thumb my nose at a 17X speedup.
Comment 10 Filip Pizlo 2013-06-13 09:59:29 PDT
(In reply to comment #9)
> (From update of attachment 204376 [details])
> Would be nice to take even more advantage and further optimize for cases like
> 
>     if (!("property" in object))
>         throw "oops!";
>     return object.property;

Yes.  Here's the plan:

1) Get the baseline JIT to use the DFG's inline caching logic, including for op_in.

2) Have the DFGByteCodeParser extract the inline cache data for op_in the same way it does for op_get_by_id and friends.

3a) For In's that only have one case in the inline cache and they didn't take slow path, simply turn them into CheckStructure and their result becomes a JSConstant(true) or JSConstant(false).

3b) For In's that too two cases (a true and a false case), integrate Branch(In(@object)) with the CFA such that the CFA will remember that on the then case of the Branch, we know that @object has the true structure, and on the else case of the Branch, we know that @object has the false structure.  We haven't yet done such things in the CFA, but they are easy to do in our IR; they'll just require a slight refiddling of the relationship between the CFA's executeEffects() method and the mergeXYZ() methods; executeEffects() will have to convey to the merge methods that we will want to merge slightly different data for the different paths of the branch.

> 
> but I can't in good conscience thumb my nose at a 17X speedup.

Seems sensible! ;-)

OTOH, this didn't really speed up pdfjs despite increasing coverage, and despite the fact that pdfjs is clearly making good use of the inline caching - disassembly dumps show lots of juicy DFG In stubs, and most of them look optimal.  So I guess those functions that used 'in' simply weren't on the hot path.  Oh well it happens.  Still happy that I made it fast.
Comment 11 Filip Pizlo 2013-06-13 13:54:57 PDT
Landed in http://trac.webkit.org/changeset/151569