Identify evictable textures by GL texture parameter

Make textures that are rendered through the prioritized resource
manager identify themselves as managed to the GPU memory manager
through a GL texture parameter.

This allows the GPU memory manager to get very precise statistics
on evictable memory usage with minimal overhead.

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

[piman, 2012-12-13 02:31:55]

https://codereview.chromium.org/11548052/diff/1/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/11548052/diff/1/cc/resource_provider.cc#newco...
cc/resource_provider.cc:181: GLC(context3d,
context3d->texParameteri(GL_TEXTURE_2D, GL_TEXTURE_POOL_CHROMIUM,
GL_TEXTURE_POOL_MANAGED_CHROMIUM));
Layering violation - the resource provider shouldn't hard-code anything about
the semantics associated with each pool

Can you pass this as a parameter instead?

[ccameron, 2012-12-13 02:36:01]

On 2012/12/13 02:31:55, piman wrote:
> https://codereview.chromium.org/11548052/diff/1/cc/resource_provider.cc
> File cc/resource_provider.cc (right):
> 
>
https://codereview.chromium.org/11548052/diff/1/cc/resource_provider.cc#newco...
> cc/resource_provider.cc:181: GLC(context3d,
> context3d->texParameteri(GL_TEXTURE_2D, GL_TEXTURE_POOL_CHROMIUM,
> GL_TEXTURE_POOL_MANAGED_CHROMIUM));
> Layering violation - the resource provider shouldn't hard-code anything about
> the semantics associated with each pool
> 
> Can you pass this as a parameter instead?

This is already passed as a parameter to allocation (namely as the pool, which
the switch is done on). According to the resource provider, the pool is an int,
but its values always come form the renderer as

    // This enum defines the various resource pools for the ResourceProvider
    // where textures get allocated.
    enum ResourcePool {
      ImplPool = 1, // This pool is for textures that get allocated on the impl
thread (e.g. RenderSurfaces).
      ContentPool // This pool is for textures that get allocated on the main
thread (e.g. tiles).
    };

Another fix for this would be to move ResourcePool into ResourceProvider, and
then search-replace Renderer::ImplPool/ContentsPool with
ResourceProvider::ImplPool/ContentsPool.

I'll do that instead (it'll touch a dozen files, but that's fine).

[piman, 2012-12-13 02:37:40]

On Wed, Dec 12, 2012 at 6:36 PM, <ccameron@chromium.org> wrote:

