Last Comment Bug 786491 - Fix v8-deltablue regression from bug 778724
: Fix v8-deltablue regression from bug 778724
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
-- normal (vote)
: mozilla18
Assigned To: general
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 778724
  Show dependency treegraph
Reported: 2012-08-28 16:17 PDT by Brian Hackett (:bhackett)
Modified: 2012-09-05 18:25 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (1.85 KB, patch)
2012-08-28 16:17 PDT, Brian Hackett (:bhackett)
luke: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Brian Hackett (:bhackett) 2012-08-28 16:17:23 PDT
Created attachment 656246 [details] [diff] [review]

dmandelin saw this in the browser and I'm able to confirm it in the shell (not sure why it didn't show earlier).  After bug 778724 v8-deltablue is no longer inlining calls to Array.push, which hurts it significantly.  This is because of a change in bug 778724 which speculatively marks the result of calls whose return value is ignored as potentially undefined, which hurts type information and breaks the Array.push fast path (which wants to see a known int32 result for the call).

This may not be the only fallout from the change, too.  If I just fix Array.push, score goes from 7200 to 9400, but if I remove the type despecialization entirely I get 9600.  (I get 9500 before bug 778724; there seem to be significantly fewer compilations post-778724).

The point of the type despecialization is to avoid recompiling large swaths of a script when it calls a function that has never been called before, adding a void type to the call site which is promptly popped.  There's a better mechanism for this now, though, used for avoiding recompilation after new overflows in (a + b) | 0 etc. operations.  This fix reverts the type despecialization change, and switches to this other mechanism to avoid any new recompilation activity in large scripts.
Comment 1 User image Brian Hackett (:bhackett) 2012-08-28 16:39:36 PDT
Comment 2 User image Brian Hackett (:bhackett) 2012-08-28 16:46:44 PDT
Actually, I'm a moron, the mechanism for (a + b) | 0 won't apply to call results as they are JOF_TYPESET opcodes and use TypeMonitorResult rather than TypeDynamicResult.  There isn't a simple fix to avoid the new recompilations, but I need to make another pass over this stuff for bug 785905 anyways and will clean things up there.
Comment 3 User image Ed Morley [:emorley] 2012-08-29 06:39:28 PDT
Comment 4 User image David Mandelin [:dmandelin] 2012-09-04 17:06:21 PDT
Luke, could you land this on the 17 branch?
Comment 5 User image Luke Wagner [:luke] 2012-09-04 18:20:50 PDT
Comment on attachment 656246 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 778724
User impact if declined: perf regression
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
Comment 7 User image David Mandelin [:dmandelin] 2012-09-05 18:25:43 PDT
(In reply to Luke Wagner [:luke] from comment #6)

Thanks again for picking up the Aurora landing.

Note You need to log in before you can comment on or make changes to this bug.