854421 - Throttle requestAnimationFrame from compositor

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

With bug 829747 landing, we can now block in SurfaceStream::WaitForCompositor() if WebGL is overproducing. This is not a great situation, because instead of waiting on that mutex we could spend time doing other things (GC, other timeouts, whatever). What we should do instead is just not fire another requestAnimationFrame if we already have an unprocessed frame pending.

I am not sure exactly how the details would work out, though. Maybe when we send a frame to the compositor we call something that prohibits RAF. When the compositor then picks that frame up, it sends something back to Gecko that allows RAF and fires one immediately if we are past due.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Where by "rAF" you mean "anything off the refresh driver", right?  Or is this specific to rAF for some reason?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> Where by "rAF" you mean "anything off the refresh driver", right?  Or is
> this specific to rAF for some reason?

Hmm, what else is driven by the refresh driver? I call out rAF specifically because it's used for animations. A generic timeout might not care about what the compositor is doing.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The refresh driver drives restyling, reflow, CSS animations, CSS transitions, SMIL animations, and rAF callbacks.  Plus a few minor things like scrolling iirc.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, and invalidation.  Not sure about painting yet, but eventually also painting.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> The refresh driver drives restyling, reflow, CSS animations, CSS
> transitions, SMIL animations, and rAF callbacks.  Plus a few minor things
> like scrolling iirc.

Oh, wow. I think we probably only want to do it for rAF then? Maybe scrolling too, not sure. We only need it when we are doing an asynchronous layer transaction. Right now that's only thebes and webgl.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, so once painting happens off the refresh driver it would also need this throttling?

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> OK, so once painting happens off the refresh driver it would also need this
> throttling?

Maybe. I should've added that Thebes layers are only async when using the tiling thing, AFAIK, and that might only be used on Android/B2G. Benoit?

Comment 8

