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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Stop calling NotifyAnimated (and thus repeatedly cycling layer activity) when sending OMT animations to the compositor

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).

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 7

[Brian Birtles (:birtles)]

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).

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Remove no-longer-needed flags to GetAnimationsForCompositor

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e78a6795d2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a078738f59

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But I should also work on adding an automated test for this, but that's going to involve considerable plumbing beforehand.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c8e78a6795d2
https://hg.mozilla.org/mozilla-central/rev/b3a078738f59

Comment 12

[Nicholas Nethercote [inactive]]

> 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.)

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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...