915673 - only use tiled layers for scrolling layers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Andreas Gal :gal]

Tiled layers disable HWC for the camera and video playback due to the UI overlay. We are missing power metrics with HWC. We also lost the homescreen/HWC, which will cause performance problems on tablets. We need support to disable tiling if the layer doesn't scroll. roc, does this make sense?

Comment 1

[Andreas Gal :gal]

blassey please investigate and put someone on this

even if we had gralloc we can't use HWC with tiled layers, we are limited to 3 layers or so with HWC

Comment 2

[Diego Wilson [:diego]]

Can we revert https://hg.mozilla.org/mozilla-central/rev/57a03f46e0b2 while this is investigated? It's affecting bring up of Hwc on JB and will affect power measurements in v1.2 in general.

Comment 3

[Diego Wilson [:diego]]

(In reply to Andreas Gal :gal from comment #1)
> even if we had gralloc we can't use HWC with tiled layers, we are limited to
> 3 layers or so with HWC

It's a bit more nuanced than that. I can try to explain the HWC limitations if you wish. It'll be a mini essay, though :)

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Diego Wilson [:diego] from comment #3)
> (In reply to Andreas Gal :gal from comment #1)
> > even if we had gralloc we can't use HWC with tiled layers, we are limited to
> > 3 layers or so with HWC
> 
> It's a bit more nuanced than that. I can try to explain the HWC limitations
> if you wish. It'll be a mini essay, though :)

I think such an essay would be very useful, for sure.

Brad and I are looking at doing the tiling only for async-scrollable layers now.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, Diego has an e-mail waiting for him to schedule such a "lecture".  We're changing the default back in bug 916112, waiting for a review and a check-in.

Comment 6

[Nicolas Silva [:nical]]

(In reply to Diego Wilson [:diego] from comment #3)
> (In reply to Andreas Gal :gal from comment #1)
> > even if we had gralloc we can't use HWC with tiled layers, we are limited to
> > 3 layers or so with HWC
> 
> It's a bit more nuanced than that. I can try to explain the HWC limitations
> if you wish. It'll be a mini essay, though :)

https://wiki.mozilla.org/Platform/GFX/hwcomposer

This would be a good place to host your essay.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: wip

This seems to mostly "work", but I don't think it will help us by itself. We still get tiled layers on the home and lock screens.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

(Not to mention this patch is really ugly)

Comment 9

[Andreas Gal :gal]

I think you are looking for active scroll roots, not just any reference frame.

nsIScrollableFrame* sf = do_QueryFrame(parent);
bool isActiveScrollRoot = sf && sf->IsScrollingActive() && sf->GetScrolledFrame() == f;

This is far outside of what I understand well, so you might want to consult with roc :)

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: my WIP

this is effectively the same patch as snorp (turns out we talked), but touches fewer files

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Roc, any thoughts on how best to do this?

Comment 12

[Andreas Gal :gal]

The vendor says the 7x25/7 has no limit on the number of layers so I think for existing hardware we can use tiled Thebes layers with HWC! 8x26 no longer has this path (copybit) and only supports MDP which has 4 layers, two of which are RGB (the other two are YUV). Higher end chips have more pipes. The driver uses 6 pipes (3/3) for chips later than 8x26. So we will need this patch, but if push comes to shove for existing hardware tiling would work as well. We definitely will need gralloc buffers for that.

Comment 13

[Andreas Gal :gal]

Roc, another option is to handle this at the layers level.

We could turn on intermediate surfaces for the topmost surface when none of the child layers change and then make that layer a gralloc buffer and composite to it and then show that with HWC. We wire up out of memory pressure to throw away these cached intermediate surfaces. This was we always switch to HWC when animation stops and we won't have a delay when animation or panning resumes. Wdyt?

Comment 14

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

Comment on attachment 804931 [details] [diff] [review]
my WIP

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

The approach here looks good.

::: dom/ipc/TabChild.cpp
@@ +1591,4 @@
>    gfxFloat top = TILEDLAYERBUFFER_TILE_SIZE *
>      floor(displayPortInLayerSpace.y / TILEDLAYERBUFFER_TILE_SIZE);
>    gfxFloat right = TILEDLAYERBUFFER_TILE_SIZE *
> +      (1 + ceil(displayPortInLayerSpace.XMost() / TILEDLAYERBUFFER_TILE_SIZE));

I have no idea why you're doing this ... I hope it's just gunk!

Comment 15

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

(In reply to Andreas Gal :gal from comment #13)
> We could turn on intermediate surfaces for the topmost surface when none of
> the child layers change and then make that layer a gralloc buffer and
> composite to it and then show that with HWC. We wire up out of memory
> pressure to throw away these cached intermediate surfaces.

The memory pressure observer won't necessarily catch out-of-gralloc-mem will it?

> This way we
> always switch to HWC when animation stops and we won't have a delay when
> animation or panning resumes. Wdyt?

Yes, I think caching precomposed intermediate surfaces is a good idea. It's going to be a significant amount of work though, not a quick safe fix if that's what you're looking for.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Comment on attachment 804931 [details] [diff] [review]
> my WIP
> 
> Review of attachment 804931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The approach here looks good.
> 
> ::: dom/ipc/TabChild.cpp
> @@ +1591,4 @@
> >    gfxFloat top = TILEDLAYERBUFFER_TILE_SIZE *
> >      floor(displayPortInLayerSpace.y / TILEDLAYERBUFFER_TILE_SIZE);
> >    gfxFloat right = TILEDLAYERBUFFER_TILE_SIZE *
> > +      (1 + ceil(displayPortInLayerSpace.XMost() / TILEDLAYERBUFFER_TILE_SIZE));
> 
> I have no idea why you're doing this ... I hope it's just gunk!

yes, I was debugging aligning tiles to displaypoorts, ignore that bit

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Mike, can you test with this patch (and tiles turned on, so bug 916112 reverted) minus the change to TabChild and report performance compared to current master?

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to test

here is the patch to test (TabChild change dropped, pref change reverted so that tiles are turned on)

Comment 19

[Benoit Girard (:BenWa)]

Comment on attachment 804931 [details] [diff] [review]
my WIP

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

This is good. It never causes us to re-create a layer with tiling where we wouldn't (the move would be costly) and we should only create a tiled layer in cases where we wouldn't hit the HWC.

::: gfx/layers/TiledLayerBuffer.h
@@ +8,4 @@
>  #define TILEDLAYERBUFFER_TILE_SIZE 256
>  
>  // Debug defines
> +#define GFX_TILEDLAYER_DEBUG_OVERLAY

Don't forget to revert this

::: gfx/layers/client/ClientThebesLayer.cpp
@@ +192,3 @@
>  {
>    NS_ASSERTION(InConstruction(), "Only allowed in construction phase");
> +  if (canScroll && Preferences::GetBool("layers.force-tiles") && GetCompositorBackendType() == LAYERS_OPENGL) {

This will cause Android to use non tiles layer so we might want to avoid doing this part of this change.

::: layout/base/FrameLayerBuilder.cpp
@@ +1336,5 @@
>      // the area is invalidated in the widget.
>    } else {
>      // Create a new thebes layer
> +    nsIScrollableFrame *scrollableFrame = do_QueryFrame((nsQueryFrame*)aReferenceFrame);
> +    layer = mManager->CreateThebesLayer(!!scrollableFrame);

Typically we prefer enum for readability. At the very least I'd rather use a temporary canScroll to make this code more readable.

Comment 20

[Mike Lee [:mlee]]

Hub, see Brad's comment #17 and review the patch in comment #18.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to test

Should have disabled the debug overlay

Comment 22

[Hubert Figuiere [:hub]]

(In reply to Mike Lee [:mlee] from comment #20)
> Hub, see Brad's comment #17 and review the patch in comment #18.

Can't review that (or its successor). You'd have to ask a peer on the gfx team.

I can test it though and provide feedback,

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

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

moving feedback request to hub. BenWa, I'm not sure if the step-down patches will unexpected pass or not with this patch, pushed to try to find out https://tbpl.mozilla.org/?tree=Try&rev=f9d5235ea1de

Comment 25

[Benoit Girard (:BenWa)]

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

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

::: b2g/app/b2g.js
@@ +73,4 @@
>  pref("mozilla.widget.use-buffer-pixmap", true);
>  pref("mozilla.widget.disable-native-theme", true);
>  pref("layout.reflow.synthMouseMove", false);
> +pref("layers.force-tiles", true);

Let's no flip this since we're not forcing tiling everywhere and this will lead to confusion. See my later comment.

::: gfx/layers/Layers.h
@@ +327,4 @@
>    virtual void Mutated(Layer* aLayer) { }
>  #endif
>  
> +  virtual already_AddRefed<ThebesLayer> CreateThebesLayer(bool canScroll) {

Lets add documentation so people don't have to search to understand 'canScroll'. How about:
/**
 * canScroll hints to the implementation that it should optimize scrolling performance.
 **/

let's rename canScroll to optimizeScrolling.

::: gfx/layers/client/ClientThebesLayer.cpp
@@ +192,4 @@
>  {
>    NS_ASSERTION(InConstruction(), "Only allowed in construction phase");
> +  if (
> +#ifdef MOZ_B2G

IMO this should be |LAYERS_OPEN && (optimizeScroll || forceTiles)|. We shouldn't flip force-tiles perf on b2g and we can continue to use the preference to forcing tiling on b2g to be analogous to forcing tiles on fennec. If we want a preference to allow tiling then we should add 'layers.use-tiling'.

Comment 26

[Benoit Girard (:BenWa)]

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

Opps, looks like bugzilla clobbered the f?

Comment 27

[Hubert Figuiere [:hub]]

It is unclear what test I should run and which perf metric it should affect.

Comment 28

[Benoit Girard (:BenWa)]

(In reply to Hubert Figuiere [:hub] from comment #27)
> It is unclear what test I should run and which perf metric it should affect.

Scrolling performance and scrolling power usage particularly on devices with HWC.

Comment 29

[Hubert Figuiere [:hub]]

but which devices have HWC?

Comment 30

[Hubert Figuiere [:hub]]

the browser scrollfps test on Inari isn't influenced either way by the this patch.

Comment 31

[Benoit Girard (:BenWa)]

(In reply to Hubert Figuiere [:hub] from comment #29)
> but which devices have HWC?

hamachi

Comment 32

[Andreas Gal :gal]

Hamachi probably won't be influenced either. I think the browser was always hitting 60fps. That path didn't seem to suffer much from what this patch is trying to fix. Plus, Sotaro has just about all the gralloc lock contention issues fixed on trunk. If at all, I would expect differences in the contacts app and settings.

That having said, I am somewhat confident that we have most perf issues under control, so we can likely survive without this patch (we are basically digging our way back to 1.1 performance). If this patch turns out to be a benefit beyond that, we can still consider it. I leave that to the people in this bug to sort out whether its worth the risk.

The overall goal is unchanged: reach 60fps in the browser and all apps for all animations and scrolling, or as close as possible, and at least as close as 1.1.

Comment 33

[Andreas Gal :gal]

My original proposal was to tile active scrollable layers. This means that once a layer times out we would no longer tile it and can use HWC.

This patch tiles all scrollable layers. This means that we will not be able to use the HWC to ever display those layers.

After some thinking about this I think the solution in this patch is actually better. The main places where we care about HWC is video and the camera. Those places don't have scrollable UI, and will not tile, and HWC will be used. On the otherhand, when we have scrolling pieces like contacts, we won't have to worry about a delay re-tiling layers that have timed out.

On top of that, we can support a small number of tiles with HWC (separate patch).

Comment 34

[Benoit Girard (:BenWa)]

Typically we only have active scrollable layers when we're scrolling and these timeout after about 1-5 seconds. If those are the only cases that this patch creates a tiled layer (and I believe it is) then we should be fine. And if it's not we can just debug and likely fix them.

Comment 35

[Hubert Figuiere [:hub]]

We get +0.6 fps on the scrollfps test on buri with this patch. This is not much.

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Hubert Figuiere [:hub] from comment #35)
> We get +0.6 fps on the scrollfps test on buri with this patch. This is not
> much.

was that browser, contacts or settings?

Comment 37

[Andreas Gal :gal]

Whats scrollfps? Can you compare just visually scrolling contacts?

Comment 38

[Hubert Figuiere [:hub]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #36)
> (In reply to Hubert Figuiere [:hub] from comment #35)
> > We get +0.6 fps on the scrollfps test on buri with this patch. This is not
> > much.
> 
> was that browser, contacts or settings?

Same as in comment 30: browser.

Comment 39

[Hubert Figuiere [:hub]]

(In reply to Andreas Gal :gal from comment #37)
> Whats scrollfps? Can you compare just visually scrolling contacts?

I don't have two similar devices to perform visual comparison.

Comment 40

[Hubert Figuiere [:hub]]

I tested contacts scrolling (1000 contacts from the reference workload)

before 54.17fps
after 53.51fps

We lost ~0.5fps with the tiling patch.

Comment 41

[Benoit Girard (:BenWa)]

(In reply to Hubert Figuiere [:hub] from comment #40)
> I tested contacts scrolling (1000 contacts from the reference workload)
> 
> before 54.17fps
> after 53.51fps
> 
> We lost ~0.5fps with the tiling patch.

Can you explain the test methodology?

Comment 42

[Hubert Figuiere [:hub]]

It uses b2gperf which uses Marionette do stuff.

The test is run in

https://github.com/mozilla/b2gperf/blob/master/b2gperf/b2gperf.py
https://github.com/mozilla/b2gperf/blob/master/b2gperf/scrollapp.js

Comment 43

[Milan Sreckovic [:milan] (needinfo for best results)]

Assign to Chris while we're investigating.

Comment 44

[Chris Lord [:cwiiis]]

Just to note, I think there's no use in comparing non-tiles performance with tiles performance without a displayport that aligns to tiles set (see bug 907743 and bug 909877) - it would likely only be the same or worse otherwise.

If this is desired behaviour, is there any reason not to commit these patches, but just not turn on tiles by default for now? It's a pain to maintain this sort of thing outside of the tree.

Comment 45

[Andreas Gal :gal]

before 54.17fps
after 53.51fps

I don't care about 0.5fps delta. Both numbers are too low. Lets land this and iterate until we hit 62fps. Tiling will improve a bunch of other things so this should definitely land. Great work guys.

Comment 46

[Andreas Gal :gal]

Lets attach a profile after you land this to see why we are leveling out at 54fps.

Comment 47

[Chris Lord [:cwiiis]]

hmm, so loading a long, scrollable page with the patch in this bug, I end up not getting a tiled layer... Which isn't good. Will see if I can figure out why that is.

Comment 48

[Chris Lord [:cwiiis]]

Just a supposition, perhaps this doesn't work quite right because layers with scrollable reference frames are getting flattened into layers without(?)

Comment 49

[Chris Lord [:cwiiis]]

An alternative hypothesis, perhaps the layer starts off as being too small to scroll and grows to a size where it can scroll, but the layer gets recycled rather than recreated(?) - given that the scrollable check just sees if there's an nsIScrollableFrame on the reference frame, I assume this can't be the case, but just throwing stuff out there.

Comment 50

[Chris Lord [:cwiiis]]

So it seems that no effort gets made to set a displayport on the document before it loads, so the scroll container is inactive and we don't create tiles.

More worryingly, I notice that when you scroll, the entire document is constantly redrawing the entire view - is this paint flashing not working, or are we really doing that?

Comment 51

[Chris Lord [:cwiiis]]

For reference, fennec sets a displayport synchronously when it receives the before-first-paint signal - b2g needs to do the same thing if we don't want the first pan gesture to always be a bit naff (and also, if we want to make sure the right layers get constructed on document load).

This doesn't really answer why this isn't working after a pan though. Still investigating.

Comment 52

[Chris Lord [:cwiiis]]

So the same code does exist for b2g (well, the equivalent), so a display-port should be set on before-first paint. Investigating why this appears as if it isn't working.

Comment 53

[Chris Lord [:cwiiis]]

So the display-port isn't set because the metrics that gets sent to UpdateRootFrame has an mScrollId of NULL_SCROLL_ID. This seems like it could happen, BEFORE_FIRST_PAINT doesn't touch mScrollId and neither does HandlePossibleViewportChange so it'll be whatever was in mLastMetrics.

Seems like the correct thing to do would be to set mScrollId to viewId in HandlePossibleViewportChange when GetIsFirstPaint returns true.

Comment 54

[Chris Lord [:cwiiis]]

So fixing that does get the displayport set, which is nice, but unfortunately doesn't fix this problem - I'll open a new bug for that.

Comment 55

[Chris Lord [:cwiiis]]

So the animated geometry root of the item that creates the thebes layer in the browser is a Canvas(html), which suggests that a scrollable frame isn't being created in this situation. Do we not create scrollable frames for remote browsers? Or something along these lines?

Comment 56

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Only use tiles on scrollable layers

Re-requesting review, as as has been suggested in comments, this now checks the animated geometry root rather than the reference frame.

Comment 57

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Use tiles on canvas frames with a displayport

So it seems that when using apzc, we end up without a scrollable frame? Here's one work-around, I'm interested in hearing whether this is a good idea or not but it does the job with no false-positives on b2g at least.

Comment 58

[Timothy Nikkel (:tnikkel)]

The canvas frame is the scrolled frame, and in general the animated geometry root will be the scrolled frame (if the type of animated geometry is has is scrolling). So you want to look at the parent of the animated geometry root to see if it is a scroll frame.

Comment 59

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8338641 [details] [diff] [review]
Part 1 - Only use tiles on scrollable layers

comment 58 are the review comments.

Comment 60

[Chris Lord [:cwiiis]]

(In reply to Timothy Nikkel (:tn) from comment #58)
> The canvas frame is the scrolled frame, and in general the animated geometry
> root will be the scrolled frame (if the type of animated geometry is has is
> scrolling). So you want to look at the parent of the animated geometry root
> to see if it is a scroll frame.

This doesn't seem to hold up - the original part 1 patch works, if this was true, it oughtn't. But even doing what you say, canScroll is always false. It seems the animated geometry root is the scroll frame, not the scrolled frame, except in the remote browser case, where a scroll frame doesn't seem to get created.

If this is correct, my original patches stand - if not, or if it's not meant to be this way, we have some debugging to do.

Comment 61

[Chris Lord [:cwiiis]]

Reading GetAnimatedGeometryRootFor, it does sound like what you're saying is what should happen...

Comment 62

[Chris Lord [:cwiiis]]

And it just ends up I'm a massive idiot and forgot to apply one of the patches in my queue - what you suggest works perfectly, as it should.

Comment 63

[Chris Lord [:cwiiis]]

Attachment: Only use tiles on scrollable layers

Comment 64

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8339275 [details] [diff] [review]
Only use tiles on scrollable layers

>+    // Check whether the layer will be scrollable. This is used as a hint to
>+    // influence whether tiled layers are used or not.
>+    bool canScroll = false;
>+    nsIFrame* aAnimatedGeometryRootParent = aAnimatedGeometryRoot->GetParent();

Just call it parent? The "aAnimatedGeometryRootParent" style is what we usually use for function argument names.

Also, get someone to okay the layers changes?

Comment 65

[Chris Lord [:cwiiis]]

Comment on attachment 8339275 [details] [diff] [review]
Only use tiles on scrollable layers

Adding r?BenWa for layers changes.

Comment 66

[Benoit Girard (:BenWa)]

Comment on attachment 8339275 [details] [diff] [review]
Only use tiles on scrollable layers

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

I'd like to see a profile with this off and on to compare how we differ. If you profile the content main thread for an app + the compositor you will get a paint timeline from which you can reason about frame rates and skipped frames.

::: gfx/layers/client/ClientLayerManager.h
@@ +65,5 @@
>    virtual void SetRoot(Layer* aLayer);
>  
>    virtual void Mutated(Layer* aLayer);
>  
> +  virtual already_AddRefed<ThebesLayer> CreateThebesLayer(bool canScroll);

Why not just use a default parameter? It's not a public API so we don't have to keep binary compatibility. I'd like to know if there's a good reason for this?

Also prefer enum to bool arguments.

::: gfx/layers/client/ClientThebesLayer.cpp
@@ +162,5 @@
> +  if (
> +#ifdef MOZ_B2G
> +    canScroll &&
> +#endif
> +    Preferences::GetBool("layers.force-tiles") && GetCompositorBackendType() == LAYERS_OPENGL) {

IMO force-tiles should force tiling everywhere. Otherwise it shouldn't have the word force. This is more consistent with our use of force.

Comment 67

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #66)
> Comment on attachment 8339275 [details] [diff] [review]
> Only use tiles on scrollable layers
> 
> Review of attachment 8339275 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to see a profile with this off and on to compare how we differ. If
> you profile the content main thread for an app + the compositor you will get
> a paint timeline from which you can reason about frame rates and skipped
> frames.

A profile of what though? Browser scrolling perf? Home-screen scrolling perf? Settings scrolling perf? Perhaps all three, I suppose... Do we want to consider what this does to memory usage too?

> ::: gfx/layers/client/ClientLayerManager.h
> @@ +65,5 @@
> >    virtual void SetRoot(Layer* aLayer);
> >  
> >    virtual void Mutated(Layer* aLayer);
> >  
> > +  virtual already_AddRefed<ThebesLayer> CreateThebesLayer(bool canScroll);
> 
> Why not just use a default parameter? It's not a public API so we don't have
> to keep binary compatibility. I'd like to know if there's a good reason for
> this?
> 
> Also prefer enum to bool arguments.

I didn't touch this part of the patch - will change it.

> ::: gfx/layers/client/ClientThebesLayer.cpp
> @@ +162,5 @@
> > +  if (
> > +#ifdef MOZ_B2G
> > +    canScroll &&
> > +#endif
> > +    Preferences::GetBool("layers.force-tiles") && GetCompositorBackendType() == LAYERS_OPENGL) {
> 
> IMO force-tiles should force tiling everywhere. Otherwise it shouldn't have
> the word force. This is more consistent with our use of force.

Well, do we want to rename the pref with this patch to enable-tiles? Or add that pref and modify usage elsewhere?

Comment 68

[Benoit Girard (:BenWa)]

(In reply to Chris Lord [:cwiiis] from comment #67)
> (In reply to Benoit Girard (:BenWa) from comment #66)
> > Comment on attachment 8339275 [details] [diff] [review]
> > Only use tiles on scrollable layers
> > 
> > Review of attachment 8339275 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'd like to see a profile with this off and on to compare how we differ. If
> > you profile the content main thread for an app + the compositor you will get
> > a paint timeline from which you can reason about frame rates and skipped
> > frames.
> 
> A profile of what though? Browser scrolling perf? Home-screen scrolling
> perf? Settings scrolling perf? Perhaps all three, I suppose... Do we want to
> consider what this does to memory usage too?

More data is better but start by gathering what you think is reasonable. And if that leaves us with more questions we can gather more. Those 3 cases should be easy to gather and not too time consuming.


> > ::: gfx/layers/client/ClientThebesLayer.cpp
> > @@ +162,5 @@
> > > +  if (
> > > +#ifdef MOZ_B2G
> > > +    canScroll &&
> > > +#endif
> > > +    Preferences::GetBool("layers.force-tiles") && GetCompositorBackendType() == LAYERS_OPENGL) {
> > 
> > IMO force-tiles should force tiling everywhere. Otherwise it shouldn't have
> > the word force. This is more consistent with our use of force.
> 
> Well, do we want to rename the pref with this patch to enable-tiles? Or add
> that pref and modify usage elsewhere?

rename should be fine. If we want to force tiles later we can add the preference then. I just hate having things that don't follow conventions but we have trouble changing because of legacy.

Comment 69

[Chris Lord [:cwiiis]]

With regards to the performance testing, it's a bit pointless at the moment - I'm seeing really bad invalidation behaviour with APZC enabled, that I don't think the numbers would mean much. I'm going to look into some of that first and get back to this when the behaviour matches what ought to be happening.

Comment 70

[Benoit Girard (:BenWa)]

Ok, IMO we should land the code now and block flipping the preference on the analysis.

Comment 71

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #70)
> Ok, IMO we should land the code now and block flipping the preference on the
> analysis.

oh, to be clear, I was never intending to flip the pref - there are *waaaay* too many bugs with apzc atm, and it's certainly a questionable change without that. I think we have a separate bug about flipping the pref? I'll add some information over there if we do, or file one if we don't.

Comment 72

[Chris Lord [:cwiiis]]

With the patch in bug 945277, I think behaviour is 'good' enough that performance numbers might mean something now, so I'm going to measure some. I expect the home-screen to improve (as it doesn't really reap any benefits from tiles) and most of the rest to stay the same. Possible slight improvement in settings screen.

The real benefit ought to be memory and battery use (where tiles stops us from using HWComposer and uses more memory due to wastage).

Comment 73

[Chris Lord [:cwiiis]]

I ran tests on b2gperf, but after spending a bit too much time on it, I realise the numbers it outputs are just not correct (likely including those in comment #40. It seems b2gperf always reports a number very near 60, when just looking, and indeed the gfx framerate counter tell a very different story.

So I've conducted my own, highly unscientific tests, by loading the apps in question, waiting for them to settle, then initiating fast flings and keeping an eye on the framerate counter, mentally recording the (very approximate) average fps.

APZC on, without tiles:

Home-screen: At the start of the pan, 10-30fps, during animation, ~57fps
Contacts: ~40fps
Settings: ~42fps

APZC on, with tiles:

Home-screen: At the start of the pan, 25-40fps, during animation, 48-57fps
Contacts: 38-45fps
Settings: ~44fps

APZC on, with tiles only on scrollable layers:

Home-screen: At the start of the pan, 25-40fps, during animation, ~56fps
Contacts: 38-42fps
Settings: ~44fps


In terms of differences you could feel, I noticed that the lag that often happens as we layerize at the start of a pan felt noticeably shorter with tiles enabled (this was most evident in the home-screen). Performance also felt marginally better on the settings screen with tiles than without. Otherwise, performance wasn't noticeably different in any of these scenarios. This tallies with my expectations, as these apps are written to perform well without asynchronous scrolling.

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

Attachment: fpsLog.patch

Chris, I found this patch from mchang to be useful when trying to perform these types of tests.  It just logs the fps numbers from the compositor instead of drawing the overlay.  A bit easier to compare against runs, particularly if you enable logcat timestamps.

Comment 75

[Chris Lord [:cwiiis]]

Attachment: Only use tiles on scrollable layers (v2)

Carrying r=tn.

Comment 76

[Chris Lord [:cwiiis]]

Comment on attachment 8341785 [details] [diff] [review]
Only use tiles on scrollable layers (v2)

My argument for including this now would be that it has no discernible effect on performance, but will reduce memory usage.

We should probably revisit this in the future, when we're getting more serious about tiles, however.

Comment 77

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/954f30096cbe

Comment 78

[Andreas Gal :gal]

+#ifdef MOZ_B2G
+      aHint == SCROLLABLE &&
+#endif

It would be nicer to drive this with a pref. For example layers.tiles-for-scrollable-layers-only or something like that.

Also, don't read prefs during creation. Please cache that into a static.

Comment 79

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/954f30096cbe