Right now we're using the default values of 256x256 for the tile size, and a pool limit of 50 tiles. The screen size of a Nexus 9 is 1536x2048, which I believe works out to 48 tiles to cover the screen. That means the pool size of 50 is effectively useless, since we need more than that just to cover the displayport itself. I think for a device like the Nexus 9, both the tile size and pool size should be bigger. It seems like we probably want to make this decision at runtime based on the hardware instead of having it in a pref? Maybe it's enough to have some simple heuristic based on the screen size?

I'd love it if we did a bunch of eideticker runs (or robocheck?) to help figure out what these values should be.

We have the layers.tiles.adjust property already, but it's not used for anything. There is a comment in gfxPlatform.cpp regarding this: // TODO We may want to take the screen size into consideration here. Yeah, we might.

Thoughts: * we should change the layers.tiles.adjust preference to default to true on B2G and Android, but false on the rest. * In gfxPlatform::ComputeTileSize(): ** I'd leave B2G as it was otherwise (code inside of MOZ_WIDGET_GONK) ** add code inside of MOZ_WIDGET_ANDROID (right :snorp?) to figure out what size of the tiles we want * looks like we may need a new gfxPlatform::GetScreenSize (sort of matching what GetScreenDepth does) * :snorp can decide what kind of heuristic we want to use (range 256 to 1024 and power of two? or multiples of 128?) * larger tiles means more wasted space * do we want to keep "50 tiles", or do we want to adjust the size of the cache based on the tile size and the "device" size? * I'd change gfxPlatform::GetTileWidth and gfxPlatform::GetTileHeight to lazily call ::ComputeTileSize to set the tile sizes (if we haven't already explicitly called SetTileSize). It's too fragile as it is.

(In reply to Milan Sreckovic [:milan] from comment #3) Something like this seems about right, but I think we should just move all of it out of gfxPlatform and into CompositorParent/CompositorChild/LayerManager. It makes sense to me that we should base the tile size based on the window size, which can change (on desktop especially), and of course should not be in the gfxPlatform singleton.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4) > (In reply to Milan Sreckovic [:milan] from comment #3) > Something like this seems about right, but I think we should just move all > of it out of gfxPlatform and into > CompositorParent/CompositorChild/LayerManager. It makes sense to me that we > should base the tile size based on the window size, which can change (on > desktop especially), and of course should not be in the gfxPlatform > singleton. Agreed, although we should not base the tile size on the window size, but on the screen resolution and then stick to it even if the window is dragged in a different screen. We don't want to throw away tiles while resizing windows (nor have to deal with a transition time where several tile sizes exist). The number of tiles in the pool can safely vary along with the window size, and we should probably start with that.

This is harder to fix than I thought it would be. We use gfxPlatform::GetTileWidth() in nsLayoutUtils::GetDisplayPort() and friends, well outside of the context of any tiling code. Looks like we need to keep things stored in gfxPlatform for now. I think it could be a PITA to get screen size information in there though.

The screen bounds in the nsWindow are updated way too late to be useful during startup

I don't know if this heuristic is right -- it seems like it might create tiles that are too large. I'm open to ideas.

Again, not sure if this heuristic is what's best. It seems like 3 screens of tiles is about right, I guess?

This is more of a cleanup than anything else. Not sure if it breaks anything yet.

Comment on attachment 8653158 [details] [diff] [review] Part 2 - Use a direct JNI call to determine screen size in nsScreenManagerAndroid Review of attachment 8653158 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/bindings/moz.build @@ +11,3 @@ > 'Bundle', > 'MediaCodec', > + 'SurfaceTexture' You can leave the trailing comma in. ::: widget/android/nsScreenManagerAndroid.cpp @@ -28,5 @@ > > NS_IMETHODIMP > nsScreenAndroid::GetRect(int32_t *outLeft, int32_t *outTop, int32_t *outWidth, int32_t *outHeight) > { > - gfxIntSize sz = nsWindow::GetAndroidScreenBounds(); I think we can get rid of nsWindow::GetAndroidScreenBounds now?

Comment on attachment 8653156 [details] [diff] [review] Part 1 - Add gfxPlatform::GetScreenSize() Review of attachment 8653156 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxPlatform.cpp @@ +2087,5 @@ > + if (!manager) { > + return nullptr; > + } > + > + nsCOMPtr<nsIScreen> screen = nullptr;; nit: Perhaps one semicolon is enough

Comment on attachment 8653159 [details] [diff] [review] Part 3 - Adjust tile sizes depending on the screen size Review of attachment 8653159 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxPlatform.cpp @@ +1016,3 @@ > if (gfxPrefs::LayersTilesAdjust()) { > + gfx::IntSize screenSize = GetScreenSize(); > + w = h = std::min(NextPowerOfTwo(screenSize.width) / 2, 1024); I think we should keep the gralloc-specific adjustment, in addition to looking at the screen size. Also, GetScreenSize can return [0, 0]. I assume it's rare but still we should have a size to default to, maybe 256x256, if the tile size looks suspicious (like < 100) after this computation.

Comment on attachment 8653160 [details] [diff] [review] Use a dynamically computed max tile pool size when layers.tiles.adjust is enabled Review of attachment 8653160 [details] [diff] [review]: ----------------------------------------------------------------- I don't have a good notion of how many tiles we should keep around so the best is probably to scroll around long pages and see how many tiles we use and how many of them just sit around in the pool. r+ because I don't have a specific objection to this heuristic but I'd rather have it backed by some measurements. ::: gfx/layers/client/ClientLayerManager.cpp @@ +672,5 @@ > + gfx::IntSize screenSize = gfxPlatform::GetPlatform()->GetScreenSize(); > + float tileWidth = gfxPlatform::GetPlatform()->GetTileWidth(); > + float tileHeight = gfxPlatform::GetPlatform()->GetTileHeight(); > + > + // We should be able to hold 3 screens worth of tiles note that on some platforms (like b2g) we have double-buffered tiles while on others (such as fennec) we have single-buffered tiles so we may want to take that into account.

(In reply to Nicolas Silva [:nical] from comment #14) > Comment on attachment 8653159 [details] [diff] [review] > Part 3 - Adjust tile sizes depending on the screen size > > Review of attachment 8653159 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/thebes/gfxPlatform.cpp > @@ +1016,3 @@ > > if (gfxPrefs::LayersTilesAdjust()) { > > + gfx::IntSize screenSize = GetScreenSize(); > > + w = h = std::min(NextPowerOfTwo(screenSize.width) / 2, 1024); > > I think we should keep the gralloc-specific adjustment, in addition to > looking at the screen size. > The gralloc bit there actually never performed any adjustment, just made sure it could allocate the tile size. I guess I can put that back and add a fallback size if the allocation fails. > Also, GetScreenSize can return [0, 0]. I assume it's rare but still we > should have a size to default to, maybe 256x256, if the tile size looks > suspicious (like < 100) after this computation. Yeah, sounds like a similar fallback case to the above.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #16) > (In reply to Nicolas Silva [:nical] from comment #14) > > Comment on attachment 8653159 [details] [diff] [review] > > Part 3 - Adjust tile sizes depending on the screen size > > > > Review of attachment 8653159 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: gfx/thebes/gfxPlatform.cpp > > @@ +1016,3 @@ > > > if (gfxPrefs::LayersTilesAdjust()) { > > > + gfx::IntSize screenSize = GetScreenSize(); > > > + w = h = std::min(NextPowerOfTwo(screenSize.width) / 2, 1024); > > > > I think we should keep the gralloc-specific adjustment, in addition to > > looking at the screen size. > > > > The gralloc bit there actually never performed any adjustment, just made > sure it could allocate the tile size. I guess I can put that back and add a > fallback size if the allocation fails. I'm an idiot. I see now that it assigns the width to the stride of the buffer. I'll put that back, but make the test buffer use the computed width/height.

This is a reworked version of the previous gfxPlatform::GetScreenSize() patch. nsIScreenManager is not thread safe, and we use gfxPlatform::GetScreenDepth() from the compositor thread on Android. This version of the patch gets the values once at startup and caches them. It also folds in the GetScreenDepth() patch since it was so similar.

This version of the patch just doubles the pool size if we are double buffering

Comment on attachment 8654183 [details] [diff] [review] Use a dynamically computed max tile pool size when layers.tiles.adjust is enabled Review of attachment 8654183 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/ClientLayerManager.cpp @@ +681,5 @@ > + (screenSize.height / tileHeight)) * 3; > + > + // If we're going to be double buffering, double it > + if (gfxPlatform::GetPlatform()->CanUseDoubleBufferedContent(aBackend)) { > + maxSize *= 2; I am not certain x2 is actually the best heuristic since double-buffered tiles only have a back buffer if they have been painted more than once, and the back buffer expires after a little while, but we can adjust later (generally double-buffered will use more textures than non-double-buffered ones but probably not twice as many except when we repaint the entire viewport). We should watch out for b2g, and make sure the memory usage stays reasonable when browsing around with these new heuristics.

This is Fennec specific change?

(In reply to Milan Sreckovic [:milan] from comment #23) > This is Fennec specific change? Nope, it will change all platforms that use tiling (which I think is Fennec, B2G, Mac?)

That's a big change, with possible far reaching effect. Bug should be set to all platforms, summary changed, and we need more conversations about this if it's going cross platform. Let's not land until that's all sorted out.

I played around with various tile sizes today and yesterday. Sizes smaller than 256 (especially very small sizes like 32) definitely have an adverse affect on compositor performance. My nexus 4 got about 10fps with 32px tiles. It got dramatically worse during panning. A tile size of 512 has no affect in steady state (since we were already getting 60fps in full-tilt mode), but the frame rate seems better when panning. Presumably this is because we avoid some per-upload penalty with fewer tiles, in addition to fewer draw calls. I think the current algorithm (which is NextPowerOfTwo(screen.width) / 2, clamped between 256 and 1024) is probably alright. I think we should probably fix the single-tile-layer path on Android first, though.

I'd agree that 256-1024 range is good for the "automatic sizing".

Comment on attachment 8656808 [details] [diff] [review] Don't try to call JNI methods in nsScreenManagerAndroid if it's not available Review of attachment 8656808 [details] [diff] [review]: ----------------------------------------------------------------- An implicit check (nsresult return value?) would be nice, but I don't see how we could do that without breaking the interface.

I am not convinced the pool size changes are helping anything. I'm going to leave that patch out for now and file a new bug to get more data about that. We might instead want to reconsider the displayport size and biasing, etc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8549221c366 https://hg.mozilla.org/integration/mozilla-inbound/rev/4080fb4b9a7f https://hg.mozilla.org/integration/mozilla-inbound/rev/9559cead8d08 https://hg.mozilla.org/integration/mozilla-inbound/rev/719a4fbded10 https://hg.mozilla.org/integration/mozilla-inbound/rev/4b301ec04c21

Probably a good idea to see what this may to do different B2G devices.

Will let qanalysts know, and I'll test drive the Monday's build. Leaving ni? as a reminder.

Backed out along with everything else in the push for android reftest orange: https://treeherder.mozilla.org/logviewer.html#?job_id=13972013&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/d48fbf99a903 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0f7cb0fc3a

clearing ni? until it is merged again

Things were passing before, but now these couple tests need some additional fuzzing.

The pandaboard does not like the larger tile size in debug tests. There seems to be weirdness with reading back the compositor (but not with actually compositing). We're in the process of killing these tests on the pandaboards anyway, so just use the smaller tile size there. It also disables the single-tile-layer stuff, since that can create a larger tile as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f77fcc543d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/157975791d7c https://hg.mozilla.org/integration/mozilla-inbound/rev/9fea88097171 https://hg.mozilla.org/integration/mozilla-inbound/rev/2e58c9d16db7 https://hg.mozilla.org/integration/mozilla-inbound/rev/f373b1897cac https://hg.mozilla.org/integration/mozilla-inbound/rev/97809a7cd494

https://hg.mozilla.org/mozilla-central/rev/3f77fcc543d8 https://hg.mozilla.org/mozilla-central/rev/157975791d7c https://hg.mozilla.org/mozilla-central/rev/9fea88097171 https://hg.mozilla.org/mozilla-central/rev/2e58c9d16db7 https://hg.mozilla.org/mozilla-central/rev/f373b1897cac https://hg.mozilla.org/mozilla-central/rev/97809a7cd494