Modify TextureClientPool to keep track of unused tiles instead of overall tile count

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gw280, Assigned: gw280)

Tracking

({feature})

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 1 obsolete attachment)

No description provided.
Keywords: feature
See Also: → 1244148
Whiteboard: [gfx-noted]
I have a wip patch for this. somewhere. I'll try to find it.
So ideally, I think this should just allocate 50 TextureClients on startup, then keep recycling them and never need another allocation. If it does, then it allocates another 50 when the unused pool becomes empty, and the pool effectively becomes 100 clients. It'll shrink down to maximum size of 50 unused if we end up returning a lot of the TextureClients and don't use them again.

I have removed the ShrinkToMinimumSize code as I don't think that makes sense anymore. Clear() is now used for memory pressure events, and I'm wondering if Clear should be renamed to DiscardUnusedTiled() to be a bit more explicit about what it does.
Attachment #8765878 - Flags: review?(jnicol)
Attachment #8765878 - Flags: review?(jmuizelaar)
Comment on attachment 8765878 [details] [diff] [review]
0001-Bug-1279341-Keep-track-of-unused-tiles-instead-of-to.patch

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

I'm concerned that allocating 50 tiles at a time is too much. That's 200MB with a 1024x1024 tile size. Maybe we need a slightly more complicated scheme - aim to keep around 10 unused tiles during normal usage, that should handle scrolling well without using too much extra memory. But it's not enough for tab switch. So maybe we also always want a minimum of 50 tiles (including outstanding ones).

::: gfx/layers/client/TextureClientPool.cpp
@@ +86,5 @@
>    RefPtr<TextureClient> textureClient;
> +  if (!mTextureClients.size()) {
> +    // We have no TextureClients available, so allocate our
> +    // pool of unused TextureClients for this and future requests
> +    AllocateUnusedTextureClients();

If we only call this here then it's down to chance (steady state num tiles % mMaxUnusedTextureClients) whether we have unused tiles lying around when we e.g. scroll? Could we call it in a timeout or at the end of a transaction or something?

@@ +172,3 @@
>    // We cull from the deferred TextureClients first, as we can't reuse those
>    // until they get returned.
> +  uint32_t totalAllocatedTextureClients = mTextureClients.size() + mTextureClientsDeferred.size();

would totalUnusedTextureClients be a better name? We're not tracking outstanding ones.

@@ +184,1 @@
>        if (!mTextureClients.size()) {

I don't think we care about this situation any more, since we're only tracking unused clients.
Attachment #8765878 - Flags: review?(jnicol) → review-
OK, I tried a bunch of allocation strategies and settled on this one. This basically:

- Allocates an initial budget (defaults to 50) for us to use
- If we run out, allocates in 10s and keeps track of our unused pool size to ensure that we always have at most 10 unused tiles in the pool, discarding as necessary when tiles are returned and defer-returned to us.

This seems to work pretty well.

On a MacBook Pro (non-retina), I was seeing tile usage up to around 85 tiles in a test browsing session with 3 tabs (cnn.com, slashdot.org, arstechnica.com). I don't think retina-vs-non-retina would make a difference here, as the tiles are sized based on screen width. 

We might want to consider allocating off the main thread in the future.
Attachment #8765878 - Attachment is obsolete: true
Attachment #8765878 - Flags: review?(jmuizelaar)
Attachment #8771563 - Flags: review?(jnicol)
Attachment #8771563 - Flags: review?(jmuizelaar)
Assignee: nobody → gwright
Also, in order for this patch to work properly, the fix for bug 1286218 needs to be in the tree (landed in m-c a few days ago)
Depends on: 1286218
Posted file tiles.log
Log file of tile allocations on my MBP. In this particular run, we hit 63 outstanding tiles at a single point in time. We also immediate got 21 deferred clients returned to us after hitting the peak of 63, then issued some more from the pool (which were immediately issued because we'd allocated 10 unused), then we started culling from the deferred pool and the immediate pool until we had 10 left in the immediate pool.

I think this is pretty ideal in terms of how we'd want the pool to be managed.
Comment on attachment 8771563 [details] [diff] [review]
0001-Bug-1279341-Keep-track-of-unused-tiles-instead-of-to.patch

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

Looks good, except for how we handle TextureClient creation failure. We want to pass nullptr to the caller of GetTextureClient, but we don't want to fill mTextureClients with nullptrs.

::: gfx/layers/client/TextureClientPool.cpp
@@ +13,5 @@
>  
>  #include "nsComponentManagerUtils.h"
>  
> +//#define TCP_LOG(...)
> +#define TCP_LOG(...) printf_stderr(__VA_ARGS__);

take it this wasn't meant to be included in the patch

@@ +122,5 @@
> +
> +  while (mTextureClients.size() < aSize) {
> +    if (gfxPrefs::ForceShmemTiles()) {
> +      // gfx::BackendType::NONE means use the content backend
> +      mTextureClients.push(TextureClient::CreateForRawBufferAccess(mSurfaceAllocator,

If TextureClient::Create*() fails will we end up with a load of nullptrs in mTextureClients? That's probably not what we want.

::: gfx/layers/client/TextureClientPool.h
@@ +131,5 @@
>    const TextureFlags mFlags;
>  
> +  // The initial number of unused texture clients to seed the pool with
> +  // on construction
> +  uint32_t mInitialUnusedTextureClients;

I'd prefer it if these two names had "Num" or something like that in them. In fact I'd probably go for mInitialPoolSize and mPoolSizeIncrement, but it's up to you. If you do change them remember to change the Constructor args and pref names too.
Attachment #8771563 - Flags: review?(jnicol) → review+
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0affe2255580
Keep track of unused tiles in determining TextureClientPool size instead of overall size r=jnicol
https://hg.mozilla.org/mozilla-central/rev/0affe2255580
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1288994
I still see tile allocations when scrolling https://internals.rust-lang.org/t/summary-of-efficient-inheritance-rfcs/494 on my macbook pro.
Flags: needinfo?(gwright)
I've spun that off into bug 1290303
Flags: needinfo?(gwright)
You need to log in before you can comment on or make changes to this bug.