Last Comment Bug 753630 - Beef up animation detection heuristics for code discarding
: Beef up animation detection heuristics for code discarding
Status: RESOLVED FIXED
[js:p1:fx16][k9o:p1:fx16]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Brian Hackett (:bhackett)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 839631
Blocks: 750834
  Show dependency treegraph
 
Reported: 2012-05-09 20:04 PDT by Brian Hackett (:bhackett)
Modified: 2013-02-10 06:37 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Spinning Balls benchmark, modified to be setTimeout-based (7.46 KB, application/zip)
2012-05-16 14:06 PDT, Till Schneidereit [till] (pto until Dec 5)
no flags Details
patch (1.19 KB, patch)
2012-06-27 17:39 PDT, Brian Hackett (:bhackett)
dmandelin: review+
roc: feedback+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2012-05-09 20:04:33 PDT
The heuristics for deciding whether to discard jitcode on GC are currently tied to calls to window.requestAnimationFrame.  Per bug 750834 comment 20 most websites which play animations don't use this API, but rather use setTimeout to control animation timing.  The heuristics should be improved to capture these websites as well, while retaining a low false positive rate (we don't want to treat websites as playing animations if they actually aren't, as then extra analysis info and jitcode may be kept around longer than necessary).
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 20:11:29 PDT
Can we enable eviction when no JS has run for one second?
Comment 2 Till Schneidereit [till] (pto until Dec 5) 2012-05-10 07:08:02 PDT
Alternatively, can we somehow set a flag in a compartment on invocation of setTimeout with an interval of less than, say, 100ms?
Comment 3 Jesse Ruderman 2012-05-15 23:43:32 PDT
It might be good to coordinate with the heuristic in bug 712478.
Comment 4 Till Schneidereit [till] (pto until Dec 5) 2012-05-16 14:06:37 PDT
Created attachment 624526 [details]
Spinning Balls benchmark, modified to be setTimeout-based

Just as a quick data point: for the attached setTimeout-based version of the Spinning Balls benchmark, the animation heuristics already seem to be working. In no GC do I get Discard Code totals longer than 0.3ms.
Comment 5 Brian Hackett (:bhackett) 2012-05-16 16:42:22 PDT
(In reply to Till Schneidereit [:till] from comment #4)
> Created attachment 624526 [details]
> Spinning Balls benchmark, modified to be setTimeout-based
> 
> Just as a quick data point: for the attached setTimeout-based version of the
> Spinning Balls benchmark, the animation heuristics already seem to be
> working. In no GC do I get Discard Code totals longer than 0.3ms.

Code discarding only really affects things when the animation/website runs a lot of code which would normally need to be reanalyzed and recompiled after each GC.  The spinning balls benchmark is a reskinned v8-splay, which has a very tiny amount of code but allocates a ton of objects (so that scanning for singleton objects when preserving code is expensive).
Comment 6 Till Schneidereit [till] (pto until Dec 5) 2012-05-16 16:47:45 PDT
> Code discarding only really affects things when the animation/website runs a
> lot of code which would normally need to be reanalyzed and recompiled after
> each GC.  The spinning balls benchmark is a reskinned v8-splay, which has a
> very tiny amount of code but allocates a ton of objects (so that scanning
> for singleton objects when preserving code is expensive).

I just started figuring out as much by profiling what the vm is doing. Thanks for the explanation - that saves me a lot of further digging.
Comment 7 David Mandelin [:dmandelin] 2012-06-22 16:15:10 PDT
Brian's been meaning to take this--he says he'll be able to get onto it by the end of next week.
Comment 8 Brian Hackett (:bhackett) 2012-06-27 17:39:11 PDT
Created attachment 637327 [details] [diff] [review]
patch

Apologies for the delay here, it's taken some time to find a heuristic that I liked.  I tried out the suggestions in comment 1 and 2, but these and similar things seem to trigger on many websites which don't use animations.  Now, keeping code around on these sites might not be bad but I'm not aware of any evidence that recompilation costs hurt outside of animations, and would rather not overreach here (avoiding code discarding unnecessarily will increase memory usage).

This heuristic just treats invalidation of all or part of a canvas element as animation activity, in the same way as calling mozRequestAnimationFrame.  This will only trip on pages that are updating a canvas element, and doesn't seem to trip on pages that are in the background.  Additionally, there are already throttling mechanisms to avoid excessive canvas invalidations, so the overhead of notifying here (mainly a gettimeofday) should be small.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-27 19:02:14 PDT
Comment on attachment 637327 [details] [diff] [review]
patch

Looks good. Just put {} around NotifyAnimationActivity.

There will be some kinds of animations not caught, because not everything uses canvas, but this will help.
Comment 10 David Mandelin [:dmandelin] 2012-06-29 14:26:16 PDT
Comment on attachment 637327 [details] [diff] [review]
patch

Review of attachment 637327 [details] [diff] [review]:
-----------------------------------------------------------------

Beautifully simple.
Comment 11 Brian Hackett (:bhackett) 2012-06-29 14:35:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c20d4ba61a
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-06-30 12:43:19 PDT
https://hg.mozilla.org/mozilla-central/rev/e8c20d4ba61a

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