Bug 127335

Created attachment 221723 [details] work in progress

Created attachment 221725 [details] almost building

Created attachment 221773 [details] it inlined something.

Created attachment 221953 [details] the patch

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?

What's the performance result here?

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

(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

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.

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.

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.

Created attachment 222228 [details] more This could use further reviewage.

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

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