Remove use of wtf/ListHashSet from CCPrioritizedTextureManager.

Remove use of wtf/ListHashSet from CCPrioritizedTextureManager.

We actually don't need the O(1) lookup for backings, so just use a list.

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

[ccameron, 2012-10-17 01:10:51]

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
File cc/prioritized_texture_manager.cc (right):

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.cc:353: m_backings.push_front(backing);
There was some controversy about doing this "add to the front of the list, then
remove from the front of the list and put at the back of the list". The reason
for this is that on some platforms we will want to be able to call createBacking
when not paired with tying it to a backing, in order to avoid driver overhead of
creating resources

[jamesr, 2012-10-17 01:31:11]

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
File cc/prioritized_texture_manager.cc (left):

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.cc:131: BackingVector& sortedBackings =
m_tempBackingVector;
can we get rid of the m_tempBackingVector member completely now?

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
File cc/prioritized_texture_manager.cc (right):

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.cc:133: m_backings.sort(compareBackings);
cool, I didn't know std::list had a O(n log(n)) sort. Any idea how it works?

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.cc:353: m_backings.push_front(backing);
On 2012/10/17 01:10:52, ccameron1 wrote:
> There was some controversy about doing this "add to the front of the list,
then
> remove from the front of the list and put at the back of the list". The reason
> for this is that on some platforms we will want to be able to call
createBacking
> when not paired with tying it to a backing, in order to avoid driver overhead
of
> creating resources

Writing code now because we will need it later isn't a great way to do things -
let's write the code we need now now and write stuff we need later later. 
createBacking() is private so we only have to worry about local invariants here
and the only caller to createBacking() puts the return value in the list itself.

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.cc:357: void
CCPrioritizedTextureManager::evictFirstBackingResource(CCPrioritizedTexture::Backing*
backing, CCResourceProvider* resourceProvider)
why does this take a backing* if it's only operating on the first in the list? 
I would just leave the parameter out

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
File cc/prioritized_texture_manager.h (right):

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.h:161: BackingVector m_evictedBackings;
Does the m_evictedBackings need O(1) access or could it be the same type as
m_backings?  The access patterns seem pretty similar

[ccameron, 2012-10-17 02:13:58]

Thanks!

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
File cc/prioritized_texture_manager.cc (left):

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.cc:131: BackingVector& sortedBackings =
m_tempBackingVector;
On 2012/10/17 01:31:11, jamesr wrote:
> can we get rid of the m_tempBackingVector member completely now?

Yeah, it's gone now.

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
File cc/prioritized_texture_manager.cc (right):

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.cc:133: m_backings.sort(compareBackings);
On 2012/10/17 01:31:11, jamesr wrote:
> cool, I didn't know std::list had a O(n log(n)) sort. Any idea how it works?

I didn't know it before today. I'd guess it uses std::list::merge().

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.cc:353: m_backings.push_front(backing);
On 2012/10/17 01:31:11, jamesr wrote:
> On 2012/10/17 01:10:52, ccameron1 wrote:
> > There was some controversy about doing this "add to the front of the list,
> then
> > remove from the front of the list and put at the back of the list". The
reason
> > for this is that on some platforms we will want to be able to call
> createBacking
> > when not paired with tying it to a backing, in order to avoid driver
overhead
> of
> > creating resources
> 
> Writing code now because we will need it later isn't a great way to do things
-
> let's write the code we need now now and write stuff we need later later. 
> createBacking() is private so we only have to worry about local invariants
here
> and the only caller to createBacking() puts the return value in the list
itself.
> 

Removed.

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.cc:357: void
CCPrioritizedTextureManager::evictFirstBackingResource(CCPrioritizedTexture::Backing*
backing, CCResourceProvider* resourceProvider)
On 2012/10/17 01:31:11, jamesr wrote:
> why does this take a backing* if it's only operating on the first in the list?

> I would just leave the parameter out

Yeah, this just made me feel warm and fuzzy.  Deleted.

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
File cc/prioritized_texture_manager.h (right):

https://codereview.chromium.org/11195015/diff/2001/cc/prioritized_texture_man...
cc/prioritized_texture_manager.h:161: BackingVector m_evictedBackings;
On 2012/10/17 01:31:11, jamesr wrote:
> Does the m_evictedBackings need O(1) access or could it be the same type as
> m_backings?  The access patterns seem pretty similar

Nah, it can be a list too.  Updated.

[jamesr, 2012-10-17 02:20:59]

lgtm

https://codereview.chromium.org/11195015/diff/6002/cc/prioritized_texture_man...
File cc/prioritized_texture_manager.cc (right):

https://codereview.chromium.org/11195015/diff/6002/cc/prioritized_texture_man...
cc/prioritized_texture_manager.cc:268: sortBackings();
This will be unnecessary a lot of the time, won't it?  Would it be worth keeping
an m_sorted bit around to avoid unnecessary sorts?

[ccameron, 2012-10-17 04:14:14]

On 2012/10/17 02:20:59, jamesr wrote:
> lgtm
> 
>
https://codereview.chromium.org/11195015/diff/6002/cc/prioritized_texture_man...
> File cc/prioritized_texture_manager.cc (right):
> 
>
https://codereview.chromium.org/11195015/diff/6002/cc/prioritized_texture_man...
> cc/prioritized_texture_manager.cc:268: sortBackings();
> This will be unnecessary a lot of the time, won't it?  Would it be worth
keeping
> an m_sorted bit around to avoid unnecessary sorts?

Added a m_backingsTailNotSorted (I didn't want to say "it's not sorted" because
most of the list is -- just not the tail).

[jamesr, 2012-10-17 04:37:09]

Not sure if the distinction will matter much in the end, but sure. Still lgtm

[commit-bot: I haz the power, 2012-10-17 06:40:59]

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

[commit-bot: I haz the power, 2012-10-17 09:18:54]

Change committed as 162335