1055050 - Add visual warning if the transaction latency is > 100 ms

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

With OMTC it's becoming harder to know that a main thread transaction is delayed. Keeping the main thread transaction snappy is still important however.

We calculate an expected competition time for transaction and/or the start time. If we're above our wanted numbers by a certain threshold, like 100ms, we give some visual feedback like flashing the screen.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 2

[Benoit Girard (:BenWa)]

Demo: https://www.dropbox.com/s/so927ty2pn59m3x/2014-08-18%2018.10.16.mp4

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8474817 [details] [diff] [review]
patch

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

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +231,5 @@
> +    if (severity > 1.f) {
> +      severity = 1.f;
> +    }
> +    mLayerManager->VisualFrameWarning(severity);
> +  }

Don't we want to check this after we've finished processing the update? It should be a fairly small amount of time, but it's part of the 'transaction' and worth measuring.

::: layout/base/nsRefreshDriver.cpp
@@ +1106,5 @@
>    AutoRestore<bool> restoreInRefresh(mInRefresh);
>    mInRefresh = true;
>  
> +  AutoRestore<TimeStamp> restoreTickStart(mTickStart);
> +  mTickStart = TimeStamp::Now();

Doesn't this just duplicate mMostRecentRefresh?

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Comment on attachment 8474817 [details] [diff] [review]
> patch
> 
> Review of attachment 8474817 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/LayerTransactionParent.cpp
> @@ +231,5 @@
> > +    if (severity > 1.f) {
> > +      severity = 1.f;
> > +    }
> > +    mLayerManager->VisualFrameWarning(severity);
> > +  }
> 
> Don't we want to check this after we've finished processing the update? It
> should be a fairly small amount of time, but it's part of the 'transaction'
> and worth measuring.

Right, I was going to move it but forgot, nice catch.
> 
> ::: layout/base/nsRefreshDriver.cpp
> @@ +1106,5 @@
> >    AutoRestore<bool> restoreInRefresh(mInRefresh);
> >    mInRefresh = true;
> >  
> > +  AutoRestore<TimeStamp> restoreTickStart(mTickStart);
> > +  mTickStart = TimeStamp::Now();
> 
> Doesn't this just duplicate mMostRecentRefresh?

Ahh, maybe. I'll check that out.

Comment 5

[Benoit Girard (:BenWa)]

I'm going to make the overlay mobile only for now.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: patch

I've made minor changes to the fade out, added a border and made this mobile only. I don't think this warrants a new review but I'll accept drive-by comments.

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

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch final

Comment 8

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53d33674eb30

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53d33674eb30