920123 - Fix Start/Stop FrameTimeRecording with OMTC

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Avi Halachmi (:avih)]

Frames intervals recording doesn't work with OMTC, since the current recording code resides on PostPresent which is called on the main thread, while OMTC uses a different Post present.

This breaks tab animation telemetry on OS X (where OMTC is now on by default, and at some stage it will also on windows), and prevents running talos TART on OS X with OMTC (TART disables OMTC for this reason, among others), therefore results are less representative of real-world (though not necessarily worse in detecting regression).

Bug 826383 improved the recording system and also added "paint durations" to the recording, but as we've gained some experience on this subject, we now know this value could be misleading, and it also doesn't have a clear definition to begin with (even more so with OMTC). IMO paint-durations recording can go if it adds complexity to this fix.

Intervals recording remains quite useful, however.

As for approaches:

- Jeff suggested to send a message from the compositor thread and collect the intervals on the main thread. While this is viable, it will introduce some runtime overhead which might not be negligible. I still can't assess the code complexity of this approach.

- Another approach might be both simpler to implement and have less runtime overhead, is to use a shared buffer between the compositor and the main thread, and access it in some thread-safe way (not necessarily locking IMO, but I didn't think about it long enough yet).

Comment 1

[Nick Cameron [:nrc]]

How is this coming along? My ThebesLayerBuffer work is ready to land and is blocked on this, so if it will take a while then I need to find some intermediate solution. Thanks!

Comment 2

[Avi Halachmi (:avih)]

jrmuizel, mstange and myself are still working on this. One patch by Markus worked but due to the main thread refresh driver not synchronized to the compositor thread, it records higher frequency than actual compositions (overproducing). I believe jeff and matt said that overproducing should be fixed (making it possible to record intervals on the main thread), but jeff thinks that until this happens, we should measure at the compositor thread. It's still work in progress.

The usage for this is talos TART, tab animation telemetry, and the fx-team work on improving Australis performance.

Improving Australis is done for now (bug 837885 comment 6).

I believe we could live for a while without talos TART and tab animation telemetry.

So I think ThebesLayerBuffer can land, and we'll still work on this regardless.

Jeff?

Comment 3

[Matt Woodrow (:mattwoodrow)]

What exactly did you want me to answer here?

Comment 4

[Avi Halachmi (:avih)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> What exactly did you want me to answer here?

Do you share Jeff's view that fixing overproduction might not happen soon? Because if it does happen soon, then we already have a patch to land for this bug.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Right, yes, agreed. We don't have a quick fix to that problem, and I don't think anyone is working on it currently.

Comment 6

[Markus Stange [:mstange]]

Attachment: record intervals in LayerManagerComposite

This patch removes the concept of paintTimes and makes ClientLayerManager forward its Start/StopFrameTimeRecording calls via sync ipdl calls through CompositorParent to the LayerManagerComposite on the compositor thread. I've moved the buffer size pref handling into DOMWindowUtils because it needs to be done on the main thread.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

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

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> https://tbpl.mozilla.org/?tree=Try&rev=010db58eee94

This run didn't have OMTC enabled and thus should be ignored.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 829345 [details] [diff] [review]
record intervals in LayerManagerComposite

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

::: gfx/layers/Layers.h
@@ +552,3 @@
>  
>    /**
>     *  Clears, then populates 2 arraye with the recorded frames timing data.

Can you fix this comment while you're at it please :)

::: gfx/layers/client/ClientLayerManager.cpp
@@ +208,5 @@
>      mWidget->PrepareWindowEffects();
>    }
>    EndTransactionInternal(aCallback, aCallbackData, aFlags);
>    ForwardTransaction();
> +  PostPresent();

Why not put this into ForwardTransaction()?

::: gfx/layers/composite/LayerManagerComposite.h
@@ +228,5 @@
>  
>    static void PlatformSyncBeforeReplyUpdate();
>  
> +  void StartFrameIntervalRecording();
> +  void StopFrameIntervalRecording(InfallibleTArray<float>& aIntervals);

These are declared, but never defined or used.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Let's try this:
https://tbpl.mozilla.org/?tree=Try&rev=edbc2056d902

Comment 11

[Markus Stange [:mstange]]

Attachment: record intervals in LayerManagerComposite

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> ::: gfx/layers/client/ClientLayerManager.cpp
> @@ +208,5 @@
> >      mWidget->PrepareWindowEffects();
> >    }
> >    EndTransactionInternal(aCallback, aCallbackData, aFlags);
> >    ForwardTransaction();
> > +  PostPresent();
> 
> Why not put this into ForwardTransaction()?

I didn't actually want to add this. ClientLayerManager never starts frame interval collecting itself (it only forwards the request along), so this wouldn't actually do anything. I've removed this.

Comment 12

[Avi Halachmi (:avih)]

(In reply to Markus Stange [:mstange] from comment #11)
> Created attachment 830216 [details] [diff] [review]
> record intervals in LayerManagerComposite

Few things:

1. It errors (but doesn't affect measurements as far as I can tell): JavaScript error: chrome://browser/content/tabbrowser.xml, line 4072: averagePaint is not defined

2. Recording is still working fine in OMTC-off mode, and measures the same values as without the patch: 3-4ms on my win7 system, and ~10ms on my OS X system, and varies with window size (bug 940977).

3. Something smells weird to me when recording with OMTC on. It looks like it always records intervals of about 14-14.5ms, with very little variance (between several runs, when changing window size, etc). This happens with the same values of 14+ms on both my win7 system and on the OS X system.

With OMTC, I think it also shouldn't be slower than without OMTC, at least not on OS X, but the numbers are worse with OMTC.

So I think something is not right with this patch, unless these results have some reasonable explanation.

Comment 13

[Markus Stange [:mstange]]

Attachment: record intervals in LayerManagerComposite

This fixes the bug in tabbrowser.xml and removes the telemetry probe entries for FX_TAB_ANIM_OPEN_PREVIEW_FRAME_PAINT_MS and FX_TAB_ANIM_OPEN_FRAME_PAINT_MS from Histograms.json (because they're no longer measured).

Comment 14

[Markus Stange [:mstange]]

(In reply to Avi Halachmi (:avih) from comment #12)
> (In reply to Markus Stange [:mstange] from comment #11)
> > Created attachment 830216 [details] [diff] [review]
> > record intervals in LayerManagerComposite
> 
> Few things:
> 
> 1. It errors (but doesn't affect measurements as far as I can tell):
> JavaScript error: chrome://browser/content/tabbrowser.xml, line 4072:
> averagePaint is not defined

Fixed.

> 2. Recording is still working fine in OMTC-off mode, and measures the same
> values as without the patch: 3-4ms on my win7 system, and ~10ms on my OS X
> system, and varies with window size (bug 940977).

This is good and works as intended.

> 3. Something smells weird to me when recording with OMTC on. It looks like
> it always records intervals of about 14-14.5ms, with very little variance
> (between several runs, when changing window size, etc). This happens with
> the same values of 14+ms on both my win7 system and on the OS X system.

There was indeed a hardcoded inter-frame delay on the compositor thread: bug 943041.

With the patch in that bug, OMTC has better numbers than non-OMTC in my tests.

Comment 15

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0eccdc854d5

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0eccdc854d5