Fix issue incremental upload can evict textures being drawn

Fix issue where textures currently being drawn by the impl thread can be recycled or evicted by the main thread.

Explicitly push priorities to their backings when a commit begins, and then update the presence of backings' resources in the impl tree when the trees are synchronized.

Note that in the worst case we can momentarily overshoot the texture budget by a factor of two because we keep around both the currently-drawing and the next frame.

Make the CCProxy implementations call the PTM directly instead of maintaining a bunch of wrapper functions in CCLayerTreeHost.

Make reduceMemory (called when synchronizing trees at the end of a commit) be the one true place where backings structures are deleted (they are just evicted in all other places).

Also, we can cut the PTM into a CCPrioritizedTextureManager and CCPrioritizedBackingManager where the former is a main-thread structure and the latter is an impl-thread structure. This change make the PTM follow parts of the commit flow, which will be more explicit with the separate structures.

BUG=152496
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161949

[epenner, 2012-10-06 05:53:13]

On 2012/10/06 00:09:37, ccameron1 wrote:

As a bit of a history for this bug:
The reason the PTM pushes all textures to the impl thread is that there is
custom code in TiledLayerChromium to double-buffer incremental updates. As it
turns out, that code was always redundant. We can't push any textures that we
may evict (or now recycle), period.

Functionally, my main concern with is this CL appears to unlink lots of textures
to insure they are recyclable. In doing that, the owners of those textures have
now lost all those textures, so they will need to repaint those textures, which
is very slow and costly. Priorities are intended to wildly change when the user
changes scroll direction etc, with painting only catching up much later.

