Beef up animation detection heuristics for code discarding

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Other Branch
mozilla16
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+)

Details

(Whiteboard: [js:p1:fx16][k9o:p1:fx16])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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).
Can we enable eviction when no JS has run for one second?
Alternatively, can we somehow set a flag in a compartment on invocation of setTimeout with an interval of less than, say, 100ms?
blocking-kilimanjaro: --- → +
Whiteboard: [k9o:p1:fx15]

Comment 3

5 years ago
It might be good to coordinate with the heuristic in bug 712478.
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.
(Assignee)

Comment 5

5 years ago
(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).
> 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.
Whiteboard: [k9o:p1:fx15] → [k9o:p1:fx15][js:p1:fx15]
Whiteboard: [k9o:p1:fx15][js:p1:fx15] → [k9o:p1:fx15][js:p1:fx16]
Whiteboard: [k9o:p1:fx15][js:p1:fx16] → [js:p1:fx16][k9o:p1:fx15]
Whiteboard: [js:p1:fx16][k9o:p1:fx15] → [js:p1:fx16][k9o:p1:fx16]
Brian's been meaning to take this--he says he'll be able to get onto it by the end of next week.
Assignee: general → bhackett1024
(Assignee)

Comment 8

5 years ago
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.
Attachment #637327 - Flags: review?(dmandelin)
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.
Attachment #637327 - Flags: feedback+
Comment on attachment 637327 [details] [diff] [review]
patch

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

Beautifully simple.
Attachment #637327 - Flags: review?(dmandelin) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c20d4ba61a
https://hg.mozilla.org/mozilla-central/rev/e8c20d4ba61a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16

Updated

5 years ago
Depends on: 839631
You need to log in before you can comment on or make changes to this bug.