[cc] Evict UIResources when the renderer is not visible

[cc] Evict UIResources when the renderer is not visible

Track the number of times that UIResources have been evicted
on the impl thread. Send this number to the main thread at the
beginning of each frame. If the main thread sees this value increase,
then recreate all resources and send an acknowledgement of this
recreation in the UI resource update queue.

Disallow drawing the LayerTreeImpl when it has not received the
acknowledgement of the recreation of all evicted UI resources.
Disallow aborting commits that will recreate UI resources.

BUG=279438
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222448

[ccameron, 2013-09-09 09:16:43]

This builds on top of the previous testing-rearrangement change.

With this in place, I can start using the linen texture as a UI resource.

https://codereview.chromium.org/23548022/diff/2001/cc/trees/layer_tree_host_u...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/23548022/diff/2001/cc/trees/layer_tree_host_u...
cc/trees/layer_tree_host_unittest_context.cc:1841: // Sequence 1 (continued):
This is somewhat simplified by the fact that resources are recreated through the
commit flow, instead of through the LayerTreeHost::DidLoseOutputSurface call.

[powei, 2013-09-09 20:34:25]

https://codereview.chromium.org/23548022/diff/2001/cc/trees/layer_tree_host_i...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/23548022/diff/2001/cc/trees/layer_tree_host_i...
cc/trees/layer_tree_host_impl.cc:2666: client_->RenewTreePriority();
Maybe a stupid question, but what's the purpose of calling this method?  Is
there an implicit contract that says priority should be updated when CanDraw
changes?

[ccameron, 2013-09-09 21:01:16]

Thanks!

https://codereview.chromium.org/23548022/diff/2001/cc/trees/layer_tree_host_i...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/23548022/diff/2001/cc/trees/layer_tree_host_i...
cc/trees/layer_tree_host_impl.cc:2666: client_->RenewTreePriority();
On 2013/09/09 20:34:25, powei wrote:
> Maybe a stupid question, but what's the purpose of calling this method?  Is
> there an implicit contract that says priority should be updated when CanDraw
> changes?

Oops! I needed to update RenewTreePriority to "prefer new content" when this
happens... that way we'll switch to the new tree (with all its UI resources).
Without that change, this did nothing...

[aelias_OOO_until_Jul13, 2013-09-09 22:52:37]

https://codereview.chromium.org/23548022/diff/7001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/23548022/diff/7001/cc/trees/layer_tree_host.h...
cc/trees/layer_tree_host.h:93: UIResourceEvictionRecreated,
Why is it necessary to track this in a different category than "Create"?  I'd
prefer to have the same enum value for both unless we have some compelling
reason.  The lost context case doesn't need a different value.

https://codereview.chromium.org/23548022/diff/7001/cc/trees/layer_tree_host.h...
cc/trees/layer_tree_host.h:341: uint64 ui_resource_eviction_count_recreated_;
It doesn't make sense to me to use a count here.  I don't see how the precise
number of evicted resources is useful for anything.  It would make more sense to
either have a boolean, or have a list of evicted UIResourceIDs.

[ccameron, 2013-09-09 23:22:04]

Thanks!!

I do feel quite strongly about the structure that I chose here. Hopefully I've
described the reasoning clearly below -- if you have any questions, I'd be happy
to drop by and discuss in person.

Your remark about the naming of the "count" being ambiguous is well taken, and
maybe we can come up with a better name.

https://codereview.chromium.org/23548022/diff/7001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/23548022/diff/7001/cc/trees/layer_tree_host.h...
cc/trees/layer_tree_host.h:93: UIResourceEvictionRecreated,
On 2013/09/09 22:52:37, aelias wrote:
> Why is it necessary to track this in a different category than "Create"?  I'd
> prefer to have the same enum value for both unless we have some compelling
> reason.  The lost context case doesn't need a different value.

I strongly prefer that this be done explicitly -- it is much easier for me trace
through the sequence mentally (see further elaboration below).