[Benoit Girard (:BenWa)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #7)
> (In reply to Boris Zbarsky (:bz) from comment #6)
> > OK, so once painting happens off the refresh driver it would also need this
> > throttling?
> 
> Maybe. I should've added that Thebes layers are only async when using the
> tiling thing, AFAIK, and that might only be used on Android/B2G. Benoit?

IMO it would be nice to have a pseudo-event loop type thing or a list of things we can run during idle time that are interruptible/granular and use it throughout Gecko. It would need to be something that is safe to run when things are on the stack (anything that can release references/destroy stuff that is live and on the stack for example is dangerous). GCs would be the primary thing.

We could run that until the previous transaction is consumed by the compositor to prevent having the main thread get ahead by more then one frame, like most OS implement their SwapBuffers.

Comment 9

[Matt Woodrow (:mattwoodrow)]

I'll take this.

What I want to do is have the refresh driver issue a 'transaction id' that we pass to the compositor every time we send a transaction. Whenever the compositor composites, it will returns the highest transaction id it has back to the refresh driver (since multiple transactions might be coalesced into one composite).

Then we can throttle the refresh driver to make sure the most recently issued transaction id doesn't get too far ahead of the most recently completed transaction id.

Comment 10

[Matt Woodrow (:mattwoodrow)]

That might be something like:

If we try to run a refresh driver tick, and there are two outstanding transactions, then skip that tick.

Once a transactions come in (and we've marked ourselves as having skipped a tick), then schedule a refresh driver tick immediately (rather than waiting whatever remains of the 16ms).

I guess we might want some sort of time cap in there as a fallback, just in case a transaction gets lost. Not ideal, but it seems preferable to hanging the refresh driver indefinitely.

Comment 11

[Benoit Girard (:BenWa)]

Plans sounds good, we can run JS and GC while waiting for the compositor this way

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> I guess we might want some sort of time cap in there as a fallback, just in
> case a transaction gets lost. Not ideal, but it seems preferable to hanging
> the refresh driver indefinitely.

Why? If the compositor isn't catching up then something serious is happening and pushing more transaction its way will hurt it.

Comment 12

[Matt Woodrow (:mattwoodrow)]

> (In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> > I guess we might want some sort of time cap in there as a fallback, just in
> > case a transaction gets lost. Not ideal, but it seems preferable to hanging
> > the refresh driver indefinitely.
> 
> Why? If the compositor isn't catching up then something serious is happening
> and pushing more transaction its way will hurt it.

I meant the broken case, where the compositor never sends the appropriate reply and the refresh driver hangs forever waiting for something that isn't coming.

Obviously it's better to just not have that bug, but a safety net might be something to consider.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> That might be something like:
> 
> If we try to run a refresh driver tick, and there are two outstanding
> transactions, then skip that tick.
> 
> Once a transactions come in (and we've marked ourselves as having skipped a
> tick), then schedule a refresh driver tick immediately (rather than waiting
> whatever remains of the 16ms).
> 
> I guess we might want some sort of time cap in there as a fallback, just in
> case a transaction gets lost. Not ideal, but it seems preferable to hanging
> the refresh driver indefinitely.

This could work, but I think it might be a little overkill for what we're doing here. We don't need to be exact, we just need the refresh driver to have a decent idea of when to schedule. I worry about the IPC overhead of a plan like this.

I was thinking we could just have the compositor send a message containing the time it took to composite the last 60 frames (or something). The refresh driver could then use that to come up with its own target framerate. This would be a lot less IPC traffic, but it also means we couldn't adjust to a single long frame thrown in there.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

So I hacked up my idea, but it has some problems.

If you time from Compositor::StartFrame() to Compositor::EndFrame(), there is a lot of hidden cost that you aren't necessarily seeing. For instance, in the case of WebGL, if we are trying to compositing at 60fps, we could end up waiting a long time for the WebGL buffer to be resolved by the GPU, increasing our frame duration. If we are compositing at 5fps, then we probably won't have to wait at all, since the GPU will be done.

If you instead start and end the timer only in Compositor::EndFrame(), you can end up stuck at the lowest FPS that you ever hit, at least with my naive patch. I think this timing is definitely more accurate, but how does content know that the compositor has more left to give? Should we also time the amount of time we are sleeping and use that to come up with a target frame interval?

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: wip

This patch does what I discussed in the above comment, essentially trying to make the "time spent drawing" and "total time to end of frame" numbers approach each other.

I have no idea if throttling the refresh driver as a whole is right for this or if we should only throttle some things...

Comment 16

[Matt Woodrow (:mattwoodrow)]

I very much doubt the IPC overhead of sending an asynchronous message back after every composite will matter, I want to do this anyway for bug 974197.

Timing between EndFrame calls has the problem that it's only relevant if we're actually painting as fast as possible. If the main thread idles and stops sending us transactions then we'll record huge frame times, which isn't useful.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> I very much doubt the IPC overhead of sending an asynchronous message back
> after every composite will matter, I want to do this anyway for bug 974197.

We disagree :)

> 
> Timing between EndFrame calls has the problem that it's only relevant if
> we're actually painting as fast as possible. If the main thread idles and
> stops sending us transactions then we'll record huge frame times, which
> isn't useful.

Yeah. My patch here measure that as well as the actual start-to-end frame time. The difference is time we are doing other stuff, so presumably *most* of that should be available for compositing.

Comment 18

[Matt Woodrow (:mattwoodrow)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #17)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> > I very much doubt the IPC overhead of sending an asynchronous message back
> > after every composite will matter, I want to do this anyway for bug 974197.
> 
> We disagree :)

Fun :)

Why do you think it'll be an issue?

Sending an asynchronous message should just be pushing a small amount of data across a socket connection, and then the main thread reads it when the event loop gets there next. It shouldn't be any different to other things like mouse events, which we get huge amounts of already.

My patch to send DidComposite events back from the compositor doesn't regress talos at all, obviously not conclusive data but it's something.

Comment 19

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

I agree with Matt. We already want/need to add this reply message in the patches in 974197. Those patches should (re)land soon so let's do that and then fix this bug on top of them.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add transaction id to DidComposite

Comment 21

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: If we cancel a composite when going into test mode, fire the DidComposite event anway in case someone is waiting on it.

Comment 22

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add a way to detect PLayerTransaction objects that exist only for testing and won't composite

Comment 23

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Flush out of date animations even if async animations are disabled since they could also be throttled by the refresh driver.

Comment 24

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Add nsRefreshDriver API to track which transactions have completed composition.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Allocate and return transaction id's from the refresh driver when using OMTC to prevent over-production.

https://tbpl.mozilla.org/?tree=Try&rev=c72865af9d7

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8414185 [details] [diff] [review]
Part 5: Add nsRefreshDriver API to track which transactions have completed composition.

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

::: layout/base/nsRefreshDriver.cpp
@@ +1340,5 @@
> +nsRefreshDriver::FinishedWaitingForTransaction()
> +{
> +  mWaitingForTransaction = false;
> +  if (mSkippedPaint && (ObserverCount() || ImageRequestCount())) {
> +    NS_DispatchToCurrentThread(NS_NewRunnableMethod(this, &nsRefreshDriver::DoRefresh));

Won't this possibly cause double-dispatch?  If we have an active rAF, say, the timers will keep calling Tick.. tick will set mSkippedPaint and not do the work.  So we could get a DoRefresh triggered from here, followed by a a Tick() call from the the driver timer in less than 1 frame time.

I don't have a good solution, except perhaps to stop the timer if we set mSkippedPaint to true, and restart it in FinishedWaitingForTransaction.  That might mean a lot of churn on the timer though, which might be okay if we're in this situation anyway.

::: layout/base/nsRefreshDriver.h
@@ +280,5 @@
> +   * completion of asynchronous work. This is similar
> +   * to NotifyTransactionCompleted except avoids
> +   * return ordering issues.
> +   */
> +  void ReturnTransactionId(uint64_t aTransactionId);

Minor nit/bikeshed: the "return" naming here is a little odd to me (mainly because return means a specific thing, and this function returns void even!).  Maybe "IgnoreTransactionId", "SkipTransactionId", "NotifyTransactionUnused"..

Comment 27

[Matt Woodrow (:mattwoodrow)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #26)
> 
> Won't this possibly cause double-dispatch?  If we have an active rAF, say,
> the timers will keep calling Tick.. tick will set mSkippedPaint and not do
> the work.  So we could get a DoRefresh triggered from here, followed by a a
> Tick() call from the the driver timer in less than 1 frame time.
> 
> I don't have a good solution, except perhaps to stop the timer if we set
> mSkippedPaint to true, and restart it in FinishedWaitingForTransaction. 
> That might mean a lot of churn on the timer though, which might be okay if
> we're in this situation anyway.

Yes and no.

If the compositor is consistently failing to hit 60fps, then the transaction started from mSkippedPaint will most likely refreeze the refresh driver again so the following normal timer tick will be ignored again.

There's also the case where we had a few slow frames, but now we're back to being able to hit our timing. If the mSkippedPaint triggered tick was only a few ms late, then we probably still want the next normal tick to be back to the regular interval.

There's the possibility that the mSkippedPaint triggered tick happens *after* the regular one. That's bad, and I can definitely fix that. Just switch to an nsITimer trigger instead and cancel it if necessary.


> Minor nit/bikeshed: the "return" naming here is a little odd to me (mainly
> because return means a specific thing, and this function returns void
> even!).  Maybe "IgnoreTransactionId", "SkipTransactionId",
> "NotifyTransactionUnused"..

How about RevokeTransactionId? I used that terminology in the comment too.

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #27)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #26)
> > 
> > Won't this possibly cause double-dispatch?  If we have an active rAF, say,
> > the timers will keep calling Tick.. tick will set mSkippedPaint and not do
> > the work.  So we could get a DoRefresh triggered from here, followed by a a
> > Tick() call from the the driver timer in less than 1 frame time.
> > 
> > I don't have a good solution, except perhaps to stop the timer if we set
> > mSkippedPaint to true, and restart it in FinishedWaitingForTransaction. 
> > That might mean a lot of churn on the timer though, which might be okay if
> > we're in this situation anyway.
> 
> Yes and no.
> 
> If the compositor is consistently failing to hit 60fps, then the transaction
> started from mSkippedPaint will most likely refreeze the refresh driver
> again so the following normal timer tick will be ignored again.
> 
> There's also the case where we had a few slow frames, but now we're back to
> being able to hit our timing. If the mSkippedPaint triggered tick was only a
> few ms late, then we probably still want the next normal tick to be back to
> the regular interval.
> 
> There's the possibility that the mSkippedPaint triggered tick happens
> *after* the regular one. That's bad, and I can definitely fix that. Just
> switch to an nsITimer trigger instead and cancel it if necessary.

Can it? That regular one should be ignored (since we'll have a pending skipped paint) -- but I guess it might not be, if the event is enqueued after we set mSkippedPaint to false, but before the DoRefresh actually occurs on the thread.  Perhaps the solution is to not set mSkippedPaint to false until the DoRefresh event is actually processed by the thread?  Thinking about it more.. why does FinishedWaitingForTransaction enqueue an event? We know we're behind, we know we're on the right thread, we know we need to paint -- let's just call DoRefresh?

As written, we'll skip painting in Tick() if we have an outstanding transaction.  But that means that we have no opportunity for parallelism between the compositor and the painting thread/app -- we should be allowed to have at the very least one transaction in flight before we throttle paints.  That is, there should be a mPendingTransactions, and we should only skip paint if it's > 1.  In other words, we should be allowed to do painting/rAF work for transaction N+1 while the compositor is still compositing the results of transaction N.

> > Minor nit/bikeshed: the "return" naming here is a little odd to me (mainly
> > because return means a specific thing, and this function returns void
> > even!).  Maybe "IgnoreTransactionId", "SkipTransactionId",
> > "NotifyTransactionUnused"..
> 
> How about RevokeTransactionId? I used that terminology in the comment too.

Yeah, perfect.

Comment 29

[Matt Woodrow (:mattwoodrow)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #28)

> Can it? That regular one should be ignored (since we'll have a pending
> skipped paint) -- but I guess it might not be, if the event is enqueued
> after we set mSkippedPaint to false, but before the DoRefresh actually
> occurs on the thread.  Perhaps the solution is to not set mSkippedPaint to
> false until the DoRefresh event is actually processed by the thread? 
> Thinking about it more.. why does FinishedWaitingForTransaction enqueue an
> event? We know we're behind, we know we're on the right thread, we know we
> need to paint -- let's just call DoRefresh?

Ok, yeah that's a good idea. I was copying the behaviour of Thaw(), but we can probably just rely on this function being called from somewhere where it's ok to immediately run the tick.

> 
> As written, we'll skip painting in Tick() if we have an outstanding
> transaction.  But that means that we have no opportunity for parallelism
> between the compositor and the painting thread/app -- we should be allowed
> to have at the very least one transaction in flight before we throttle
> paints.  That is, there should be a mPendingTransactions, and we should only
> skip paint if it's > 1.  In other words, we should be allowed to do
> painting/rAF work for transaction N+1 while the compositor is still
> compositing the results of transaction N.

We're already doing this. GetTransactionId only sets mWaitingForTransaction if the just allocated id number is 2 higher than the most recently returned one (> N + 1).

Comment 30

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Add nsRefreshDriver API to track which transactions have completed composition. v2

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8416278 [details] [diff] [review]
Part 5: Add nsRefreshDriver API to track which transactions have completed composition. v2

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

Looks good, one question/comment that I didn't fully follow..

::: layout/base/nsRefreshDriver.cpp
@@ +1357,5 @@
> +      !mWaitingForTransaction &&
> +      !mTestControllingRefreshes) {
> +    mWaitingForTransaction = true;
> +    mSkippedPaint = false;
> +  }

Hrm, given !mWaitingForTransaction, I guess mSkippedPaint = false is harmless here, but I think the only place it should be set to false is in FinishedWaitingForTransaction.. so may want to remove it here, unless I'm missing a reason to set it to false here?

Comment 32

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

Comment on attachment 8414186 [details] [diff] [review]
Part 6: Allocate and return transaction id's from the refresh driver when using OMTC to prevent over-production.

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

Referencing the refresh driver from layers is a nasty layering violation. We should either split out the ID functionality into its own object, or put the refresh driver behind an abstract interface. I prefer the former because it probably results in less tangled lifetime management.

::: layout/base/nsPresShell.cpp
@@ +5865,5 @@
>        nsAutoPtr<LayerProperties> props(computeInvalidRect ?
>                                           LayerProperties::CloneFrom(layerManager->GetRoot()) :
>                                           nullptr);
>  
> +      mozilla::layout::MaybeSetupTransactionIdProvider(layerManager, aViewToPaint);

using namespace mozilla::layout please

Comment 33

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Add gfx API for allocating transaction ids

This also removes nsISupports from nsRefreshDriver since it didn't appear to actually be doing anything.

Comment 34

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 7: Allocate and return transaction id's from the refresh driver when using OMTC to prevent over-production. v2

Comment 35

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

Comment on attachment 8417930 [details] [diff] [review]
Part 6: Add gfx API for allocating transaction ids

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

::: gfx/layers/TransactionIdAllocator.h
@@ +10,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +class TransactionIdAllocator {

Add a comment explaining what this interface is for

Comment 36

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

Comment on attachment 8414184 [details] [diff] [review]
Part 4: Flush out of date animations even if async animations are disabled since they could also be throttled by the refresh driver.

Sorry for the delay getting to this:

two brief comments right now.  I think there are two things to review here:

 (1) whether the interaction with the up-to-date checks is ok with only this bit of the OMT animations code turned on (probably is, but I want to check)

 (2) (the bigger issue) whether we're ok using FlushAnimations and FlushTransitions right now.  They're pretty broken in a bunch of ways; I have a big patch queue for bug 996796 to make them use much better-tested code, although it has some issues that I need to deal with and haven't had time to.  I'm not sure whether using them right now (without that patch queue) would lead to Web-visible regressions (and what sorts, if so), and need to think about whether that's likely to be the case.  (I'd like to get bug 996796 in this release cycle, but not sure how realistic that is given my schedule.)

Comment 37

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

I guess I also don't understand what Part 4 accomplishes.  Is something else in this patch series putting us in a situation where we're not doing the work of a standard refresh driver tick but we *are* advancing the result of nsRefreshDriver::MostRecentRefresh?  (I'd have expected that we'd leave MostRecentRefresh reflecting when the most recent refresh actually was, though I also feel like I had this discussion about a week ago with Matt and had my expectation reversed after about 5 minutes of discussion, but I no longer remember why.)

Comment 38

[Matt Woodrow (:mattwoodrow)]

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC-7) from comment #37)
> I guess I also don't understand what Part 4 accomplishes.  Is something else
> in this patch series putting us in a situation where we're not doing the
> work of a standard refresh driver tick but we *are* advancing the result of
> nsRefreshDriver::MostRecentRefresh?  (I'd have expected that we'd leave
> MostRecentRefresh reflecting when the most recent refresh actually was,
> though I also feel like I had this discussion about a week ago with Matt and
> had my expectation reversed after about 5 minutes of discussion, but I no
> longer remember why.)

Yes, when we 'freeze' the refresh driver (because the compositor has fallen behind) we still hit the normal ticks (and update MostRecentRefresh) but don't do any of the style/layout flushing or painting.

I added this because we fail test_transitions/animations intermittently (mainly on mochitest-e10s and b2g) without this.

It appeared that the tests are expecting transitions to advance based on approximately wall clock time, but if we completely freeze the refresh driver this doesn't happen.

Updating the MostRecentRefresh value, and then flushing animations/transitions if JS ever tries to read the values avoids this. It also avoids wasting flushing them on the frozen refresh driver tick in the general case.

Comment 39

[Sotaro Ikeda [:sotaro]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #0)
> With bug 829747 landing, we can now block in
> SurfaceStream::WaitForCompositor() if WebGL is overproducing.

In my understanding, SurfaceStream::WaitForCompositor() does not work as overproducing detection, but works as WebGL side's SwapProducer() miss detection. SwapProducer() and SwapConsumer() is always called one by one under ClientCanvasLayer. Only case that stuck in WaitForCompositor() is unbalanced call between SwapProducer() and SwapConsumer(). From the source code, this could happen only when mStaging is missed onece because of failure happened under CanvasLayer::FirePreTransactionCallback().

Is my understanding wrong?

The followings are call sequence of  SwapProducer() and SwapConsumer().

*[1] SwapProducer() related call sequence

ClientCanvasLayer::RenderLayer()
  ->CanvasLayer::FirePreTransactionCallback()
    ->WebGLContextUserData::PreTransactionCallback()
      ->WebGLContextUserData::PreTransactionCallback()
        ->WebGLContext::PresentScreenBuffer()
          ->GLScreenBuffer::PublishFrame()
            ->GLScreenBuffer::AssureBlitted()
              ->GLScreenBuffer::BindReadFB_Internal()
              ->GLScreenBuffer::BindDrawFB_Internal()
              ->GLContext::raw_fBlitFramebuffer()
            ->GLScreenBuffer::Swap()
              ->SurfaceStream_TripleBuffer::SwapProducer()
                ->SurfaceStream::RecycleScraps()
                  ->SurfaceStream::Recycle()
                    ->SurfaceFactory::Recycle()
                ->SurfaceStream_TripleBuffer_Async::WaitForCompositor()
                  ->mMonitor.Wait(PR_MillisecondsToInterval(100))
                ->SurfaceStream::Move(mProducer, mStaging);//Move mProducer to mStaging
                ->SharedSurface_Gralloc::Fence()
                ->SurfaceStream::New(factory, size, mProducer);//Assign mProducer
                  ->SurfaceFactory::NewSharedSurface()//recycle or allocate SharedSurface
              ->GLScreenBuffer::Attach()
                ->SharedSurface_GL::LockProd()
                ->GLContext::LockSurface();
  ->CanvasClientSurfaceStream::Update() [2] call sequence ****************


*[2] SwapConsumer() related call sequence

ClientCanvasLayer::RenderLayer()
  ->CanvasLayer::FirePreTransactionCallback() [1] call sequence ****************
  ->CanvasClientSurfaceStream::Update()
    ->SurfaceStream::SwapConsumer()
      ->SurfaceStream_TripleBuffer::SwapConsumer_NoWait()
        ->if (mStaging) // mStaging is  present *****************************
          ->SurfaceStream::Scrap(mConsumer)
            ->mScraps.push(scrap)
          ->SurfaceStream::Move(mStaging, mConsumer);// staging to consumer
          ->mMonitor.NotifyAll();
      ->SharedSurface::WaitSync(); //synchronously wait GL write complete

Comment 40

[Sotaro Ikeda [:sotaro]]

I am going to work overproducing detection in different way because of B2g specific reason in bug 1006957.

Comment 41

[Sotaro Ikeda [:sotaro]]

> 
> The followings are call sequence of  SwapProducer() and SwapConsumer().
> 

Sorry, it is gonk specific call sequence...

Comment 42

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #41)
> > 
> > The followings are call sequence of  SwapProducer() and SwapConsumer().
> > 
> 
> Sorry, it is gonk specific call sequence...

Sequence is totally different in gonk case. Ignore my comment.

Comment 43

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

Comment on attachment 8414184 [details] [diff] [review]
Part 4: Flush out of date animations even if async animations are disabled since they could also be throttled by the refresh driver.

OK, I guess I'm ok with this, although this might be implicitly adding a dependency to bug 996796 (e.g., because of web-compat regressions), which I might not manage to get in this release.

However, I think we want to add code elsewhere so that in the normal flush case, we do:

  if (!nsLayoutUtils::AreAsyncAnimationsEnabled()) {
    mPresContext->TickLastStyleUpdateForAllAnimations();
  }

so that (for performance) we hit this code only when we actually need to (especially given that it's rather sketchy right now).

Maybe put this in the "if (i == 0)" case in nsRefreshDriver::Tick?  (Also, while you're there, please move the "      // This is the Flush_Style case." comment back up to the top of that if, above the code that got inserted.

r=dbaron with that

Comment 44

[Matt Woodrow (:mattwoodrow)]

Found a bug with this when working through talos results.

The current patches only disable the root refresh driver for a given window, which is the one that drivers painting.

However, rAF for content documents is driven by the refresh driver for that document, which isn't frozen.

This is particularly bad on our talos tests where we set the refresh driver to tick as fast as possible. We freeze the painting refresh driver, and then just call rAF as fast as we can until the compositor completes. We get great tscroll scores, but they're meaningless.

One option to fix this would be to try find the 'root' refresh driver and check if that's frozen as well as checking if the current refresh driver is frozen. We'd then need to add a refresh observer on that root refresh driver so that when it unfreezes and refresh, we can be notified and refresh ourselves too.

I'm also a bit confused about ordering of ticking multiple refresh drivers. It appears the timer just ticks all the refresh drivers in the order they were added, which seems like it would tick the root one first and content document ones after. That sounds like we'd paint and then run rAF immediately after painting. The rAF changes wouldn't get painted until the next tick.

If that's true (and desired), then the current solution might change things because the refresh observer on the root would fire *before* painting and we'd refresh the children out of order.

Vlad, roc, any ideas on the above?

Comment 45

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

I think we should paint the content refresh drivers first and the root last.

Freezing/unfreezing the content drivers seems like the right thing to do.

Comment 46

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

What roc said -- the current behaviour is surprising to me, and sounds wrong.  If the root is what triggers the composite, it should always be last.

Comment 47

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #44)
> I'm also a bit confused about ordering of ticking multiple refresh drivers.
> It appears the timer just ticks all the refresh drivers in the order they
> were added, which seems like it would tick the root one first and content
> document ones after. That sounds like we'd paint and then run rAF
> immediately after painting. The rAF changes wouldn't get painted until the
> next tick.

I've noticed this too with the newer Refresh Driver instrumentation I've added to the profiler:
https://www.dropbox.com/s/pvobs1l97uv85pi/Screenshot%202014-05-14%2013.15.47.png

The green background is the refresh timer ticking (shared by refresh drivers). Then you notice how we will issue a paint that goes to the screen and run additional rAF timers after we've already finished the paint.

Comment 48

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 8: Try find a 'root' refresh driver to see if it's blocked

Comment 49

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 9: Send DidComposite during ClientLayerManager destruction

We can destroy the ClientLayerManager before we receive the DidComposite() message back across IPC and we freeze the refresh driver forever.

Comment 50

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 10: Resume the refresh driver if we miss too many ticks

We shouldn't actually *need* this, but any bugs in this code would result in the refresh driver being frozen forever. It's a lot safer to add this timeout instead. 4 was chosen entirely arbitrarily.

Comment 51

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 11: Use rAF instead of setTimeout in a test

setTimeout is bad.

Looks like we're green enough to land: https://tbpl.mozilla.org/?tree=Try&rev=96fa555b9769

Comment 52

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8428998 [details] [diff] [review]
Part 10: Resume the refresh driver if we miss too many ticks

Hmm, we could get in this state if the compositor is *really* busy, could we not?

Comment 53

[Matt Woodrow (:mattwoodrow)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #52)
> Comment on attachment 8428998 [details] [diff] [review]
> Part 10: Resume the refresh driver if we miss too many ticks
> 
> Hmm, we could get in this state if the compositor is *really* busy, could we
> not?

Yeah we could. I might bump 4 up a bit higher (say 10?) to reduce the chances of this happening. If we really are compositing at 6fps then we're basically done for anyway.

Comment 54

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4226ea1c1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d658e89549b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce26a65c8dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/743bd08df703
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d9f1a4d7838
https://hg.mozilla.org/integration/mozilla-inbound/rev/056cc43a50b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f58a7521a51
https://hg.mozilla.org/integration/mozilla-inbound/rev/123998cc3f92
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe947863ad3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f05a5d9d89
https://hg.mozilla.org/integration/mozilla-inbound/rev/efff1d901dca

Comment 55

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1d4226ea1c1b
https://hg.mozilla.org/mozilla-central/rev/0d658e89549b
https://hg.mozilla.org/mozilla-central/rev/0ce26a65c8dd
https://hg.mozilla.org/mozilla-central/rev/743bd08df703
https://hg.mozilla.org/mozilla-central/rev/1d9f1a4d7838
https://hg.mozilla.org/mozilla-central/rev/056cc43a50b6
https://hg.mozilla.org/mozilla-central/rev/3f58a7521a51
https://hg.mozilla.org/mozilla-central/rev/123998cc3f92
https://hg.mozilla.org/mozilla-central/rev/cbe947863ad3
https://hg.mozilla.org/mozilla-central/rev/b4f05a5d9d89
https://hg.mozilla.org/mozilla-central/rev/efff1d901dca

Comment 56

[Ed Morley [:emorley]]

Unfortunately I had to back this out for causing bug 1017478:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Panda.*mochitest-4&tochange=295d14e9a169&fromchange=cbf657095e2d

remote:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=9a72db713446

Comment 57

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 12: Use the view tree to find the subdocument frame

Comment 58

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a50e3a1cec0
https://hg.mozilla.org/integration/mozilla-inbound/rev/911028717c58
https://hg.mozilla.org/integration/mozilla-inbound/rev/764b7e720800
https://hg.mozilla.org/integration/mozilla-inbound/rev/163dbef74332
https://hg.mozilla.org/integration/mozilla-inbound/rev/02da421dee19
https://hg.mozilla.org/integration/mozilla-inbound/rev/00a034661274
https://hg.mozilla.org/integration/mozilla-inbound/rev/41d32b33363b
https://hg.mozilla.org/integration/mozilla-inbound/rev/cde8e3cec3d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/87c99fb4a3b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/22ed8645736b
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b47f698c28d
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc1eb7543012

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a50e3a1cec0
https://hg.mozilla.org/mozilla-central/rev/911028717c58
https://hg.mozilla.org/mozilla-central/rev/764b7e720800
https://hg.mozilla.org/mozilla-central/rev/163dbef74332
https://hg.mozilla.org/mozilla-central/rev/02da421dee19
https://hg.mozilla.org/mozilla-central/rev/00a034661274
https://hg.mozilla.org/mozilla-central/rev/41d32b33363b
https://hg.mozilla.org/mozilla-central/rev/cde8e3cec3d3
https://hg.mozilla.org/mozilla-central/rev/87c99fb4a3b2
https://hg.mozilla.org/mozilla-central/rev/22ed8645736b
https://hg.mozilla.org/mozilla-central/rev/7b47f698c28d
https://hg.mozilla.org/mozilla-central/rev/dc1eb7543012