Closed Bug 1149848 Opened 5 years ago Closed 5 years ago

repeated layer activity cycles due to throttled off-main-thread animations

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: dbaron, Assigned: dbaron, NeedInfo)

References

(Depends on 1 open bug)

Details

(Keywords: perf, power)

Attachments

(3 files)

I've been poking at what we're currently doing on the main thread to check that off-main-thread animations work as I expect them to.

One of the things I noticed is that it looks like we're doing a lot more main thread painting than we should be because of how we poke the ActiveLayerTracker.  In particular, we seem to paint every 75-100ms (the activity timeout) because of a cycle where:

1.  when we do painting, CommonAnimationManager::GetAnimationsForCompositor (which effectively has only one caller, nsDisplayListBuilder::AddAnimationsAndTransitionsToLayer) calls ActiveLayerTracker::NotifyAnimated

2.  for a while (75-100ms), we happily run the animation on the compositor thread without doing any style or painting on the main thread

3.  but then, we call LayerActivityTracker::NotifyExpired, which calls nsIFrame::SchedulePaint, which makes us do a paint, which sends us back to (1)


I feel like we've talked about this problem before, but I can't find any history of it.

In any case, it's not clear to me how the ActiveLayerTracker is intended to work.  We *could* add calls to ActiveLayerTracker::NotifyAnimated for animations that are throttled, but that adds more main thread work that seems vaguely unnecessary.  It's probably not a big deal in reality, although it might interfere with trying to reduce the amount of main-thread stuff we do when we have throttled animations.  That seems simple, and maybe it's the right thing.

That said, it's not clear to me how NotifyAnimated is supposed to interact with IsStyleAnimated.  Should it?  Is there something smarter we should be doing here?

(Bug 1046055 touched some similar areas of code and is vaguely related.)
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #0)
> In any case, it's not clear to me how the ActiveLayerTracker is intended to
> work.  We *could* add calls to ActiveLayerTracker::NotifyAnimated for
> animations that are throttled, but that adds more main thread work that
> seems vaguely unnecessary.  It's probably not a big deal in reality,
> although it might interfere with trying to reduce the amount of main-thread
> stuff we do when we have throttled animations.  That seems simple, and maybe
> it's the right thing.

That is, around where we set didThrottle to true in nsAnimationManager::FlushAnimations and nsTransitionManager::FlushTransitions, we could make the NotifyAnimated call for the appropriate properties.
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #0)
> I feel like we've talked about this problem before, but I can't find any
> history of it.

I don't remember talking about this before.

> In any case, it's not clear to me how the ActiveLayerTracker is intended to
> work.  We *could* add calls to ActiveLayerTracker::NotifyAnimated for
> animations that are throttled, but that adds more main thread work that
> seems vaguely unnecessary.  It's probably not a big deal in reality,
> although it might interfere with trying to reduce the amount of main-thread
> stuff we do when we have throttled animations.  That seems simple, and maybe
> it's the right thing.
> 
> That said, it's not clear to me how NotifyAnimated is supposed to interact
> with IsStyleAnimated.  Should it?  Is there something smarter we should be
> doing here?
> 
> (Bug 1046055 touched some similar areas of code and is vaguely related.)

NotifyAnimated is documented to time out after a short period of time. Thus, I don't think CommonAnimationManager::GetAnimationsForCompositor should be calling it.

It seems to me that if we just remove that call, everything should be fine, since ActiveLayerTracker::IsStyleAnimated already checks nsLayoutUtils::HasCurrentAnimationsForProperty, so should return true for these properties regardless of whether NotifyAnimated is being called.
I checked in gdb that with the patch, we're no longer calling
PresShell::Paint repeatedly on the testcase (whereas we are without the
patch).
Attachment #8586511 - Flags: review?(bbirtles)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Though assuming you're ok with that patch, it should now also mean reverting much (the flags addition) of:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bec9324b555
And I should really also write a test for this, which might mean making manual calls to the ActiveLayerTracker's nsExpirationTracker::AgeOneGeneration from nsDOMWindowUtils::AdvanceTimeAndRefresh, and adding paint counts to nsDOMWindowUtils (alongside framesConstructed and framesReflowed, although probably not as a count of frames).
Flags: in-testsuite?
Comment on attachment 8586511 [details] [diff] [review]
Stop calling NotifyAnimated (and thus repeatedly cycling layer activity) when sending OMT animations to the compositor

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #5)
> Though assuming you're ok with that patch, it should now also mean reverting
> much (the flags addition) of:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6bec9324b555

Yes, by all means, please get rid of that flag (and the whole GetCompositorAnimationOptions enum).
Attachment #8586511 - Flags: review?(bbirtles) → review+
This reverts all of bug 1109390 part 20 (except for a whitespace change
and a comment removal that patch 1 made irrelevant) and some of part 21.
Attachment #8588669 - Flags: review?(bbirtles)
Attachment #8588669 - Flags: review?(bbirtles) → review+
But I should also work on adding an automated test for this, but that's going to involve considerable plumbing beforehand.
Flags: needinfo?(dbaron)
https://hg.mozilla.org/mozilla-central/rev/c8e78a6795d2
https://hg.mozilla.org/mozilla-central/rev/b3a078738f59
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
> One of the things I noticed is that it looks like we're doing a lot more
> main thread painting than we should be because of how we poke the
> ActiveLayerTracker.  In particular, we seem to paint every 75-100ms (the
> activity timeout) because of a cycle where:

Drive-by comment... LayerActivityTracker looks like this:

> class LayerActivityTracker final : public nsExpirationTracker<LayerActivity,4> {
> public:
>   // 75-100ms is a good timeout period. We use 4 generations of 25ms each.
>   enum { GENERATION_MS = 100 };
>   LayerActivityTracker()
>     : nsExpirationTracker<LayerActivity,4>(GENERATION_MS) {}

AIUI the "4 generations of 25ms each" comment is bogus. This code actually gives 4 generations of 100ms each, which in practice gives lack-of-use timeouts in the 300--400ms range.

(I found this while looking at bug 1190722, which has lots of wakeups due to LayerActivityTracker.)
(In reply to Nicholas Nethercote [:njn] from comment #12)
> AIUI the "4 generations of 25ms each" comment is bogus. This code actually
> gives 4 generations of 100ms each, which in practice gives lack-of-use
> timeouts in the 300--400ms range.

Yeah, I'd noticed that before; I think we need to fix the API, since I don't think this is the only misuse...
Depends on: 1229283
You need to log in before you can comment on or make changes to this bug.