WRT using Create, consider the following situation.
1. main thread creates 1 resource
2. that resource is passed to the impl thread and allocated
3. main thread deletes that resource
4. a lost context happens (while the delete is still in the queue)
The LTI is perfectly up-to-date, but it won't know that because it didn't and
won't get a Create to tell it that.

Now, this is something of a straw-man because we can say "well, apply it to
Delete as well Create". The next comment discusses that issue in more detail.

https://codereview.chromium.org/23548022/diff/7001/cc/trees/layer_tree_host.h...
cc/trees/layer_tree_host.h:341: uint64 ui_resource_eviction_count_recreated_;
On 2013/09/09 22:52:37, aelias wrote:
> It doesn't make sense to me to use a count here.  I don't see how the precise
> number of evicted resources is useful for anything.  It would make more sense
to
> either have a boolean, or have a list of evicted UIResourceIDs.

This isn't the number of resource that have been evicted -- it is the number of
times that an eviction has happened. LMK if you have ideas for variable naming
here (I played with some others, but I thought that the comments in LTHI.h were
enough -- I can add more here).

That said, I feel very strongly that this is much easier to follow and verify
correctness of.

The reason for a counter instead of a boolean is that it isn't immediately clear
to me that a boolean will be sufficient.

In the case of the (largely symmetric) ContentsTexturesPurged situation, we use
a bool, but I'd prefer we didn't. We can use a bool in that case because the
value is updated in PushPropertiesTo rather than tree activation, so the fact
that only outstanding 1 commit can exist makes it so that we don't have about
having multiple pending "relevant" ContentsTexturesPurged events outstanding.

In particular, the following situation appears possible to me
1. to start, there exist some UI resources
2. a new resource is created on the main thread
3. that resource is passed to the impl thread and the queue is hung off a LTI
4. the UI old resources are evicted before the queue with the new resource is
processed
5. the LTI is activated, and the queue is processed

Now, if we use a bool, then, when the queue is processed, we will say "all
resources have been recreated, we're good". But we're not -- that queue doesn't
have the recreation of the resources destroyed in step 4.

If we use a bool, then when thinking about this code, we have to prove that the
above sequence can't happen. If we use this eviction count/sequence-number
scheme, we don't have to make this proof every time we consider modifying this
code.

[aelias_OOO_until_Jul13, 2013-09-09 23:44:30]

https://codereview.chromium.org/23548022/diff/7001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/23548022/diff/7001/cc/trees/layer_tree_host.h...
cc/trees/layer_tree_host.h:341: uint64 ui_resource_eviction_count_recreated_;
I agree with your reasoning that a bool is insufficient.  What about my other
suggestion, to store a list of evicted UIResourceIDs?  Or, equivalently, have an
additional bool in the struct stored in the map (Powei is making the LTH map
hold a struct in his NinePatchLayer patch) about whether a given resource needs
recreation?

[ccameron, 2013-09-10 00:10:51]

On 2013/09/09 23:44:30, aelias wrote:
> What about my other
> suggestion, to store a list of evicted UIResourceIDs?  Or, equivalently, have
an
> additional bool in the struct stored in the map

We went this way for a while with the ContentsTexturesPurged, but it added a
fair amount of complexity -- we ended up getting rid of it in
https://chromiumcodereview.appspot.com/11411251.

