Bug 127335 – FTL should do polyvariant Call/Construct inlining

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 221723 [details]
work in progress

[Filip Pizlo]

Created attachment 221725 [details]
almost building

[Filip Pizlo]

Created attachment 221773 [details]
it inlined something.

[Filip Pizlo]

Created attachment 221953 [details]
the patch

[Oliver Hunt]

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?

[Geoffrey Garen]

What's the performance result here?

[Filip Pizlo]

(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.

[Filip Pizlo]

(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

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 222228 [details]
more

This could use further reviewage.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/162788.

[Csaba Osztrogonác]

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).