Bug 127335 - FTL should do polyvariant Call/Construct inlining
Summary: FTL should do polyvariant Call/Construct inlining
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: 127538
Blocks: 127325
  Show dependency treegraph
 
Reported: 2014-01-20 23:14 PST by Filip Pizlo
Modified: 2014-02-13 03:51 PST (History)
9 users (show)

See Also:


Attachments
work in progress (24.67 KB, patch)
2014-01-20 23:15 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
almost building (40.82 KB, patch)
2014-01-20 23:54 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it inlined something. (42.78 KB, patch)
2014-01-21 12:08 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (50.54 KB, patch)
2014-01-23 00:01 PST, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff
rebased patch (50.62 KB, patch)
2014-01-24 19:54 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (64.37 KB, patch)
2014-01-25 13:02 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2014-01-20 23:14:10 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2014-01-20 23:15:42 PST
Created attachment 221723 [details]
work in progress
Comment 2 Filip Pizlo 2014-01-20 23:54:54 PST
Created attachment 221725 [details]
almost building
Comment 3 Filip Pizlo 2014-01-21 12:08:49 PST
Created attachment 221773 [details]
it inlined something.
Comment 4 Filip Pizlo 2014-01-23 00:01:19 PST
Created attachment 221953 [details]
the patch
Comment 5 Oliver Hunt 2014-01-23 00:18:45 PST
Comment on attachment 221953 [details]
the patch

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

> Source/JavaScriptCore/jit/JITOperations.cpp:1189
> +            vm, codeBlock->newReplacement().get(), 0, DFG::DFGMode, bytecodeIndex,

nullptr here?
Comment 6 Geoffrey Garen 2014-01-23 12:13:51 PST
What's the performance result here?
Comment 7 Filip Pizlo 2014-01-23 12:53:34 PST
(In reply to comment #6)
> What's the performance result here?

None yet, since V8v7/raytrace uses op_call_varargs and the DFG bytecode parser always emits a Call node rather than going doing the handleInlining path.  That will be a separate fix.
Comment 8 Filip Pizlo 2014-01-23 20:08:38 PST
(In reply to comment #7)
> (In reply to comment #6)
> > What's the performance result here?
> 
> None yet, since V8v7/raytrace uses op_call_varargs and the DFG bytecode parser always emits a Call node rather than going doing the handleInlining path.  That will be a separate fix.

I'm actually going to implement op_call_varargs inlining before landing this.  https://bugs.webkit.org/show_bug.cgi?id=127538
Comment 9 Filip Pizlo 2014-01-24 19:54:17 PST
Created attachment 222187 [details]
rebased patch

Still testing this.  I'm seeing weird performance pathologies including a large raytrace regression despite the patch successfully leading to the right things being inlined.  I am investigating.
Comment 10 Filip Pizlo 2014-01-25 11:42:46 PST
Lol, I think we're running into pathologies here because we disallow recursive inlining and because the DFG cannot compile op_call_varargs except by way of inlining.

Without this patch, we always inline Raytrace's "constructors" but then fail to inline the initialize methods. The constructors then get to run in the DFG because they were inlined.

With this patch, we then inline the initialize methods, which may allocate some other objects, resulting in another call to the Raytrace "constructor".  But, at this point, we choose not to inline the "constructor" because we think that it's a bad idea to do recursive inlining.

I think there are a handful of issues here:

1) Our hardline prohibition against recursive inlining is probably hurting us.

2) The depth/recursion limits on inlining should never apply for functions that can only be compiled by the DFG if they are inlined.

3) op_call_varargs should be compilable by the DFG even if it ain't inlined.

Note that (3) won't actually solve any problems - if we ever have the Raytrace "constructor" execute in an uninlined context then we will have problems, since we will be running it in a monovariant context.  I think that even if op_call_varargs gets supported by the DFG in non-inlined mode, we'll want heuristics that tend to cause op_call_varargs users to get inlined.
Comment 11 Filip Pizlo 2014-01-25 12:39:32 PST
OK - I've fixed the regression.  I still don't see speed-ups on V8v7/raytrace.  There's more work to do.  I think that we'll also need polyvariant GetById to see a speed-up on this benchmark.
Comment 12 Filip Pizlo 2014-01-25 13:02:39 PST
Created attachment 222228 [details]
more

This could use further reviewage.
Comment 13 Filip Pizlo 2014-01-25 20:01:37 PST
Landed in http://trac.webkit.org/changeset/162788.
Comment 14 Csaba Osztrogonác 2014-02-13 03:51:44 PST
Comment on attachment 222228 [details]
more

Cleared review? from attachment 222228 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).