Closed Bug 915673 Opened 6 years ago Closed 6 years ago

only use tiled layers for scrolling layers

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: gal, Assigned: cwiiis)

References

Details

(Keywords: perf, Whiteboard: [c=handeye p= s= u=1.3])

Attachments

(4 files, 6 obsolete files)

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?
Assignee: nobody → blassey.bugs
Blocks: 887819
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
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.
(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 :)
(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.
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.
(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.
Attached patch wipSplinter Review
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.
(Not to mention this patch is really ugly)
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 :)
Attached patch my WIPSplinter Review
this is effectively the same patch as snorp (turns out we talked), but touches fewer files
Roc, any thoughts on how best to do this?
Flags: needinfo?(roc)
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.
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 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!
Attachment #804931 - Flags: feedback+
(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.
Flags: needinfo?(roc)
(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
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?
Flags: needinfo?(mlee)
Attached patch patch to test (obsolete) — Splinter Review
here is the patch to test (TabChild change dropped, pref change reverted so that tiles are turned on)
Attachment #806033 - Flags: review?(mlee)
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.
Attachment #804931 - Flags: feedback+
Hub, see Brad's comment #17 and review the patch in comment #18.
Status: NEW → ASSIGNED
Flags: needinfo?(mlee) → needinfo?(hub)
Keywords: perf
Whiteboard: [c=handeye p= s= u=]
Attachment #806033 - Flags: review?(mlee) → review?(hub)
Attached patch patch to test (obsolete) — Splinter Review
Should have disabled the debug overlay
Attachment #806033 - Attachment is obsolete: true
Attachment #806033 - Flags: review?(hub)
Attachment #806076 - Flags: feedback?(mlee)
(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,
Flags: needinfo?(hub)
Attached patch patch (obsolete) — Splinter Review
Attachment #806120 - Flags: review?(bgirard)
Attachment #806120 - Flags: feedback?(mlee)
Attachment #806076 - Attachment is obsolete: true
Attachment #806076 - Flags: feedback?(mlee)
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
Attachment #806120 - Flags: feedback?(mlee) → feedback?(hub)
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'.
Attachment #806120 - Flags: review?(bgirard)
Attachment #806120 - Flags: review+
Attachment #806120 - Flags: feedback?(mlee)
Attachment #806120 - Flags: feedback?(hub)
Comment on attachment 806120 [details] [diff] [review]
patch

Opps, looks like bugzilla clobbered the f?
Attachment #806120 - Flags: feedback?(mlee) → feedback?(hub)
It is unclear what test I should run and which perf metric it should affect.
(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.
but which devices have HWC?
the browser scrollfps test on Inari isn't influenced either way by the this patch.
(In reply to Hubert Figuiere [:hub] from comment #29)
> but which devices have HWC?

hamachi
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.
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).
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.
We get +0.6 fps on the scrollfps test on buri with this patch. This is not much.
Attachment #806120 - Flags: feedback?(hub)
(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?
Whats scrollfps? Can you compare just visually scrolling contacts?
(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.
(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.
I tested contacts scrolling (1000 contacts from the reference workload)

before 54.17fps
after 53.51fps

We lost ~0.5fps with the tiling patch.
(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?
Assign to Chris while we're investigating.
Assignee: blassey.bugs → chrislord.net
blocking-b2g: --- → 1.3+
Whiteboard: [c=handeye p= s= u=] → [c=handeye p= s= u=1.3]
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.
Flags: needinfo?(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.
Flags: needinfo?(gal)
Lets attach a profile after you land this to see why we are leveling out at 54fps.
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.
Just a supposition, perhaps this doesn't work quite right because layers with scrollable reference frames are getting flattened into layers without(?)
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.
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?
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.
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.
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.
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.
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?
Re-requesting review, as as has been suggested in comments, this now checks the animated geometry root rather than the reference frame.
Attachment #806120 - Attachment is obsolete: true
Attachment #8338641 - Flags: review?(tnikkel)
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.
Attachment #8338644 - Flags: review?(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 on attachment 8338641 [details] [diff] [review]
Part 1 - Only use tiles on scrollable layers

comment 58 are the review comments.
Attachment #8338641 - Flags: review?(tnikkel)
Attachment #8338644 - Flags: review?(tnikkel)
(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.
Flags: needinfo?(tnikkel)
Reading GetAnimatedGeometryRootFor, it does sound like what you're saying is what should happen...
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.
Flags: needinfo?(tnikkel)
Attachment #8338641 - Attachment is obsolete: true
Attachment #8338644 - Attachment is obsolete: true
Attachment #8339275 - Flags: review?(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?
Attachment #8339275 - Flags: review?(tnikkel) → review+
Comment on attachment 8339275 [details] [diff] [review]
Only use tiles on scrollable layers

Adding r?BenWa for layers changes.
Attachment #8339275 - Flags: review?(bgirard)
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.
Attachment #8339275 - Flags: review?(bgirard) → review-
(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?
Flags: needinfo?(bgirard)
(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.
Flags: needinfo?(bgirard)
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.
Ok, IMO we should land the code now and block flipping the preference on the analysis.
(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.
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).
Depends on: 945277
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.
Attached patch fpsLog.patchSplinter Review
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.
Carrying r=tn.
Attachment #8339275 - Attachment is obsolete: true
Attachment #8341785 - Flags: review+
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.
Attachment #8341785 - Flags: review?(bgirard)
Attachment #8341785 - Flags: review?(bgirard) → review+
+#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.
https://hg.mozilla.org/mozilla-central/rev/954f30096cbe
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 950696
You need to log in before you can comment on or make changes to this bug.