> On 2012/12/13 02:31:55, piman wrote:
>
>> https://codereview.chromium.**org/11548052/diff/1/cc/**
>>
resource_provider.cc<https://codereview.chromium.org/11548052/diff/1/cc/resource_provider.cc>
>> File cc/resource_provider.cc (right):
>>
>
>
> https://codereview.chromium.**org/11548052/diff/1/cc/**
>
resource_provider.cc#**newcode181<https://codereview.chromium.org/11548052/diff/1/cc/resource_provider.cc#newcode181>
>
>> cc/resource_provider.cc:181: GLC(context3d,
>> context3d->texParameteri(GL_**TEXTURE_2D, GL_TEXTURE_POOL_CHROMIUM,
>> GL_TEXTURE_POOL_MANAGED_**CHROMIUM));
>> Layering violation - the resource provider shouldn't hard-code anything
>> about
>> the semantics associated with each pool
>>
>
>  Can you pass this as a parameter instead?
>>
>
> This is already passed as a parameter to allocation (namely as the pool,
> which
> the switch is done on). According to the resource provider, the pool is an
> int,
> but its values always come form the renderer as
>
>     // This enum defines the various resource pools for the
> ResourceProvider
>     // where textures get allocated.
>     enum ResourcePool {
>       ImplPool = 1, // This pool is for textures that get allocated on the
> impl
> thread (e.g. RenderSurfaces).
>       ContentPool // This pool is for textures that get allocated on the
> main
> thread (e.g. tiles).
>     };
>
> Another fix for this would be to move ResourcePool into ResourceProvider,
> and
> then search-replace Renderer::ImplPool/**ContentsPool with
> ResourceProvider::ImplPool/**ContentsPool.
>
> I'll do that instead (it'll touch a dozen files, but that's fine).
>

Please don't.


>
>
https://codereview.chromium.**org/11548052/<https://codereview.chromium.org/1...
>

[piman, 2012-12-13 02:41:16]

On Wed, Dec 12, 2012 at 6:37 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Wed, Dec 12, 2012 at 6:36 PM, <ccameron@chromium.org> wrote:
>
>> On 2012/12/13 02:31:55, piman wrote:
>>
>>> https://codereview.chromium.**org/11548052/diff/1/cc/**
>>>
resource_provider.cc<https://codereview.chromium.org/11548052/diff/1/cc/resource_provider.cc>
>>> File cc/resource_provider.cc (right):
>>>
>>
>>
>> https://codereview.chromium.**org/11548052/diff/1/cc/**
>>
resource_provider.cc#**newcode181<https://codereview.chromium.org/11548052/diff/1/cc/resource_provider.cc#newcode181>
>>
>>> cc/resource_provider.cc:181: GLC(context3d,
>>> context3d->texParameteri(GL_**TEXTURE_2D, GL_TEXTURE_POOL_CHROMIUM,
>>> GL_TEXTURE_POOL_MANAGED_**CHROMIUM));
>>> Layering violation - the resource provider shouldn't hard-code anything
>>> about
>>> the semantics associated with each pool
>>>
>>
>>  Can you pass this as a parameter instead?
>>>
>>
>> This is already passed as a parameter to allocation (namely as the pool,
>> which
>> the switch is done on). According to the resource provider, the pool is
>> an int,
>> but its values always come form the renderer as
>>
>>     // This enum defines the various resource pools for the
>> ResourceProvider
>>     // where textures get allocated.
>>     enum ResourcePool {
>>       ImplPool = 1, // This pool is for textures that get allocated on
>> the impl
>> thread (e.g. RenderSurfaces).
>>       ContentPool // This pool is for textures that get allocated on the
>> main
>> thread (e.g. tiles).
>>     };
>>
>> Another fix for this would be to move ResourcePool into ResourceProvider,
>> and
>> then search-replace Renderer::ImplPool/**ContentsPool with
>> ResourceProvider::ImplPool/**ContentsPool.
>>
>> I'll do that instead (it'll touch a dozen files, but that's fine).
>>
>
> Please don't.
>

There are more pools than just Impl vs Content, and the fact that they are
in Renderer is a historical artifact, and not where it belongs.
The ResourceProvider should not attach hard-coded semantics to the pool.
If you want, you could have the clients attach those semantics by adding a
"setPoolIsManaged" or something, and then keep the association in a map or
sth.

Antoine


>
>
>>
>>
https://codereview.chromium.**org/11548052/<https://codereview.chromium.org/1...
>>
>
>

[piman, 2012-12-13 02:43:47]

On Wed, Dec 12, 2012 at 6:40 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Wed, Dec 12, 2012 at 6:37 PM, Antoine Labour <piman@chromium.org>wrote:
>
>>
>>
>>  On Wed, Dec 12, 2012 at 6:36 PM, <ccameron@chromium.org> wrote:
>>
>>> On 2012/12/13 02:31:55, piman wrote:
>>>
>>>> https://codereview.chromium.**org/11548052/diff/1/cc/**
>>>>
resource_provider.cc<https://codereview.chromium.org/11548052/diff/1/cc/resource_provider.cc>
>>>> File cc/resource_provider.cc (right):
>>>>
>>>
>>>
>>> https://codereview.chromium.**org/11548052/diff/1/cc/**
>>>
resource_provider.cc#**newcode181<https://codereview.chromium.org/11548052/diff/1/cc/resource_provider.cc#newcode181>
>>>
>>>> cc/resource_provider.cc:181: GLC(context3d,
>>>> context3d->texParameteri(GL_**TEXTURE_2D, GL_TEXTURE_POOL_CHROMIUM,
>>>> GL_TEXTURE_POOL_MANAGED_**CHROMIUM));
>>>> Layering violation - the resource provider shouldn't hard-code anything
>>>> about
>>>> the semantics associated with each pool
>>>>
>>>
>>>  Can you pass this as a parameter instead?
>>>>
>>>
>>> This is already passed as a parameter to allocation (namely as the pool,
>>> which
>>> the switch is done on). According to the resource provider, the pool is
>>> an int,
>>> but its values always come form the renderer as
>>>
>>>     // This enum defines the various resource pools for the
>>> ResourceProvider
>>>     // where textures get allocated.
>>>     enum ResourcePool {
>>>       ImplPool = 1, // This pool is for textures that get allocated on
>>> the impl
>>> thread (e.g. RenderSurfaces).
>>>       ContentPool // This pool is for textures that get allocated on the
>>> main
>>> thread (e.g. tiles).
>>>     };
>>>
>>> Another fix for this would be to move ResourcePool into
>>> ResourceProvider, and
>>> then search-replace Renderer::ImplPool/**ContentsPool with
>>> ResourceProvider::ImplPool/**ContentsPool.
>>>
>>> I'll do that instead (it'll touch a dozen files, but that's fine).
>>>
>>
>> Please don't.
>>
>
> There are more pools than just Impl vs Content, and the fact that they are
> in Renderer is a historical artifact, and not where it belongs.
>

I should add that the explicit enums will likely disappear as we will need
to allocate the pool ids (for ubercomp clients).


>  The ResourceProvider should not attach hard-coded semantics to the pool.
> If you want, you could have the clients attach those semantics by adding a
> "setPoolIsManaged" or something, and then keep the association in a map or
> sth.
>

> Antoine
>
>
>>
>>
>>>
>>>
https://codereview.chromium.**org/11548052/<https://codereview.chromium.org/1...
>>>
>>
>>
>

[ccameron, 2012-12-13 09:20:14]

On 2012/12/13 02:41:16, piman wrote:
> On Wed, Dec 12, 2012 at 6:37 PM, Antoine Labour <mailto:piman@chromium.org>
wrote:
> 
> There are more pools than just Impl vs Content

Are you sure this is true?  There are a couple places where the value is set to
0 (accidentally, I'm guessing), but the only non-zero values that I saw outside
of unit tests were Impl and Content.

Is it instead the case that at some point in the future we may wish to
dynamically create pools, but we don't now?

> If you want, you could have the clients attach those semantics by adding a
> "setPoolIsManaged" or something

Whether or not a pool's textures have this flag should be set at the time that
the pool is created. This should not be a mutable property of pools (you then
have to go change all resources in that pool whenever the property is changed,
etc, when there is no need to have that behavior). Adding a setPoolIsManaged
function (which should not be added) and then calling
setPoolIsManaged(Renderer::ContentsPool) somewhere is adding quite a bit of
complexity and moving in the wrong direction.

The proposed patch is significantly less bad than that.

The other option I see is to create dynamic resource pools and get rid of the
hardcoded IDs before submitting this patch.

[piman, 2012-12-13 17:47:01]

On Thu, Dec 13, 2012 at 1:20 AM, <ccameron@chromium.org> wrote:

> On 2012/12/13 02:41:16, piman wrote:
>
>> On Wed, Dec 12, 2012 at 6:37 PM, Antoine Labour <mailto:
>> piman@chromium.org>
>>
> wrote:
>
>
>  There are more pools than just Impl vs Content
>>
>
> Are you sure this is true?  There are a couple places where the value is
> set to
> 0 (accidentally, I'm guessing), but the only non-zero values that I saw
> outside
> of unit tests were Impl and Content.
>

Yes, it's work in progress, it hasn't landed yet. But it's part of the
design.


>
> Is it instead the case that at some point in the future we may wish to
> dynamically create pools, but we don't now?
>
>
>  If you want, you could have the clients attach those semantics by adding a
>> "setPoolIsManaged" or something
>>
>
> Whether or not a pool's textures have this flag should be set at the time
> that
> the pool is created.


Sure


> This should not be a mutable property of pools (you then
> have to go change all resources in that pool whenever the property is
> changed,
> etc, when there is no need to have that behavior). Adding a
> setPoolIsManaged
> function (which should not be added) and then calling
> setPoolIsManaged(Renderer::**ContentsPool) somewhere is adding quite a
> bit of
> complexity and moving in the wrong direction.
>

Why? You'd call it at initialization time, you could add a DCHECK that
verifies that there's no resource from that pool in the RP.
An alternative is to pass an extra parameter as I suggested earlier.


> The proposed patch is significantly less bad than that.
>

It breaks layering. We're working towards avoiding these layering problems,
which add complexity, make reviews more complicated, and generally slow us
down.

Optimizing for the smallest change is not a good criterion.

The other option I see is to create dynamic resource pools and get rid of
> the
> hardcoded IDs before submitting this patch.
>

I don't think it is necessary, but that's fine for me if you want to do
that.

>
>
>
https://codereview.chromium.**org/11548052/<https://codereview.chromium.org/1...
>

[ccameron, 2012-12-13 18:48:20]

It looks like there are plans for this class that aren't evident from its
current form, and its current form suggests something different.

I'll drop by so I can get a clearer picture of what was intended by pools.

[ccameron, 2012-12-14 00:16:02]

We decided to remove the idea of pools from the ResourceProvider because their
use was to be for deleteOwnedResources, which we moved away from with the
PrioritizedResourceManager in favor of having the owner of the resources remove
them.

I've updated this patch to add a createManagedResource function, instead of
adding an argument to createResource. The two (argument versus function) are
relatively equal to me. I was slightly pushed in the direction of creating a
function because the vast majority of the callers (all except 1) of
createResource specify the same argument. But I'm completely open to changing to
having an argument to createResource.

[piman, 2012-12-14 02:02:10]

lgtm

[ccameron, 2012-12-14 21:39:39]

Thanks!!

(off to battle the trybots again)

[commit-bot: I haz the power, 2012-12-14 21:41:39]

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

[commit-bot: I haz the power, 2012-12-15 00:53:48]

Retried try job too often on win_rel for step(s) browser_tests