Closed Bug 1055050 Opened 5 years ago Closed 5 years ago

Add visual warning if the transaction latency is > 100 ms

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1055041
No longer blocks: 1055046
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8474817 - Flags: review?(matt.woodrow)
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?
Attachment #8474817 - Flags: review?(matt.woodrow) → review+
(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.
I'm going to make the overlay mobile only for now.
Attached patch patch (obsolete) — Splinter Review
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
Attachment #8474817 - Attachment is obsolete: true
Attachment #8480798 - Flags: review+
Attached patch patch finalSplinter Review
Attachment #8480798 - Attachment is obsolete: true
Attachment #8480839 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/53d33674eb30
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1061846
No longer blocks: 1061846
You need to log in before you can comment on or make changes to this bug.