Closed Bug 753630 Opened 13 years ago Closed 12 years ago

Beef up animation detection heuristics for code discarding

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16
blocking-kilimanjaro +

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

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

Attachments

(2 files)

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]
It might be good to coordinate with the heuristic in bug 712478.
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.
(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
Attached patch patch (deleted) — Splinter Review
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+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Depends on: 839631
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: