974197 - Fire MozAfterPaint once the compositor finishes

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Currently we fire these events as soon as we paint layers.

I think this will be an improvement, and should be a good building block for bug 854421.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Call MozAfterPaint after the compositor runs

Comment 2

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

Comment on attachment 8378710 [details] [diff] [review]
Call MozAfterPaint after the compositor runs

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

::: dom/ipc/TabChild.cpp
@@ +104,5 @@
>  static bool sCpowsEnabled = false;
>  static int32_t sActiveDurationMs = 10;
>  static bool sActiveDurationMsSet = false;
>  
> +typedef std::map<uint64_t, TabChild*> TabChildMap;

nsTHashtable

::: dom/ipc/TabChild.h
@@ +518,5 @@
>      ScreenOrientation mOrientation;
>      bool mUpdateHitRegion;
>      bool mContextMenuHandled;
>      bool mWaitingTouchListeners;
> +    bool mLayersId;

Fix!

::: gfx/layers/ipc/PCompositor.ipdl
@@ +38,5 @@
>  child:
>    // The child should invalidate everything so that the whole window is redrawn.
>    async InvalidateAll();
>  
> +  async DidComposite(uint64_t id);

document this

Comment 3

[Matt Woodrow (:mattwoodrow)]

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

Comment 4

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/eff307d94044 for a cornucopia of failures in b2g reftest-4 and reftest-6 (https://tbpl.mozilla.org/php/getParsedLog.php?id=35180027&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=35183449&tree=Mozilla-Inbound are characteristic, but not exhaustive) and for making layout/reftests/printing/745025-1.html race to intermittent failure, maybe 30% or so on 10.6 opt (which is gifted with a whole lot of slow) and considerably less on 10.8 opt (https://tbpl.mozilla.org/php/getParsedLog.php?id=35190376&tree=Mozilla-Inbound was the only one out of 15 pushes after you).

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Call MozAfterPaint after the compositor runs v2

Found and fixed the b2g bugs. Debugging emulators is not fun.

Comment 6

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

Comment on attachment 8387199 [details] [diff] [review]
Call MozAfterPaint after the compositor runs v2

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +704,5 @@
> +  unused << SendDidComposite(0);
> +
> +  for (LayerTreeMap::iterator it = sIndirectLayerTrees.begin();
> +       it != sIndirectLayerTrees.end(); it++)
> +  {

goes on previous line

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Ed Morley [:emorley]]

Backed out for causing bug 980944:
https://hg.mozilla.org/mozilla-central/rev/7a2edc5171e6

Comment 9

[Zac C (:zac)]

Cross post from bug 980944:
Matt, I ran this again (although on a different computer) and it was passing. I don't know what to try apart from another Try run!

Comment 10

[Zac C (:zac)]

NB for comment #9 I used the Gecko from last week together with today's Gaia.

Comment 11

[Matt Woodrow (:mattwoodrow)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for Gaia integration test perma-fail.
https://hg.mozilla.org/integration/mozilla-inbound/rev/279e3bc8979d

https://tbpl.mozilla.org/php/getParsedLog.php?id=37516268&tree=Mozilla-Inbound

Comment 13

[Matt Woodrow (:mattwoodrow)]

Kevin, any ideas what I should do here?

Afaict, it's not possible to even run the gaia integration tests with a local build of gecko, so I'm not sure how I'm supposed to debug this.

I also can't find any references to MozAfterPaint in gaia or marionette-js, so it's not obvious why my patch would have affected this.

Is there anything special about the places tests (all the tests in this one file fail) that might be relevant?

Comment 14

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> I also can't find any references to MozAfterPaint in gaia or marionette-js,
> so it's not obvious why my patch would have affected this.

This one might be related:
http://hg.mozilla.org/mozilla-central/annotate/d68c74e48075/dom/browser-element/BrowserElementChildPreload.js#l574

Comment 15

[Matt Woodrow (:mattwoodrow)]

Could be!

From what I can tell, that file results in the events 'mozbrowserfirstpaint', 'mozbrowsernextpaint' and 'mozbrowserdocumentfirstpaint' being fired in the parent process.

I can't see b2g relying on any of those either though :(

Comment 16

[Markus Stange [:mstange]]

For nextpaint:

Something calls browser.waitForNextPaint, e.g. here: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L1388
waitForNextPaint calls addNextPaintListener: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/browser_mixin.js#L56
BrowserElementParent.addNextPaintListener adds the listener to its _nextPaintListeners list: http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.jsm#678

When MozAfterPaint fires,
BrowserElementChildPreload calls sendAsync("nextpaint"): http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#587
BrowserElementParent calls BrowserElementParent._recvNextPaint: http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.jsm#272
And that calls the listeners in nextPaintListeners.

Comment 17

[Kevin Grandon :kgrandon]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> Kevin, any ideas what I should do here?
> 
> Afaict, it's not possible to even run the gaia integration tests with a
> local build of gecko, so I'm not sure how I'm supposed to debug this.

We should be able to run the tests with a local build of b2g desktop by placing it in the gaia/b2g/ folder. I can probably help with this if needed.

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

This causes a 28.6% tresize regression on osx 10.6:
http://graphs.mozilla.org/graph.html#tests=[[254,63,21]]&sel=none&displayrange=7&datatype=running

when this was backed out it went back to normal

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

and this had caused a 4.71% regression on android 4.0 for ts, paint:
http://graphs.mozilla.org/graph.html#tests=[[83,63,29]]&sel=none&displayrange=7&datatype=running

Comment 20

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #16)
> For nextpaint:
> 
> Something calls browser.waitForNextPaint, e.g. here:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.
> js#L1388
> waitForNextPaint calls addNextPaintListener:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/browser_mixin.
> js#L56
> BrowserElementParent.addNextPaintListener adds the listener to its
> _nextPaintListeners list:
> http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/
> BrowserElementParent.jsm#678
> 
> When MozAfterPaint fires,
> BrowserElementChildPreload calls sendAsync("nextpaint"):
> http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/
> BrowserElementChildPreload.js#587
> BrowserElementParent calls BrowserElementParent._recvNextPaint:
> http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/
> BrowserElementParent.jsm#272
> And that calls the listeners in nextPaintListeners.

Thanks Markus, nice work.

Unfortunately I've now confirmed that the breakage isn't related MozAfterPaint in a child process.

Comment 21

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/263f232855ad

Comment 22

[Markus Stange [:mstange]]

Huh?

So it looks like you disabled layers acceleration for B2G Desktop builds. What implications does that have?

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/263f232855ad

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(In reply to Markus Stange [:mstange] from comment #22)
> Huh?
> 
> So it looks like you disabled layers acceleration for B2G Desktop builds.
> What implications does that have?

Seeing a lot more Gi failures than usual again too since this landed.

Comment 25

[Matt Woodrow (:mattwoodrow)]

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

(In reply to Markus Stange [:mstange] from comment #22)
> Huh?
> 
> So it looks like you disabled layers acceleration for B2G Desktop builds.
> What implications does that have?

That was entirely accidental, well spotted. I have no idea how it happened, it wasn't in my try pushes.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> Seeing a lot more Gi failures than usual again too since this landed.

This is pretty weird, because disabling layers acceleration would also disable any changes by my patch. Unless the failures are related to the layers acceleration change itself.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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