Did you have any thoughts on the quick CL I put together?
(https://codereview.chromium.org/11048004/)  That together with simply changing
the condition used when pushing textures to the impl side should be sufficient
to solve this.  Currently callers use "haveTexture()" to see if they should push
a texture to the impl side, whereas they should have been using something like
"canUseTexture() { return backing->abovePriorityCutoff; }". I think variables
such as 'inNextImplTree/priorityInNextImplTree' indicate the commit logic is not
timed right. When things are working correctly, there should only be one
canonical version, and no need to save multiple next/previous priorities etc.

This is my understanding of what the flow should be to fix this, where there is
only one canonical version of everything:

- Layers all prioritize their textures
- PTM::PrioritizeTextures
- Several upload frames recycle from below below-priority-cutoff backings
(correctly prioritized as how they were put in the impl tree!)
- PTM::PrioritizeBackings is called only once (not every upload) just before
SynchronizeTrees
- SynchronizeTrees (which now pushes only the above cut-off backings into the
tree)
- Repeat

Is there a problem with that flow or a reason it wouldn't work? I think it
solves it the flow alone, with no extra member variables or backing lists. On a
related note, I'm cleaning up all the code in TiledLayerChromium, as after this
I realized it never served a purpose except for identifying partial updates.

[ccameron, 2012-10-07 21:32:38]

The CL that you uploaded fixes the issue and is overall much simpler.  That
said, there are some things that I think the additional information in this CL
helps with.  We should sync up offline to decide on a plan for this.

The things that I think this CL adds for the extra code are:

impl-side-texture-eviction is more precise (this is a big one for me): By
keeping around data for priorities in the in-the-process-of-being-committed and
the currently-being-drawn frames, impl-side-texture-eviction can know exactly
what it is deleting.  The fact that the previous and next frames were basically
merged before bothered me.  The reason why I take 2 snapshots (as opposed to
just the snapshot and a call to owner()) is that impl-side-texture-eviction
can't necessarily call owner(), because the main thread may be messing around
with it.

texture recycling for final upload: I re-allow aggressive recycling (including
things in the impl tree) during the final upload (which is the only upload when
there isn't threaded compositing).  This does add some complexity, but results
in greater recycling.

the amount of painting should be unaffected (IIUC):
> Functionally, my main concern with is this CL appears to unlink lots of
textures
> to insure they are recyclable. In doing that, the owners of those textures
have
> now lost all those textures, so they will need to repaint those textures,
which
IIUC, the PTM as it is now is very surgical about what it marks as "not above
the priority cutoff", and almost everything that doesn't make the cutoff is
deleted during uploads or the subsequent reduceMemory -- things only don't make
the cutoff if there isn't memory for them.  So unlinking these things at the
beginning of the frame shouldn't result in additional painting.  I haven't
empirically tested this yet though -- I can do that.

In the future (maybe very soon, if it's easy), I'd like to allow the PTM's
beginFrameComplete to steal backings from the impl-tree (and have the impl tree
remove references to those backings), to get us back to the previous level of
recycling.

We should also discuss how this can be made to work with 0-copy and 1-copy.

[epenner, 2012-10-07 22:33:15]

Sounds good, let's meet up to discuss. If anything though, we shouldn't add
functionality until we have first fixed the bug. Did you land the m23 fix on
trunk?  I think we should either land that, or something based off off my
minimal fix, before we add extra functionality.

>impl-side-texture-eviction is more precise (this is a big one for me)

Ah, I think I get the end goal. I think what you are aiming for would be the
ability to do something like evictNonVisibleTilesOnImplThread?  I still don't
think having newer main thread priorities will help for that. First, the
majority of frames where this could happen are in-paint frames, when we don't
yet have the main thread priorities anyway. Second, the main-thread priorities
are also out-of-date, so impl-side eviction will still not be accurate if you
want to do evictions accurately.  Lastly, if you were to re-prioritize on the
impl-thread (according the the actual layer positions etc), then the main-thread
priorities would be ancient in comparison, so then you still wouldn't need them.
Only prioritizing on the impl-thread will solve your problem, and in that case
the code should remain as: paint using main-thread priorities and evict based on
impl-side priorities.

> IIUC, the PTM as it is now is very surgical about what it marks as "not above
the priority cutoff", and almost everything that doesn't make the cutoff is
deleted during uploads or the subsequent reduceMemory

Here's an example. You have outstanding requests for an entire page of 1000MB,
and the limit is 100MB, 20MB of which is visible. As you scroll up, to the top
of the painted area, unpainted priorities will account for 50% of the budget if
we prioritize based on distance. 

Part of the reason this doesn't happen currently is the particular behavior or
TiledLayerChromium, but as it gets better the above case accurately describes
what will happen.

[epenner, 2012-10-07 22:50:40]

> Part of the reason this doesn't happen currently is the particular behavior or
> TiledLayerChromium, but as it gets better the above case accurately describes
> what will happen.

Also, if we add scroll direction as a multiplier on priority, then now almost
all the above-priority-cutoff memory would be unpainted. When you start
scrolling the other direction, everything would be gone and need to be
repainted.

[ccameron, 2012-10-08 08:17:15]

On 2012/10/07 22:33:15, epenner wrote:
> Sounds good, let's meet up to discuss. If anything though, we shouldn't add
> functionality until we have first fixed the bug. Did you land the m23 fix on
> trunk?  I think we should either land that, or something based off off my
> minimal fix, before we add extra functionality.
> 
> >impl-side-texture-eviction is more precise (this is a big one for me)
> 
> Ah, I think I get the end goal. I think what you are aiming for would be the
> ability to do something like evictNonVisibleTilesOnImplThread?  I still don't
> think having newer main thread priorities will help for that.

Yes, the impl-side remains ignorant of any of the main-thread
work/prioritization/painting going on before beginFrameCompleteOnImplThread.

That wasn't what I was trying to address -- the particular scenario I'm
concerned about here is incremental texture upload -- the time between
beginFrameCompleteOnImplThread and scheduledActionCommit (during which the main
tread is blocked).  During that time, we (at least in the current implementation
-- this is where impl side painting may change everything) have the complete
list of painted tiles to upload, and their new priorities, and the entire new
layertreehost, we just haven't haven't pushed them to the the layertreehostimpl
yet (we're still drawing out of the old one).  

It's a kind-of-screwy time, and having a clear picture of "we're
mostly-committed to this next frame, but it's not there yet" really helped me
reason about things.

I think a date with the whiteboard near the dungeon will clear this up.

> > IIUC, the PTM as it is now is very surgical about what it marks as "not
above
> the priority cutoff", and almost everything that doesn't make the cutoff is
> deleted during uploads or the subsequent reduceMemory
> 
> Here's an example...

Ah, got it -- you're right.

I do like the idea of doing all of the recycling in one go, since ordering
recycling decisions by upload order can result in suboptimal recycling (e.g,
upload #1 evicts something that could have been recycled to back upload #2). 
But my patch doesn't actually do that.

[ccameron, 2012-10-09 08:36:49]

Eric and I talked about this offline, and we decided to keep the two-phase "push
priorities at begin frame", then "update being in impl tree at commit"
structure, but, we only track one priority (the priority from the begin-frame).

I removed some of the structural changes with respect to tracking the list of
recyclable backings.

I also cleaned up some of the liftime issues (making reduceMemory be the One
True Place where evicted backings are unlinked and destroyed).  This involved
changing some of the signatures of CCPrioritizedTextureManager functions, and so
instead of updating the wrapper functions in CCLayerTreeHost, I just made the
proxy implementations call into the CCPrioritizedTextureManager directly.

ptal.

[epenner, 2012-10-09 17:25:04]

Looking good.

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
File cc/CCPrioritizedTextureManager.cpp (right):

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
cc/CCPrioritizedTextureManager.cpp:402: ASSERT(m_backings.find(backing) ==
m_backings.end());
Maybe ASSERT that it's in the evicted list?

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
cc/CCPrioritizedTextureManager.cpp:414: if (!(*it)->canBeRecycled())
This is good to have for if we enable more recycling.

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
File cc/CCPrioritizedTextureManager.h (right):

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
cc/CCPrioritizedTextureManager.h:60: bool linkedEvictedBackingsExist() const;
Nit: I was having trouble understanding what this function meant and 'linked' is
kind of an implementation detail. Maybe "backingsWereLost" or something that
that indicates an unexpected eviction has occurred?

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
cc/CCPrioritizedTextureManager.h:99: // Sort first by priority (note that
backings that no longer have owners will
If we make some backings recyclable by intentionally not pushing them into the
tree, then it looks like pushing the new priorities too early will mess with
their eviction order and disable that recycling (the recyclable textures will no
longer be at the beginning of this list). This is why I prefer to think of the
backings being in eviction order, as determined at sync-trees.

Anyway, it's not an issue as it stands since this doesn't keep recycling of
still-linked textures, but it makes me think we may need to revisit this again.

[epenner, 2012-10-09 17:27:21]

On 2012/10/09 17:25:04, epenner wrote:
> Looking good.

The description needs updating as a lot has changed.

[ccameron, 2012-10-09 21:03:56]

Thanks!

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
File cc/CCPrioritizedTextureManager.cpp (right):

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
cc/CCPrioritizedTextureManager.cpp:402: ASSERT(m_backings.find(backing) ==
m_backings.end());
On 2012/10/09 17:25:04, epenner wrote:
> Maybe ASSERT that it's in the evicted list?

Done.

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
File cc/CCPrioritizedTextureManager.h (right):

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
cc/CCPrioritizedTextureManager.h:60: bool linkedEvictedBackingsExist() const;
I added a comment saying that this returns true if "there exist any textures
that are linked to backings that have had their resources evicted", and some
more detail.

https://codereview.chromium.org/11079007/diff/7001/cc/CCPrioritizedTextureMan...
cc/CCPrioritizedTextureManager.h:99: // Sort first by priority (note that
backings that no longer have owners will
I went ahead and put a "canBeRecycled" test first.

[ccameron, 2012-10-11 20:28:21]

Resolved against head.  Ptal.

[ccameron, 2012-10-12 19:12:46]

ptal

[ccameron, 2012-10-13 00:21:33]

Resolved against The Great Renaming.

[ccameron, 2012-10-15 17:18:01]

ptal

[enne (OOO), 2012-10-15 18:12:15]

lgtm

lgtm

> Also, we can cut the PTM into a CCPrioritizedTextureManager and
CCPrioritizedBackingManager where the former is a main-thread structure and the
latter is an impl-thread structure.

Everything old is new again.

[jamesr, 2012-10-15 18:13:14]

On 2012/10/15 18:12:15, enne wrote:
> lgtm
> 
> lgtm
> 
> > Also, we can cut the PTM into a CCPrioritizedTextureManager and
> CCPrioritizedBackingManager where the former is a main-thread structure and
the
> latter is an impl-thread structure.
> 
> Everything old is new again.

http://t1.gstatic.com/images?q=tbn:ANd9GcTfWhqIbmf4wDwNx7ggpC0iY1Ezr6ZWTEPeOX...

[enne (OOO), 2012-10-15 18:15:08]

More seriously, before such a cut happens, I'd like to whiteboard out how such a
split might work (or not) in an impl-side painting world.

[ccameron, 2012-10-15 18:29:11]

Thanks!!

On 2012/10/15 18:15:08, enne wrote:
> More seriously, before such a cut happens, I'd like to whiteboard out how such
a
> split might work (or not) in an impl-side painting world.

Yes, certainly.

[commit-bot: I haz the power, 2012-10-15 18:29:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/11079007/21001

[commit-bot: I haz the power, 2012-10-15 20:43:27]

Change committed as 161949