I think it's easier to reason about being "up to date" with all evictions rather
than checking their state. It's also less data to pass around, from a
memory+performance perspective (not that that's a compelling reason on the scale
we're doing).

Also, I'm thinking to replace the term "count" with "seqnum" -- do you think
that would improve clarity?

[aelias_OOO_until_Jul13, 2013-09-10 00:43:41]

Sorry but I think this sequence number is the sort of concept that is crystal
clear in the original author's mind but won't be understandable to anyone else
browsing the code.  It's a magic number indirectly representing the set of
UIResources existing at a given time.  It would be much more direct and readable
to simply hold the list of evicted UIResources.

How about marking them all evicted on the LTHI in a separate
"std::set<UIResourceID> evicted_ui_resources_".  When LTH goes visible, send a
normal "Create" request for all LTH-known UIResources.  On the LTHI, either a
Create or Delete request removes a UIResourceID from evicted_ui_resources_.  And
block the draw on whether evicted_ui_resources.empty().  I can't think of a
scenario where that doesn't work, can you?

[ccameron, 2013-09-10 19:47:07]

On 2013/09/10 00:43:41, aelias wrote:
> Sorry but I think this sequence number is the sort of concept that is crystal
> clear in the original author's mind but won't be understandable to anyone else
> browsing the code.  It's a magic number indirectly representing the set of
> UIResources existing at a given time.  It would be much more direct and
readable
> to simply hold the list of evicted UIResources.
> 
> How about marking them all evicted on the LTHI in a separate
> "std::set<UIResourceID> evicted_ui_resources_".  When LTH goes visible, send a
> normal "Create" request for all LTH-known UIResources.  On the LTHI, either a
> Create or Delete request removes a UIResourceID from evicted_ui_resources_. 
And
> block the draw on whether evicted_ui_resources.empty().  I can't think of a
> scenario where that doesn't work, can you?

That scheme will work ... and is much simpler ... so I've implemented and
uploaded it.

(but I still think the sequence number thing is cool)

[aelias_OOO_until_Jul13, 2013-09-10 20:02:32]

Thanks!

https://chromiumcodereview.appspot.com/23548022/diff/20001/cc/trees/thread_pr...
File cc/trees/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/23548022/diff/20001/cc/trees/thread_pr...
cc/trees/thread_proxy.cc:770: // Recreate all UI resources if there were evicted
UI resources when the impl
Hmm, I don't know of anything that guarantees no commit will ever occur when
invisible, so you're at risk of a random commit on an invisible tab defeating
your memory saving.  Do we need to tie recreation more explicitly to visibility?

[ccameron, 2013-09-10 21:27:49]

https://chromiumcodereview.appspot.com/23548022/diff/20001/cc/trees/thread_pr...
File cc/trees/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/23548022/diff/20001/cc/trees/thread_pr...
cc/trees/thread_proxy.cc:770: // Recreate all UI resources if there were evicted
UI resources when the impl
On 2013/09/10 20:02:33, aelias wrote:
> Hmm, I don't know of anything that guarantees no commit will ever occur when
> invisible, so you're at risk of a random commit on an invisible tab defeating
> your memory saving.

Only forced commits will happen, which I'm told is very rare when not visible
(like, for the thumbnailer). It looks like there are some issues with resource
eviction in that path already. In particular
- either all non-UI memory is evicted and the thumbnailer paints nothing
- the non-UI memory is recreated and we end up wasting GPU memory

Given the situation, I'd prefer to leave this as it is, and address "what
exactly do non-visible commits get" in another bug.

> Do we need to tie recreation more explicitly to visibility?

I prefer recreation coming in through commits rather than visibility, because it
is easier to reason about correctness through commits. Also, it matches
ContentsTexturesPurged, so there are fewer distinct systems.

It may be that we should re-dump all resources after forced commits when
backgrounded -- but this needs to be fixed for UI and non-UI resources.

[aelias_OOO_until_Jul13, 2013-09-10 21:39:17]

Sounds reasonable, lgtm.

[powei, 2013-09-10 21:50:51]

Just a quick typo before you land this.  lgtm.

https://chromiumcodereview.appspot.com/23548022/diff/20001/cc/trees/layer_tre...
File cc/trees/layer_tree_impl.cc (right):

https://chromiumcodereview.appspot.com/23548022/diff/20001/cc/trees/layer_tre...
cc/trees/layer_tree_impl.cc:633: // If all UI resource evictions were not be
recreated by this commit, then
be

[ccameron, 2013-09-10 22:06:14]

Thanks!!

[commit-bot: I haz the power, 2013-09-10 22:08:44]

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

[commit-bot: I haz the power, 2013-09-11 01:44:49]

Change committed as 222448