Fix v8-deltablue regression from bug 778724

RESOLVED FIXED in Firefox 17



JavaScript Engine
5 years ago
5 years ago


(Reporter: bhackett, Unassigned)


Other Branch
Mac OS X

Firefox Tracking Flags

(firefox17+ fixed, firefox18+ fixed)


(Whiteboard: [js:p0])


(1 attachment)



5 years ago
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.
Attachment #656246 - Flags: review?(luke)


5 years ago
Attachment #656246 - Flags: review?(luke) → review+
Whiteboard: [js:p0]
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?

Comment 1

5 years ago

Comment 2

5 years ago
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.
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18


5 years ago
status-firefox18: affected → fixed


5 years ago
tracking-firefox17: ? → +
tracking-firefox18: ? → +
Luke, could you land this on the 17 branch?

Comment 5

5 years ago
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
Attachment #656246 - Flags: approval-mozilla-aurora?
Attachment #656246 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 6

5 years ago
status-firefox17: affected → fixed
(In reply to Luke Wagner [:luke] from comment #6)

Thanks again for picking up the Aurora landing.
You need to log in before you can comment on or make changes to this bug.