Closed Bug 920123 Opened 7 years ago Closed 7 years ago
Fix Start/Stop Frame
Time Recording with OMTC
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).
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!
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?
What exactly did you want me to answer here?
(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.
Right, yes, agreed. We don't have a quick fix to that problem, and I don't think anyone is working on it currently.
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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #829345 - Flags: review?(matt.woodrow)
(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 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.
Attachment #829345 - Flags: review?(matt.woodrow) → review+
Let's try this: https://tbpl.mozilla.org/?tree=Try&rev=edbc2056d902
(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.
Attachment #829345 - Attachment is obsolete: true
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).
Attachment #830216 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.