Closed Bug 1243589 Opened 9 years ago Closed 9 years ago

Don't use tiled layers for layers that are smaller than a single tile

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox45 --- affected
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: mstange, Assigned: kats)

References

Details

Attachments

(1 file, 1 obsolete file)

MozReview Request: Bug 1243589 - Use SingleTiledContentClient even for scrollable layers if the layer is smaller than a single tile. r?mattwoodrow
58 bytes, text/x-review-board-request
mattwoodrow
: review+
lizzard
: approval-mozilla-aurora+
Details 
In bug 1239615 we're dealing with a case where we're creating many small layers (28x14) which are all part of a scrollable piece of content. Using tiles for those layers is a big  waste of memory and of performance (clearing the tile background + texture upload).
We're correctly marking these layers as scrollable - if they were bigger, we'd absolutely want to use tiles for them. So I think we should add an explicit size check.
This patch has a small problem: Layers that start out small will stay single-tile layers even as they grow, which is not great because single tiled layers aren't optimized for size changes. One case where that might happen is if we have a long scrollable page where an extra layer starts somewhere down the page in such a way that initially only a small part of it is inside the display port. As the display port shifts down, the layer will grow larger, but we won't use tiles for it.
Actually, this will only happen if both the width and the height of the layer are initially smaller than one tile, so this problem might not be frequent enough to be worth worrying about.
Matt, what do you think? Worth fixing? If so, do you have a good idea how? I suppose I have to add another virtual method to TiledContentClient that tells us what type of layer it is, or I have to track the content client type we created inside ClientTiledPaintedLayer.
We also have the related problem where we'll have to reallocate textures for all size changes. Even if the size stays small (like they will for bug 1239615), any layer that is partially visible will require texture allocation when scrolling.
(In reply to Markus Stange [:mstange] from comment #2)
> Matt, what do you think? Worth fixing? If so, do you have a good idea how? I
> suppose I have to add another virtual method to TiledContentClient that
> tells us what type of layer it is, or I have to track the content client
> type we created inside ClientTiledPaintedLayer.

I think it is worth fixing. Either one of those seems fine, I prefer the latter by a small margin.
I stole Markus' patch and finished it off based on the comments above. I'm not really sure how to test this patch - please review carefully and let me know if anything looks amiss!
Assignee: mstange → bugmail.mozilla
Attachment #8712955 - Attachment is obsolete: true
Attachment #8712955 - Flags: review?(matt.woodrow)
Oh also note that Markus used layerSize < tileSize in his patch, I changed it to <= because it seemed more correct. I can change it back to < if I thought wrong.
This is something I'd like to uplift to 46 if it's safe enough. The tile issue exists in 44 and 45 as well and is likely the cause behind both bug 1239615 and bug 1243554. With APZ in 46 the displayport is larger so we potentially create more of these tiles than we were doing before, and so in 46 the resulting jankiness is worse. Uplifting this to 46 would fix that.
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox46: --- → affected
Attachment #8718124 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8718124 [details]
MozReview Request: Bug 1243589 - Use SingleTiledContentClient even for scrollable layers if the layer is smaller than a single tile. r?mattwoodrow

https://reviewboard.mozilla.org/r/34417/#review31149
Looks like there is an android invalidation reftest failure in the try push above, I'll investigate tomorrow.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> This is something I'd like to uplift to 46 if it's safe enough. The tile
> issue exists in 44 and 45 as well and is likely the cause behind both bug
> 1239615 and bug 1243554. With APZ in 46 the displayport is larger so we
> potentially create more of these tiles than we were doing before, and so in
> 46 the resulting jankiness is worse. Uplifting this to 46 would fix that.
Yes please! Devtools heavily relies on codemirror editors (in the debugger, style-editor, inspector and scratchpad), and we started noticing the jank in 46.
46 is dev-edition which is a "product" we market at developers. So we really would like to address this issue in 46.
Thanks a lot for fixing this!
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> Looks like there is an android invalidation reftest failure in the try push
> above, I'll investigate tomorrow.

So the failure is caused because the test starts off with one element on a layer, which is 200x200. It then makes a second element visible, which goes on the same layer expanding it to 440x200. On Fennec the tile size is 256x256 so this causes it to go from being a single-tile to a multi-tile, and that causes the elements to get repainted. And the test is trying to ensure that the first element doesn't get repainted when the second one is made visible.

I think the test is checking for invalidation in layout code rather than due to tiling changes, so I think we should just work around the failure because it's not particularly relevant to this change. I would prefer to just force a larger tile size for that test, but the tile size can't be changed after startup, so instead I'm going to try shrinking the elements in the reftest so that they fit on one tile. Squishing all the width/left CSS properties by 50% should do the trick and maintain the purpose of the test.
After some more investigation I think the test was written specifically to exercise MultiTiledContentClient rather than SingleTiledContentClient. With SingleTiledContentClient any changes in size will trigger a repaint of the full area [1], I think because it needs to allocate a new texture. And it's not Fennec-specific; this test fails on OS X as well with the patch (at least locally, I triggered the jobs on the try push to confirm).

So now I'm thinking that the patch here makes the test invalid. To make the test valid again we would need to force it back onto the MultiTiledContentClient path on all platforms. One way to do this is to force the content to take up more than one tile of space. A better way is to set the layers.single-tile.enabled pref to false for the test. That seems to work locally, I did a try push to confirm [2].

[1] https://dxr.mozilla.org/mozilla-central/rev/ac39fba33c6daf95b2cda71e588ca18e2eb752ab/gfx/layers/client/SingleTiledContentClient.cpp#128
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=734259dd07c3
*sigh* Same problem on layout/reftests/invalidation/fast-scrolling.html, but only on OS X because the tile size is larger there than on Fennec. I'm setting the pref on that one too.
Ok, with that OS X is green as well, https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ff6b165bbd5 - will land in a sec.
Same problem, but in three mochitests. Flipping the pref on them seems to fix it locally. Try push to confirm: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed47a60d7f0e

I discussed with Matt on IRC and we decided that we should land this patch with the pref flipped on the failing tests, but also file a follow up to enhance SingleTiledContentClient so that it avoids repainting where possible. I'll put more details in the bug I file.
Flags: needinfo?(bugmail.mozilla)
Filed bug 1247827 as a follow-up, and the try is green, so re-landing.
https://hg.mozilla.org/mozilla-central/rev/9bce2f3c14e8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8718124 [details]
MozReview Request: Bug 1243589 - Use SingleTiledContentClient even for scrollable layers if the layer is smaller than a single tile. r?mattwoodrow

Approval Request Comment
[Feature/regressing bug #]: side-effect from bug 1221073, aggravated by APZ
[User impact if declined]: scrolling on some things (e.g. codemirror views, which includes the devtools code views) can be extremely janky. See for example bug 1239615 and bug 1243554 which are both examples.
[Describe test coverage new/current, TreeHerder]: tested locally, on m-c. There are automated tests that cover some of this code
[Risks and why]: There is a risk that this patch will result in more repainting and allocation churn on the content process, but with APZ that should not affect the compositor thread and will be mostly hidden from the user. I filed a follow-up bug to address this issue. Even taking this possible regression into account I think we should uplift this because it improves the devtools use-cases very significantly.
[String/UUID change made/needed]: none
Attachment #8718124 - Flags: approval-mozilla-aurora?
Comment on attachment 8718124 [details]
MozReview Request: Bug 1243589 - Use SingleTiledContentClient even for scrollable layers if the layer is smaller than a single tile. r?mattwoodrow

Some risk here but should improve apz/dev tools for users.
Attachment #8718124 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: