cc: Partial tile update for one-copy raster.

cc: Partial tile update for one-copy raster.

When a tile on the active tree is invalidated, we record the
old tile id with the raster task to playback for the new resource.
If possible, we find the resource that was used for the old tile
in the staging buffers pool, and reuse it, saving us from having
to raster the full tile. Instead we only have to raster the part
that has changed.

This takes a single tab spinner in the browser ui from .30ms per
frame to .08ms per frame, for a 73% reduction in raster time.

R=enne, piman@chromium.org, reveman, vmpstr
BUG=487387
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e0b5478e9fd582c904d1968726c437fd365ab3ea
Cr-Commit-Position: refs/heads/master@{#332069}

[danakj, 2015-05-14 00:50:33]

Patchset #1 (id:1) has been deleted

[danakj, 2015-05-14 00:50:55]

Please be gentle :3 I hope it's not crazy, but it does work.

[vmpstr, 2015-05-14 17:58:49]

Just some partial thoughts.

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/one_copy_t...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/one_copy_t...
cc/resources/one_copy_tile_task_worker_pool.cc:365:
previous_frame_read_lock->GetGpuMemoryBuffer();
This allocates one if it doesn't have one. Do we guarantee that it does exist?

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:269: std::vector<InvalidTiles>
new_tile_keys;
If you make this a (hash)map keyed by TileMapKey, then I think the logic below
is a bit simpler... ie

gfx::Rect& inv = new_tile_keys[iter.index()];
inv.Union(content_rect);

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/tile_task_...
File cc/resources/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/tile_task_...
cc/resources/tile_task_worker_pool.cc:197: canvas->translate(raster_offset.x(),
raster_offset.y());
It would honestly make more sense to me if we passed a gfx::Rect as in
"canvas_rect" and another gfx::Rect as in "raster_this_rect" and do the
translate inside PlaybackToCanvas... In the !partial_update case, canvas_rect ==
raster_this_rect, so it would be cool if that could be handled naturally

Wdyt?

[piman, 2015-05-14 19:50:47]

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
File cc/resources/display_list_raster_source.cc (right):

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
cc/resources/display_list_raster_source.cc:132: if
(!display_list_rect.IsEmpty())
nit: {}

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/one_copy_t...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/one_copy_t...
cc/resources/one_copy_tile_task_worker_pool.cc:375: memcpy(data, previous_data,
bytes);
Should we copy everything but the damage rect? If the damage rect is large we'd
be touching everything twice.

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:70: for (auto it = unused_resources_.begin(); it
!= unused_resources_.end();
Should we check busy_resources_ too? We only need the resource for reading, not
writing.

Or more accurately, if the resource is available for writing, then we can use it
directly, only rasterizing the damage rect, without even needing to copy.
Otherwise, if it's available, but only for reading, then we can copy from it
into a new resource.

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:71: ++it) {
should unused_resources_ be a hash map indexed by id?

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:74: pr.resource->size() != size)
If id match but format/size are different, can we just early exit?
Actually should we release that old resource altogether too? Things got resized,
doesn't sound like we would typically reuse it.

And if we did left it in, then we might end up with 2 (or more) resources with
the same id, one older than the other, and the logic here could end up giving
you a resource that doesn't match the damage rect you think you need (e.g.
resize from size A to B back to A).

(I don't understand how tile ids work though, so what I'm saying possibly
doesn't make sense)

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:153:
unused_resources_.push_back(PoolResource(resource, id));
Do we need to check for duplicates here?

[danakj, 2015-05-14 19:59:48]

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/one_copy_t...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/one_copy_t...
cc/resources/one_copy_tile_task_worker_pool.cc:365:
previous_frame_read_lock->GetGpuMemoryBuffer();
On 2015/05/14 17:58:49, vmpstr wrote:
> This allocates one if it doesn't have one. Do we guarantee that it does exist?

This came out of the unused_resources_ in the pool, since it was returned by
ResourcePool::TryAcquireOldResource. So I think that means we are in fact
guaranteed to not create a new one. At least for some GpuMemoryBuffer
implementations. I need to make TryAcquireOldResource return null or something
if it won't be true. I need some input from reveman@ about how to check if it
will be true.

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:269: std::vector<InvalidTiles>
new_tile_keys;
On 2015/05/14 17:58:49, vmpstr wrote:
> If you make this a (hash)map keyed by TileMapKey, then I think the logic below
> is a bit simpler... ie
> 
> gfx::Rect& inv = new_tile_keys[iter.index()];
> inv.Union(content_rect);

enne@ just pointed out to me that I can just union on the Tile directly and
don't need to do extra vector walks here with temporary rects.

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/one_copy_t...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/one_copy_t...
cc/resources/one_copy_tile_task_worker_pool.cc:375: memcpy(data, previous_data,
bytes);
On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> Should we copy everything but the damage rect? If the damage rect is large
we'd
> be touching everything twice.

Ya... I debated.. but then I have to write some more complicated and less
performant copying code. Maybe I need a branch for memcpy still.

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:70: for (auto it = unused_resources_.begin(); it
!= unused_resources_.end();
On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> Should we check busy_resources_ too? We only need the resource for reading,
not
> writing.
> 
> Or more accurately, if the resource is available for writing, then we can use
it
> directly, only rasterizing the damage rect, without even needing to copy.
> Otherwise, if it's available, but only for reading, then we can copy from it
> into a new resource.

Oh that's a good point, we don't need to copy if it's in unused at all.

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:71: ++it) {
On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> should unused_resources_ be a hash map indexed by id?

i could also make it a base::SmallMap, i'm not actually sure how big it gets in
the ui compositor.. i'll check.

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:74: pr.resource->size() != size)
On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> If id match but format/size are different, can we just early exit?
> Actually should we release that old resource altogether too? Things got
resized,
> doesn't sound like we would typically reuse it.

We can reuse it rather than mallocing a new one in AcquireResource() even if we
can't reuse the pixel data inside it.

> And if we did left it in, then we might end up with 2 (or more) resources with
> the same id, one older than the other, and the logic here could end up giving
> you a resource that doesn't match the damage rect you think you need (e.g.
> resize from size A to B back to A).

The id is unique, every invalidation generates a new Tile which is a new id.

> (I don't understand how tile ids work though, so what I'm saying possibly
> doesn't make sense)

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:153:
unused_resources_.push_back(PoolResource(resource, id));
On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> Do we need to check for duplicates here?

I don't think so, id is unique.

[vmpstr, 2015-05-14 20:08:34]

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:269: std::vector<InvalidTiles>
new_tile_keys;
On 2015/05/14 19:59:47, danakj wrote:
> On 2015/05/14 17:58:49, vmpstr wrote:
> > If you make this a (hash)map keyed by TileMapKey, then I think the logic
below
> > is a bit simpler... ie
> > 
> > gfx::Rect& inv = new_tile_keys[iter.index()];
> > inv.Union(content_rect);
> 
> enne@ just pointed out to me that I can just union on the Tile directly and
> don't need to do extra vector walks here with temporary rects.

You'd still need to record which tiles were invalidated though, so I guess
you'll also keep the original vector then? Temp map of key to rect doesn't sound
too bad to me, but whatever makes the code simpler is good with me.

[danakj, 2015-05-14 20:10:18]

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:269: std::vector<InvalidTiles>
new_tile_keys;
On 2015/05/14 20:08:34, vmpstr wrote:
> On 2015/05/14 19:59:47, danakj wrote:
> > On 2015/05/14 17:58:49, vmpstr wrote:
> > > If you make this a (hash)map keyed by TileMapKey, then I think the logic
> below
> > > is a bit simpler... ie
> > > 
> > > gfx::Rect& inv = new_tile_keys[iter.index()];
> > > inv.Union(content_rect);
> > 
> > enne@ just pointed out to me that I can just union on the Tile directly and
> > don't need to do extra vector walks here with temporary rects.
> 
> You'd still need to record which tiles were invalidated though, so I guess
> you'll also keep the original vector then? Temp map of key to rect doesn't
sound
> too bad to me, but whatever makes the code simpler is good with me.

The original vector of keys, yah. I have this change in the last patchset, I
think it's pretty clean.

[danakj, 2015-05-14 21:19:05]

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/tile_task_...
File cc/resources/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/20001/cc/resources/tile_task_...
cc/resources/tile_task_worker_pool.cc:197: canvas->translate(raster_offset.x(),
raster_offset.y());
On 2015/05/14 17:58:49, vmpstr wrote:
> It would honestly make more sense to me if we passed a gfx::Rect as in
> "canvas_rect" and another gfx::Rect as in "raster_this_rect" and do the
> translate inside PlaybackToCanvas... In the !partial_update case, canvas_rect
==
> raster_this_rect, so it would be cool if that could be handled naturally
> 
> Wdyt?

Done.

[danakj, 2015-05-14 22:03:42]

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
File cc/resources/display_list_raster_source.cc (right):

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
cc/resources/display_list_raster_source.cc:132: if
(!display_list_rect.IsEmpty())
On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> nit: {}

Undone.

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:70: for (auto it = unused_resources_.begin(); it
!= unused_resources_.end();
On 2015/05/14 19:59:47, danakj wrote:
> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> > Should we check busy_resources_ too? We only need the resource for reading,
> not
> > writing.
> > 
> > Or more accurately, if the resource is available for writing, then we can
use
> it
> > directly, only rasterizing the damage rect, without even needing to copy.
> > Otherwise, if it's available, but only for reading, then we can copy from it
> > into a new resource.
> 
> Oh that's a good point, we don't need to copy if it's in unused at all.

This was awesome. For on-screen animations we don't need to touch
busy_resources_ so this takes the raster time from .11 to under .08ms for a
spinner now.

On chromeos if you do it just write and drag a window with a spinner to push it
off the screen, you can occasionally activate while the resource is still
uploading and then the next frame sees a busy resource. So maybe it's good to
have that too. But 99.9% (100% on desktop) of the time we hit the non-busy
resources so, win.

https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:71: ++it) {
On 2015/05/14 19:59:47, danakj wrote:
> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> > should unused_resources_ be a hash map indexed by id?
> 
> i could also make it a base::SmallMap, i'm not actually sure how big it gets
in
> the ui compositor.. i'll check.

There are at most 32 things in the vector.
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

My intuition is that doesn't warrent a hash_map. WDYT?

[danakj, 2015-05-14 22:25:04]

Updated with reusing buffers better as per piman's idea.

[vmpstr, 2015-05-14 22:28:27]

https://codereview.chromium.org/1139063002/diff/120001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/120001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:295: RemoveTileData& remove_tile_data =
remove_tiles[iter.index()];
At a first glance, this seems to be changing behaviour:

- We should only be recreating tiles if recreate_tiles is true.
- We should only be creating tiles in regions where we removed tiles.

^ those two are no longer true. 

I'd prefer to keep old behaviour. I think you had a right thing before where you
moved RemoveTileAt to be in the second loop. The first loop just need to
accumulate rects (so the small map can be Key -> Rect, I think). The second loop
would do something like

if (RemoveTile(...) && recreate_tile) {
  CreateTile(...);
  tile->set_stuff(...);
}

[danakj, 2015-05-14 22:35:00]

https://codereview.chromium.org/1139063002/diff/120001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/120001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:295: RemoveTileData& remove_tile_data =
remove_tiles[iter.index()];
On 2015/05/14 22:28:27, vmpstr wrote:
> At a first glance, this seems to be changing behaviour:
> 
> - We should only be recreating tiles if recreate_tiles is true.
> - We should only be creating tiles in regions where we removed tiles.
> 
> ^ those two are no longer true. 
> 
> I'd prefer to keep old behaviour. I think you had a right thing before where
you
> moved RemoveTileAt to be in the second loop. The first loop just need to
> accumulate rects (so the small map can be Key -> Rect, I think). The second
loop
> would do something like
> 
> if (RemoveTile(...) && recreate_tile) {
>   CreateTile(...);
>   tile->set_stuff(...);
> }

Ya you're very right and your suggestion is spot on. Fixed this.

[vmpstr, 2015-05-14 23:11:46]

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/one_copy_...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:379: bool prv =
previous_gpu_memory_buffer->Map(&previous_data);
nit: s/prv/success/ or something like that?

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:299: Tile::Id old_id;
paranoidnit: Tile::Id old_id = 0u;

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/resource_...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/resource_...
cc/resources/resource_pool.cc:73: if (pr.id != id)
If this id is going to be unique, (and I think it is?) can you do a find instead
and deal with the result?

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/resource_...
cc/resources/resource_pool.cc:97: if (pr.id != id)
Same here.

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/tile_mana...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/tile_mana...
cc/resources/tile_manager.cc:756:
resource_pool_->ReleaseResource(resource.Pass(), tile->id());
This should be 0, because it's solid color: we might have not rasterized
anything into the texture, so you shouldn't reuse it if it becomes invalidated

[piman, 2015-05-14 23:13:01]

On Thu, May 14, 2015 at 3:03 PM, <danakj@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
> File cc/resources/display_list_raster_source.cc (right):
>
>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
> cc/resources/display_list_raster_source.cc:132: if
> (!display_list_rect.IsEmpty())
> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
>
>> nit: {}
>>
>
> Undone.
>
>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> File cc/resources/resource_pool.cc (right):
>
>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> cc/resources/resource_pool.cc:70: for (auto it =
> unused_resources_.begin(); it != unused_resources_.end();
> On 2015/05/14 19:59:47, danakj wrote:
>
>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
>> > Should we check busy_resources_ too? We only need the resource for
>>
> reading,
>
>> not
>> > writing.
>> >
>> > Or more accurately, if the resource is available for writing, then
>>
> we can use
>
>> it
>> > directly, only rasterizing the damage rect, without even needing to
>>
> copy.
>
>> > Otherwise, if it's available, but only for reading, then we can copy
>>
> from it
>
>> > into a new resource.
>>
>
>  Oh that's a good point, we don't need to copy if it's in unused at
>>
> all.
>
> This was awesome. For on-screen animations we don't need to touch
> busy_resources_ so this takes the raster time from .11 to under .08ms
> for a spinner now.
>
> On chromeos if you do it just write and drag a window with a spinner to
> push it off the screen, you can occasionally activate while the resource
> is still uploading and then the next frame sees a busy resource. So
> maybe it's good to have that too. But 99.9% (100% on desktop) of the
> time we hit the non-busy resources so, win.
>
>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> cc/resources/resource_pool.cc:71: ++it) {
> On 2015/05/14 19:59:47, danakj wrote:
>
>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
>> > should unused_resources_ be a hash map indexed by id?
>>
>
>  i could also make it a base::SmallMap, i'm not actually sure how big
>>
> it gets in
>
>> the ui compositor.. i'll check.
>>
>
> There are at most 32 things in the vector.
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
>
> My intuition is that doesn't warrent a hash_map. WDYT?
>

A std::list is very pointer-chasing and cache-unfriendly.
If generally small, a SmallMap seems appropriate?


> https://codereview.chromium.org/1139063002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-14 23:16:11]

On Thu, May 14, 2015 at 4:12 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Thu, May 14, 2015 at 3:03 PM, <danakj@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
>> File cc/resources/display_list_raster_source.cc (right):
>>
>>
>>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
>> cc/resources/display_list_raster_source.cc:132: if
>> (!display_list_rect.IsEmpty())
>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
>>
>>> nit: {}
>>>
>>
>> Undone.
>>
>>
>>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
>> File cc/resources/resource_pool.cc (right):
>>
>>
>>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
>> cc/resources/resource_pool.cc:70: for (auto it =
>> unused_resources_.begin(); it != unused_resources_.end();
>> On 2015/05/14 19:59:47, danakj wrote:
>>
>>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
>>> > Should we check busy_resources_ too? We only need the resource for
>>>
>> reading,
>>
>>> not
>>> > writing.
>>> >
>>> > Or more accurately, if the resource is available for writing, then
>>>
>> we can use
>>
>>> it
>>> > directly, only rasterizing the damage rect, without even needing to
>>>
>> copy.
>>
>>> > Otherwise, if it's available, but only for reading, then we can copy
>>>
>> from it
>>
>>> > into a new resource.
>>>
>>
>>  Oh that's a good point, we don't need to copy if it's in unused at
>>>
>> all.
>>
>> This was awesome. For on-screen animations we don't need to touch
>> busy_resources_ so this takes the raster time from .11 to under .08ms
>> for a spinner now.
>>
>> On chromeos if you do it just write and drag a window with a spinner to
>> push it off the screen, you can occasionally activate while the resource
>> is still uploading and then the next frame sees a busy resource. So
>> maybe it's good to have that too. But 99.9% (100% on desktop) of the
>> time we hit the non-busy resources so, win.
>>
>>
>>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
>> cc/resources/resource_pool.cc:71: ++it) {
>> On 2015/05/14 19:59:47, danakj wrote:
>>
>>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
>>> > should unused_resources_ be a hash map indexed by id?
>>>
>>
>>  i could also make it a base::SmallMap, i'm not actually sure how big
>>>
>> it gets in
>>
>>> the ui compositor.. i'll check.
>>>
>>
>> There are at most 32 things in the vector.
>>
>>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
>>
>> My intuition is that doesn't warrent a hash_map. WDYT?
>>
>
> A std::list is very pointer-chasing and cache-unfriendly.
> If generally small, a SmallMap seems appropriate?
>

Oh, this is a list. Hm. A SmallMap would just be a vector for some small
number of elements that we have to define. Maybe I should just use a vector
here?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmpstr, 2015-05-14 23:18:46]

On 2015/05/14 23:16:11, danakj wrote:
> On Thu, May 14, 2015 at 4:12 PM, Antoine Labour <mailto:piman@chromium.org>
wrote:
> 
> >
> >
> > On Thu, May 14, 2015 at 3:03 PM, <mailto:danakj@chromium.org> wrote:
> >
> >>
> >>
> >>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
> >> File cc/resources/display_list_raster_source.cc (right):
> >>
> >>
> >>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
> >> cc/resources/display_list_raster_source.cc:132: if
> >> (!display_list_rect.IsEmpty())
> >> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> >>
> >>> nit: {}
> >>>
> >>
> >> Undone.
> >>
> >>
> >>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> >> File cc/resources/resource_pool.cc (right):
> >>
> >>
> >>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> >> cc/resources/resource_pool.cc:70: for (auto it =
> >> unused_resources_.begin(); it != unused_resources_.end();
> >> On 2015/05/14 19:59:47, danakj wrote:
> >>
> >>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> >>> > Should we check busy_resources_ too? We only need the resource for
> >>>
> >> reading,
> >>
> >>> not
> >>> > writing.
> >>> >
> >>> > Or more accurately, if the resource is available for writing, then
> >>>
> >> we can use
> >>
> >>> it
> >>> > directly, only rasterizing the damage rect, without even needing to
> >>>
> >> copy.
> >>
> >>> > Otherwise, if it's available, but only for reading, then we can copy
> >>>
> >> from it
> >>
> >>> > into a new resource.
> >>>
> >>
> >>  Oh that's a good point, we don't need to copy if it's in unused at
> >>>
> >> all.
> >>
> >> This was awesome. For on-screen animations we don't need to touch
> >> busy_resources_ so this takes the raster time from .11 to under .08ms
> >> for a spinner now.
> >>
> >> On chromeos if you do it just write and drag a window with a spinner to
> >> push it off the screen, you can occasionally activate while the resource
> >> is still uploading and then the next frame sees a busy resource. So
> >> maybe it's good to have that too. But 99.9% (100% on desktop) of the
> >> time we hit the non-busy resources so, win.
> >>
> >>
> >>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> >> cc/resources/resource_pool.cc:71: ++it) {
> >> On 2015/05/14 19:59:47, danakj wrote:
> >>
> >>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> >>> > should unused_resources_ be a hash map indexed by id?
> >>>
> >>
> >>  i could also make it a base::SmallMap, i'm not actually sure how big
> >>>
> >> it gets in
> >>
> >>> the ui compositor.. i'll check.
> >>>
> >>
> >> There are at most 32 things in the vector.
> >>
> >>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> >>
> >> My intuition is that doesn't warrent a hash_map. WDYT?
> >>
> >
> > A std::list is very pointer-chasing and cache-unfriendly.
> > If generally small, a SmallMap seems appropriate?
> >
> 
> Oh, this is a list. Hm. A SmallMap would just be a vector for some small
> number of elements that we have to define. Maybe I should just use a vector
> here?
> 

Also I meant id is unique as long as it's non zero, right? there can be multiple
0 id entries, as i understand it

> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-05-14 23:22:28]

On Thu, May 14, 2015 at 4:15 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, May 14, 2015 at 4:12 PM, Antoine Labour <piman@chromium.org>
> wrote:
>
>>
>>
>> On Thu, May 14, 2015 at 3:03 PM, <danakj@chromium.org> wrote:
>>
>>>
>>>
>>>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
>>> File cc/resources/display_list_raster_source.cc (right):
>>>
>>>
>>>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
>>> cc/resources/display_list_raster_source.cc:132: if
>>> (!display_list_rect.IsEmpty())
>>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
>>>
>>>> nit: {}
>>>>
>>>
>>> Undone.
>>>
>>>
>>>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
>>> File cc/resources/resource_pool.cc (right):
>>>
>>>
>>>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
>>> cc/resources/resource_pool.cc:70: for (auto it =
>>> unused_resources_.begin(); it != unused_resources_.end();
>>> On 2015/05/14 19:59:47, danakj wrote:
>>>
>>>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
>>>> > Should we check busy_resources_ too? We only need the resource for
>>>>
>>> reading,
>>>
>>>> not
>>>> > writing.
>>>> >
>>>> > Or more accurately, if the resource is available for writing, then
>>>>
>>> we can use
>>>
>>>> it
>>>> > directly, only rasterizing the damage rect, without even needing to
>>>>
>>> copy.
>>>
>>>> > Otherwise, if it's available, but only for reading, then we can copy
>>>>
>>> from it
>>>
>>>> > into a new resource.
>>>>
>>>
>>>  Oh that's a good point, we don't need to copy if it's in unused at
>>>>
>>> all.
>>>
>>> This was awesome. For on-screen animations we don't need to touch
>>> busy_resources_ so this takes the raster time from .11 to under .08ms
>>> for a spinner now.
>>>
>>> On chromeos if you do it just write and drag a window with a spinner to
>>> push it off the screen, you can occasionally activate while the resource
>>> is still uploading and then the next frame sees a busy resource. So
>>> maybe it's good to have that too. But 99.9% (100% on desktop) of the
>>> time we hit the non-busy resources so, win.
>>>
>>>
>>>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
>>> cc/resources/resource_pool.cc:71: ++it) {
>>> On 2015/05/14 19:59:47, danakj wrote:
>>>
>>>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
>>>> > should unused_resources_ be a hash map indexed by id?
>>>>
>>>
>>>  i could also make it a base::SmallMap, i'm not actually sure how big
>>>>
>>> it gets in
>>>
>>>> the ui compositor.. i'll check.
>>>>
>>>
>>> There are at most 32 things in the vector.
>>>
>>>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
>>>
>>> My intuition is that doesn't warrent a hash_map. WDYT?
>>>
>>
>> A std::list is very pointer-chasing and cache-unfriendly.
>> If generally small, a SmallMap seems appropriate?
>>
>
> Oh, this is a list. Hm. A SmallMap would just be a vector for some small
> number of elements that we have to define. Maybe I should just use a vector
> here?
>

True. Maybe a vector is fine...

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-14 23:22:42]

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/one_copy_...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:379: bool prv =
previous_gpu_memory_buffer->Map(&previous_data);
On 2015/05/14 23:11:46, vmpstr wrote:
> nit: s/prv/success/ or something like that?

Ya I'm not super excited about all the naming in here. Lemme make something more
rational. The src/dst stuff is not great imo.

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:299: Tile::Id old_id;
On 2015/05/14 23:11:46, vmpstr wrote:
> paranoidnit: Tile::Id old_id = 0u;

Done.

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/resource_...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/resource_...
cc/resources/resource_pool.cc:73: if (pr.id != id)
On 2015/05/14 23:11:46, vmpstr wrote:
> If this id is going to be unique, (and I think it is?) can you do a find
instead
> and deal with the result?

Ya good plan! Done.

https://codereview.chromium.org/1139063002/diff/180001/cc/resources/resource_...
cc/resources/resource_pool.cc:97: if (pr.id != id)
On 2015/05/14 23:11:46, vmpstr wrote:
> Same here.

Done.

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/tile_mana...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/tile_mana...
cc/resources/tile_manager.cc:756:
resource_pool_->ReleaseResource(resource.Pass(), tile->id());
On 2015/05/14 23:11:46, vmpstr wrote:
> This should be 0, because it's solid color: we might have not rasterized
> anything into the texture, so you shouldn't reuse it if it becomes invalidated

Thanks, added comment too.

[danakj, 2015-05-14 23:28:32]

On 2015/05/14 23:22:28, piman (Very slow to review) wrote:
> On Thu, May 14, 2015 at 4:15 PM, Dana Jansens <mailto:danakj@chromium.org>
wrote:
> 
> > On Thu, May 14, 2015 at 4:12 PM, Antoine Labour <mailto:piman@chromium.org>
> > wrote:
> >
> >>
> >>
> >> On Thu, May 14, 2015 at 3:03 PM, <mailto:danakj@chromium.org> wrote:
> >>
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
> >>> File cc/resources/display_list_raster_source.cc (right):
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
> >>> cc/resources/display_list_raster_source.cc:132: if
> >>> (!display_list_rect.IsEmpty())
> >>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> >>>
> >>>> nit: {}
> >>>>
> >>>
> >>> Undone.
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> >>> File cc/resources/resource_pool.cc (right):
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> >>> cc/resources/resource_pool.cc:70: for (auto it =
> >>> unused_resources_.begin(); it != unused_resources_.end();
> >>> On 2015/05/14 19:59:47, danakj wrote:
> >>>
> >>>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> >>>> > Should we check busy_resources_ too? We only need the resource for
> >>>>
> >>> reading,
> >>>
> >>>> not
> >>>> > writing.
> >>>> >
> >>>> > Or more accurately, if the resource is available for writing, then
> >>>>
> >>> we can use
> >>>
> >>>> it
> >>>> > directly, only rasterizing the damage rect, without even needing to
> >>>>
> >>> copy.
> >>>
> >>>> > Otherwise, if it's available, but only for reading, then we can copy
> >>>>
> >>> from it
> >>>
> >>>> > into a new resource.
> >>>>
> >>>
> >>>  Oh that's a good point, we don't need to copy if it's in unused at
> >>>>
> >>> all.
> >>>
> >>> This was awesome. For on-screen animations we don't need to touch
> >>> busy_resources_ so this takes the raster time from .11 to under .08ms
> >>> for a spinner now.
> >>>
> >>> On chromeos if you do it just write and drag a window with a spinner to
> >>> push it off the screen, you can occasionally activate while the resource
> >>> is still uploading and then the next frame sees a busy resource. So
> >>> maybe it's good to have that too. But 99.9% (100% on desktop) of the
> >>> time we hit the non-busy resources so, win.
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> >>> cc/resources/resource_pool.cc:71: ++it) {
> >>> On 2015/05/14 19:59:47, danakj wrote:
> >>>
> >>>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> >>>> > should unused_resources_ be a hash map indexed by id?
> >>>>
> >>>
> >>>  i could also make it a base::SmallMap, i'm not actually sure how big
> >>>>
> >>> it gets in
> >>>
> >>>> the ui compositor.. i'll check.
> >>>>
> >>>
> >>> There are at most 32 things in the vector.
> >>>
> >>>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> >>>
> >>> My intuition is that doesn't warrent a hash_map. WDYT?
> >>>
> >>
> >> A std::list is very pointer-chasing and cache-unfriendly.
> >> If generally small, a SmallMap seems appropriate?
> >>
> >
> > Oh, this is a list. Hm. A SmallMap would just be a vector for some small
> > number of elements that we have to define. Maybe I should just use a vector
> > here?
> >
> 
> True. Maybe a vector is fine...

I made it a vector. List is terrible apparently even for erase.
http://blog.davidecoppola.com/2014/05/cpp-benchmarks-vector-vs-list-vs-deque/

[danakj, 2015-05-14 23:32:10]

On 2015/05/14 23:28:32, danakj wrote:
> On 2015/05/14 23:22:28, piman (Very slow to review) wrote:
> > On Thu, May 14, 2015 at 4:15 PM, Dana Jansens <mailto:danakj@chromium.org>
> wrote:
> > 
> > > On Thu, May 14, 2015 at 4:12 PM, Antoine Labour
<mailto:piman@chromium.org>
> > > wrote:
> > >
> > >>
> > >>
> > >> On Thu, May 14, 2015 at 3:03 PM, <mailto:danakj@chromium.org> wrote:
> > >>
> > >>>
> > >>>
> > >>>
> >
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
> > >>> File cc/resources/display_list_raster_source.cc (right):
> > >>>
> > >>>
> > >>>
> >
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/display_li...
> > >>> cc/resources/display_list_raster_source.cc:132: if
> > >>> (!display_list_rect.IsEmpty())
> > >>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> > >>>
> > >>>> nit: {}
> > >>>>
> > >>>
> > >>> Undone.
> > >>>
> > >>>
> > >>>
> >
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> > >>> File cc/resources/resource_pool.cc (right):
> > >>>
> > >>>
> > >>>
> >
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> > >>> cc/resources/resource_pool.cc:70: for (auto it =
> > >>> unused_resources_.begin(); it != unused_resources_.end();
> > >>> On 2015/05/14 19:59:47, danakj wrote:
> > >>>
> > >>>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> > >>>> > Should we check busy_resources_ too? We only need the resource for
> > >>>>
> > >>> reading,
> > >>>
> > >>>> not
> > >>>> > writing.
> > >>>> >
> > >>>> > Or more accurately, if the resource is available for writing, then
> > >>>>
> > >>> we can use
> > >>>
> > >>>> it
> > >>>> > directly, only rasterizing the damage rect, without even needing to
> > >>>>
> > >>> copy.
> > >>>
> > >>>> > Otherwise, if it's available, but only for reading, then we can copy
> > >>>>
> > >>> from it
> > >>>
> > >>>> > into a new resource.
> > >>>>
> > >>>
> > >>>  Oh that's a good point, we don't need to copy if it's in unused at
> > >>>>
> > >>> all.
> > >>>
> > >>> This was awesome. For on-screen animations we don't need to touch
> > >>> busy_resources_ so this takes the raster time from .11 to under .08ms
> > >>> for a spinner now.
> > >>>
> > >>> On chromeos if you do it just write and drag a window with a spinner to
> > >>> push it off the screen, you can occasionally activate while the resource
> > >>> is still uploading and then the next frame sees a busy resource. So
> > >>> maybe it's good to have that too. But 99.9% (100% on desktop) of the
> > >>> time we hit the non-busy resources so, win.
> > >>>
> > >>>
> > >>>
> >
>
https://codereview.chromium.org/1139063002/diff/60001/cc/resources/resource_p...
> > >>> cc/resources/resource_pool.cc:71: ++it) {
> > >>> On 2015/05/14 19:59:47, danakj wrote:
> > >>>
> > >>>> On 2015/05/14 19:50:46, piman (Very slow to review) wrote:
> > >>>> > should unused_resources_ be a hash map indexed by id?
> > >>>>
> > >>>
> > >>>  i could also make it a base::SmallMap, i'm not actually sure how big
> > >>>>
> > >>> it gets in
> > >>>
> > >>>> the ui compositor.. i'll check.
> > >>>>
> > >>>
> > >>> There are at most 32 things in the vector.
> > >>>
> > >>>
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> > >>>
> > >>> My intuition is that doesn't warrent a hash_map. WDYT?
> > >>>
> > >>
> > >> A std::list is very pointer-chasing and cache-unfriendly.
> > >> If generally small, a SmallMap seems appropriate?
> > >>
> > >
> > > Oh, this is a list. Hm. A SmallMap would just be a vector for some small
> > > number of elements that we have to define. Maybe I should just use a
vector
> > > here?
> > >
> > 
> > True. Maybe a vector is fine...
> 
> I made it a vector. List is terrible apparently even for erase.
> http://blog.davidecoppola.com/2014/05/cpp-benchmarks-vector-vs-list-vs-deque/

Argh, I missed that we pop_front. deque instead ftw.

[reveman, 2015-05-14 23:52:20]

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:52: previous_frame_lock_.reset(
We need to somehow explicitly allocated a GpuMemoryBuffer that provides the
reuse guarantees that this requires. I think the best would be to just implement
our own shared memory backed GpuMemoryBuffer here. It would only work in the
browser process where we can allocated shared memory but I assume that's fine
for now.

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:97: scoped_ptr<ScopedResource>
previous_frame_raster_resource_;
nit: previous_raster_resource_ instead as I think we should avoid the use of
"frame" here. this code is not aware of the concept of frames

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:100: previous_frame_lock_;
nit: previous_lock_

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:384: if (previous_stride ==
stride) {
what if this is false?

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:385: size_t bytes =
static_cast<size_t>(stride) * src->size().height();
Two problems here:

1. gfx::GpuMemoryBuffer::GetStride() can return a negative stride.

2. It's bad idea in general to assume that stride is similar to "width * bpp".
(e.g. an implementation can use a large buffer to store a number of smaller
buffers, a texture atlas basically). If we copy stride amount of bytes, then we
might be copying much more than needed and also writing to memory that is used
by the GPU.

I guess we could make these assumptions here if we knew how the GpuMemoryBuffer
was allocated and that is after all a requirement for being able to assume that
a previous buffer can be reused. I would still recommend fixing these two
problems in case we want to extend the use to non-shmem buffers in the future.

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/resource_...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/resource_...
cc/resources/resource_pool.h:31: ResourceFormat format,
Why are we providing |size| and |format| when asking for a specific id?

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/resource_...
cc/resources/resource_pool.h:34: ResourceFormat format,
ditto

[danakj, 2015-05-14 23:56:34]

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:384: if (previous_stride ==
stride) {
On 2015/05/14 23:52:20, reveman wrote:
> what if this is false?

We just don't use the old resource, and we re-raster.

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:385: size_t bytes =
static_cast<size_t>(stride) * src->size().height();
On 2015/05/14 23:52:20, reveman wrote:
> Two problems here:
> 
> 1. gfx::GpuMemoryBuffer::GetStride() can return a negative stride.

What does a negative stride mean?
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/gpu_memory_...
makes no mention of this.

> 2. It's bad idea in general to assume that stride is similar to "width * bpp".
> (e.g. an implementation can use a large buffer to store a number of smaller
> buffers, a texture atlas basically). If we copy stride amount of bytes, then
we
> might be copying much more than needed and also writing to memory that is used
> by the GPU.
> 
> I guess we could make these assumptions here if we knew how the
GpuMemoryBuffer
> was allocated and that is after all a requirement for being able to assume
that
> a previous buffer can be reused. I would still recommend fixing these two
> problems in case we want to extend the use to non-shmem buffers in the future.

Oh ok, I'll do a memcpy per row then.

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/resource_...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/resource_...
cc/resources/resource_pool.h:31: ResourceFormat format,
On 2015/05/14 23:52:20, reveman wrote:
> Why are we providing |size| and |format| when asking for a specific id?

In case the new tile has a different size/format than the previous tile did.
This makes things more robust I think?

[reveman, 2015-05-15 00:02:57]

https://codereview.chromium.org/1139063002/diff/240001/cc/resources/resource_...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1139063002/diff/240001/cc/resources/resource_...
cc/resources/resource_pool.h:81: typedef std::deque<PoolResource> ResourceList;
It's currently a list as it's common that we remove elements in the middle of
the list. A deque might make that more expensive. While this is not going to be
very large in the staging resource pool case, there's not really a limit to this
pool in the general tile resource pool case. Except the GPU memory limit of
course..

HashingMRUCache maybe?

Either way, would be nice to see this land separately together with a small
perftest.

[danakj, 2015-05-15 00:06:06]

https://codereview.chromium.org/1139063002/diff/240001/cc/resources/resource_...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1139063002/diff/240001/cc/resources/resource_...
cc/resources/resource_pool.h:81: typedef std::deque<PoolResource> ResourceList;
On 2015/05/15 00:02:56, reveman wrote:
> It's currently a list as it's common that we remove elements in the middle of
> the list. A deque might make that more expensive. While this is not going to
be
> very large in the staging resource pool case, there's not really a limit to
this
> pool in the general tile resource pool case. Except the GPU memory limit of
> course..
> 
> HashingMRUCache maybe?
> 
> Either way, would be nice to see this land separately together with a small
> perftest.

Removing from the middle of a std::list is worse than removing from the middle
of a std::deque because the find part is terrible:
http://blog.davidecoppola.com/2014/05/cpp-benchmarks-vector-vs-list-vs-deque/

[danakj, 2015-05-15 00:09:59]

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:52: previous_frame_lock_.reset(
On 2015/05/14 23:52:20, reveman wrote:
> We need to somehow explicitly allocated a GpuMemoryBuffer that provides the
> reuse guarantees that this requires. I think the best would be to just
implement
> our own shared memory backed GpuMemoryBuffer here. It would only work in the
> browser process where we can allocated shared memory but I assume that's fine
> for now.

What's the plan to extend it to the renderer then? Can we just expose a promise
on the GpuMemoryBuffer API and only use this when that is true?

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:97: scoped_ptr<ScopedResource>
previous_frame_raster_resource_;
On 2015/05/14 23:52:20, reveman wrote:
> nit: previous_raster_resource_ instead as I think we should avoid the use of
> "frame" here. this code is not aware of the concept of frames

Done.

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:100: previous_frame_lock_;
On 2015/05/14 23:52:20, reveman wrote:
> nit: previous_lock_

Done.

[reveman, 2015-05-15 00:15:48]

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:385: size_t bytes =
static_cast<size_t>(stride) * src->size().height();
On 2015/05/14 23:56:34, danakj wrote:
> On 2015/05/14 23:52:20, reveman wrote:
> > Two problems here:
> > 
> > 1. gfx::GpuMemoryBuffer::GetStride() can return a negative stride.
> 
> What does a negative stride mean?
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/gpu_memory_...
> makes no mention of this.

No special meaning. If stride=-10 then start of row 4 for example is simply
located at -30 byte offset from start of row 1. It allows an implementation to
return buffers with a flipped Y axis. 

> 
> > 2. It's bad idea in general to assume that stride is similar to "width *
bpp".
> > (e.g. an implementation can use a large buffer to store a number of smaller
> > buffers, a texture atlas basically). If we copy stride amount of bytes, then
> we
> > might be copying much more than needed and also writing to memory that is
used
> > by the GPU.
> > 
> > I guess we could make these assumptions here if we knew how the
> GpuMemoryBuffer
> > was allocated and that is after all a requirement for being able to assume
> that
> > a previous buffer can be reused. I would still recommend fixing these two
> > problems in case we want to extend the use to non-shmem buffers in the
future.
> 
> Oh ok, I'll do a memcpy per row then.

[reveman, 2015-05-15 00:33:21]

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:52: previous_frame_lock_.reset(
On 2015/05/15 00:09:58, danakj wrote:
> On 2015/05/14 23:52:20, reveman wrote:
> > We need to somehow explicitly allocated a GpuMemoryBuffer that provides the
> > reuse guarantees that this requires. I think the best would be to just
> implement
> > our own shared memory backed GpuMemoryBuffer here. It would only work in the
> > browser process where we can allocated shared memory but I assume that's
fine
> > for now.
> 
> What's the plan to extend it to the renderer then? Can we just expose a
promise
> on the GpuMemoryBuffer API and only use this when that is true?

I'd like to have more data on performance and usage of different types of
buffers before we allow this in the general case. It might be easy to support in
a number of implementations but I'm not sure it's a good idea. It could result
in shadow copies of the data that make native buffers worse and have less
predictable performance than using shared memory.

[danakj, 2015-05-15 00:38:05]

On Thu, May 14, 2015 at 5:33 PM, <reveman@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
> File cc/resources/one_copy_tile_task_worker_pool.cc (right):
>
>
>
https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
> cc/resources/one_copy_tile_task_worker_pool.cc:52:
> previous_frame_lock_.reset(
> On 2015/05/15 00:09:58, danakj wrote:
>
>> On 2015/05/14 23:52:20, reveman wrote:
>> > We need to somehow explicitly allocated a GpuMemoryBuffer that
>>
> provides the
>
>> > reuse guarantees that this requires. I think the best would be to
>>
> just
>
>> implement
>> > our own shared memory backed GpuMemoryBuffer here. It would only
>>
> work in the
>
>> > browser process where we can allocated shared memory but I assume
>>
> that's fine
>
>> > for now.
>>
>
>  What's the plan to extend it to the renderer then? Can we just expose
>>
> a promise
>
>> on the GpuMemoryBuffer API and only use this when that is true?
>>
>
> I'd like to have more data on performance and usage of different types
> of buffers before we allow this in the general case. It might be easy to
> support in a number of implementations but I'm not sure it's a good
> idea. It could result in shadow copies of the data that make native
> buffers worse and have less predictable performance than using shared
> memory.
>

Yah, I agree let's turn it on narrowly. But can we do that by turning it on
narrowly via the GpuMemoryBuffer API? If we write some custom thing over
here then it's much harder to even try turn it on elsewhere, we have to
delete/rewrite it all.

And if it works on a platform for the browser, it should be okay to work
for the renderer too?


>
> https://codereview.chromium.org/1139063002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-15 00:40:09]

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
File cc/resources/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/190001/cc/resources/one_copy_...
cc/resources/one_copy_tile_task_worker_pool.cc:385: size_t bytes =
static_cast<size_t>(stride) * src->size().height();
On 2015/05/15 00:15:48, reveman wrote:
> On 2015/05/14 23:56:34, danakj wrote:
> > On 2015/05/14 23:52:20, reveman wrote:
> > > Two problems here:
> > > 
> > > 1. gfx::GpuMemoryBuffer::GetStride() can return a negative stride.
> > 
> > What does a negative stride mean?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/gpu_memory_...
> > makes no mention of this.
> 
> No special meaning. If stride=-10 then start of row 4 for example is simply
> located at -30 byte offset from start of row 1. It allows an implementation to
> return buffers with a flipped Y axis. 
> 
> > 
> > > 2. It's bad idea in general to assume that stride is similar to "width *
> bpp".
> > > (e.g. an implementation can use a large buffer to store a number of
smaller
> > > buffers, a texture atlas basically). If we copy stride amount of bytes,
then
> > we
> > > might be copying much more than needed and also writing to memory that is
> used
> > > by the GPU.
> > > 
> > > I guess we could make these assumptions here if we knew how the
> > GpuMemoryBuffer
> > > was allocated and that is after all a requirement for being able to assume
> > that
> > > a previous buffer can be reused. I would still recommend fixing these two
> > > problems in case we want to extend the use to non-shmem buffers in the
> future.
> > 
> > Oh ok, I'll do a memcpy per row then.
> 

Ah okay, thanks. Fixed it.

[danakj, 2015-05-15 00:55:00]

OK All existing unit tests pass now. I'd like to add some new ones next.

[danakj, 2015-05-15 03:19:34]

https://codereview.chromium.org/1139063002/diff/320001/cc/resources/resource_...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/320001/cc/resources/resource_...
cc/resources/resource_pool.cc:85: scoped_ptr<ScopedResource>
ResourcePool::TryAcquireOldBusyResource(
I was thinking about it and I think this reusing busy resources is too sketchy
and not worth it.

We're returning the resource in here as a scoped_ptr but it still should be in
the busy_resources list, cuz it is busy. Else we need to track its unbusy state
somehow when we return it. It seems fragile.

Also, it's only needed for a cornercase of an invalidation in the prepaint area.
For UI compositor there's basically no prepaint issue. And for animations in
prepaint, we try to avoid that in blink (disabling animated gifs etc).

I'm going to remove the busy resource part from this patch, which also removes
all the complicated copying code and having 2 resources in the OneCopy worker
pool code.

[piman, 2015-05-15 03:38:36]

https://codereview.chromium.org/1139063002/diff/320001/cc/resources/resource_...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/320001/cc/resources/resource_...
cc/resources/resource_pool.cc:85: scoped_ptr<ScopedResource>
ResourcePool::TryAcquireOldBusyResource(
On 2015/05/15 03:19:34, danakj wrote:
> I was thinking about it and I think this reusing busy resources is too sketchy
> and not worth it.
> 
> We're returning the resource in here as a scoped_ptr but it still should be in
> the busy_resources list, cuz it is busy. Else we need to track its unbusy
state
> somehow when we return it. It seems fragile.
> 
> Also, it's only needed for a cornercase of an invalidation in the prepaint
area.
> For UI compositor there's basically no prepaint issue. And for animations in
> prepaint, we try to avoid that in blink (disabling animated gifs etc).
> 
> I'm going to remove the busy resource part from this patch, which also removes
> all the complicated copying code and having 2 resources in the OneCopy worker
> pool code.

It sounds ok for UI, but for the renderer, aren't resources sent to the parent
always busy (i.e. TryAcquireOldResource will always fail)?
We can leave it out of this patch, but I think it'd be nice to hit later / as a
followup. E.g. do we have to return it as a scoped_ptr? We're only reading from
it. I guess we need to make sure it's not evicted nor written to if referenced
by a task for reading.

[danakj, 2015-05-15 16:52:58]

On 2015/05/15 03:38:36, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/1139063002/diff/320001/cc/resources/resource_...
> File cc/resources/resource_pool.cc (right):
> 
>
https://codereview.chromium.org/1139063002/diff/320001/cc/resources/resource_...
> cc/resources/resource_pool.cc:85: scoped_ptr<ScopedResource>
> ResourcePool::TryAcquireOldBusyResource(
> On 2015/05/15 03:19:34, danakj wrote:
> > I was thinking about it and I think this reusing busy resources is too
sketchy
> > and not worth it.
> > 
> > We're returning the resource in here as a scoped_ptr but it still should be
in
> > the busy_resources list, cuz it is busy. Else we need to track its unbusy
> state
> > somehow when we return it. It seems fragile.
> > 
> > Also, it's only needed for a cornercase of an invalidation in the prepaint
> area.
> > For UI compositor there's basically no prepaint issue. And for animations in
> > prepaint, we try to avoid that in blink (disabling animated gifs etc).
> > 
> > I'm going to remove the busy resource part from this patch, which also
removes
> > all the complicated copying code and having 2 resources in the OneCopy
worker
> > pool code.
> 
> It sounds ok for UI, but for the renderer, aren't resources sent to the parent
> always busy (i.e. TryAcquireOldResource will always fail)?

For one copy this isn't using the resources on the tiles, it's using the
resources used for uploading, so they have nothing to do with the ones sent to
the browser compositor.

For software compositing and gpu raster, we'll want to reuse resources in use on
the tiles themselves cuz there's no staging buffers to reuse, but those will
have different implementations.

> We can leave it out of this patch, but I think it'd be nice to hit later / as
a
> followup. E.g. do we have to return it as a scoped_ptr? We're only reading
from
> it. I guess we need to make sure it's not evicted nor written to if referenced
> by a task for reading.

[danakj, 2015-05-15 16:57:00]

Cool, removed 150 loc to drop the busy stuff.

On Fri, May 15, 2015 at 9:52 AM, <danakj@chromium.org> wrote:

> On 2015/05/15 03:38:36, piman (Very slow to review) wrote:
>
>
>
https://codereview.chromium.org/1139063002/diff/320001/cc/resources/resource_...
>
>> File cc/resources/resource_pool.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1139063002/diff/320001/cc/resources/resource_...
>
>> cc/resources/resource_pool.cc:85: scoped_ptr<ScopedResource>
>> ResourcePool::TryAcquireOldBusyResource(
>> On 2015/05/15 03:19:34, danakj wrote:
>> > I was thinking about it and I think this reusing busy resources is too
>>
> sketchy
>
>> > and not worth it.
>> >
>> > We're returning the resource in here as a scoped_ptr but it still
>> should be
>>
> in
>
>> > the busy_resources list, cuz it is busy. Else we need to track its
>> unbusy
>> state
>> > somehow when we return it. It seems fragile.
>> >
>> > Also, it's only needed for a cornercase of an invalidation in the
>> prepaint
>> area.
>> > For UI compositor there's basically no prepaint issue. And for
>> animations in
>> > prepaint, we try to avoid that in blink (disabling animated gifs etc).
>> >
>> > I'm going to remove the busy resource part from this patch, which also
>>
> removes
>
>> > all the complicated copying code and having 2 resources in the OneCopy
>>
> worker
>
>> > pool code.
>>
>
>  It sounds ok for UI, but for the renderer, aren't resources sent to the
>> parent
>> always busy (i.e. TryAcquireOldResource will always fail)?
>>
>
> For one copy this isn't using the resources on the tiles, it's using the
> resources used for uploading, so they have nothing to do with the ones
> sent to
> the browser compositor.
>
> For software compositing and gpu raster, we'll want to reuse resources in
> use on
> the tiles themselves cuz there's no staging buffers to reuse, but those
> will
> have different implementations.
>
>
>  We can leave it out of this patch, but I think it'd be nice to hit later
>> / as
>>
> a
>
>> followup. E.g. do we have to return it as a scoped_ptr? We're only reading
>>
> from
>
>> it. I guess we need to make sure it's not evicted nor written to if
>> referenced
>> by a task for reading.
>>
>
> https://codereview.chromium.org/1139063002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-15 18:30:04]

David, the only capability/promise I need from the GpuMemoryBuffer
implementation now is that mapping the same buffer twice gives you the same
pointer. Can we expose that on the GMB interface somewhere?

[danakj, 2015-05-15 18:51:21]

Patchset #20 (id:360001) has been deleted

[vmpstr, 2015-05-15 20:54:22]

This looks pretty good to me, just a couple of comments

https://codereview.chromium.org/1139063002/diff/380001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/380001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:500: bool
PictureLayerTiling::RemoveTileAt(int i, int j, Tile::Id* id) {
FWIW, now that you've changed Tile::id() to be > 0, this function can just
return Tile::Id with 0 indicating "no tile was erased" (unless the id
overflows).

https://codereview.chromium.org/1139063002/diff/380001/cc/resources/picture_p...
File cc/resources/picture_pile_impl_unittest.cc (right):

https://codereview.chromium.org/1139063002/diff/380001/cc/resources/picture_p...
cc/resources/picture_pile_impl_unittest.cc:402: // XXX ADD RASTER PARTIAL TILE
TESTS HERE XXX
Is this a TODO or are you going to do this before landing?

https://codereview.chromium.org/1139063002/diff/380001/cc/resources/raster_so...
File cc/resources/raster_source_helper.cc (right):

https://codereview.chromium.org/1139063002/diff/380001/cc/resources/raster_so...
cc/resources/raster_source_helper.cc:35: canvas_playback_rect -
canvas_bitmap_rect.OffsetFromOrigin()));
Can you leave a comment at the top of this function that explains the spaces
|canvas|, |canvas_playback_rect|  |canvas_bitmap_rect| are in?

What I mean to say it's not really clear to me, why this clip subtracts
bitmap_rect origin from playback_Rect, but say on line 70, we don't subtract
things when checking if content rect contains it, plus later we do a bunch of
translates /o\

This is possibly has nothing to do with your patch; this is probably just me not
really understanding what this code is doing.

[danakj, 2015-05-15 21:52:15]

https://codereview.chromium.org/1139063002/diff/380001/cc/resources/picture_p...
File cc/resources/picture_pile_impl_unittest.cc (right):

https://codereview.chromium.org/1139063002/diff/380001/cc/resources/picture_p...
cc/resources/picture_pile_impl_unittest.cc:402: // XXX ADD RASTER PARTIAL TILE
TESTS HERE XXX
On 2015/05/15 20:54:21, vmpstr wrote:
> Is this a TODO or are you going to do this before landing?

I'm going to do this before landing.

https://codereview.chromium.org/1139063002/diff/380001/cc/resources/raster_so...
File cc/resources/raster_source_helper.cc (right):

https://codereview.chromium.org/1139063002/diff/380001/cc/resources/raster_so...
cc/resources/raster_source_helper.cc:35: canvas_playback_rect -
canvas_bitmap_rect.OffsetFromOrigin()));
On 2015/05/15 20:54:22, vmpstr wrote:
> Can you leave a comment at the top of this function that explains the spaces
> |canvas|, |canvas_playback_rect|  |canvas_bitmap_rect| are in?
> 
> What I mean to say it's not really clear to me, why this clip subtracts
> bitmap_rect origin from playback_Rect, but say on line 70, we don't subtract
> things when checking if content rect contains it, plus later we do a bunch of
> translates /o\
> 
> This is possibly has nothing to do with your patch; this is probably just me
not
> really understanding what this code is doing.

OK sure. They're in the same space.

The thing is the canvas here hasn't been translated so we subtract the bitmap
rect to move into an untranslated space relative to the canvas instead of
relative to the recording/playback.

[reveman, 2015-05-16 17:00:45]

On 2015/05/15 at 18:30:04, danakj wrote:
> David, the only capability/promise I need from the GpuMemoryBuffer
implementation now is that mapping the same buffer twice gives you the same
pointer. Can we expose that on the GMB interface somewhere?

We could add a new type of usage. Currently we have MAP and SCANOUT. We could
add PERSISTENT_MAP. This would allow an implementation to be more efficient in
the MAP usage case (e.g. video playback) but the compositor could always use
PERSISTENT_MAP for tiles and behave the same in terms of partial updates
independent of the platform.

[danakj, 2015-05-20 22:53:07]

OK This is rebased onto the PERSISTENT_MAP GpuMemoryBuffer and makes a request
for a persistent buffer in the onecopy worker pool when making a buffer.

[piman, 2015-05-20 23:17:14]

LGTM for content. I don't feel quite knowledgeable enough to vet the rest, but
it looks reasonable enough.

[reveman, 2015-05-21 19:52:23]

Just a quick comment related to kUseImageTextureTarget. I haven't had a chance
to look at the cc/ changes yet but will do asap.

https://codereview.chromium.org/1139063002/diff/440001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1139063002/diff/440001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1145: if
(IsOneCopyUploadEnabled())
Can we add a IsPartialTileUpdatesEnabled() setting instead of making one-copy
and only one-copy always mean partial updates?

We can make it non-configurable for the browser compositor but I'd like to be
able to turn it on/off for the renderer where it's not clear if it's always the
best option.

[danakj, 2015-05-21 19:59:30]

https://codereview.chromium.org/1139063002/diff/440001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1139063002/diff/440001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1145: if
(IsOneCopyUploadEnabled())
On 2015/05/21 19:52:22, reveman wrote:
> Can we add a IsPartialTileUpdatesEnabled() setting instead of making one-copy
> and only one-copy always mean partial updates?
> 
> We can make it non-configurable for the browser compositor but I'd like to be
> able to turn it on/off for the renderer where it's not clear if it's always
the
> best option.

I don't understand the concern for the renderer, why would it be worse to reuse
the resource if it's available?

[reveman, 2015-05-21 20:23:23]

On 2015/05/21 at 19:59:30, danakj wrote:
>
https://codereview.chromium.org/1139063002/diff/440001/content/browser/render...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1139063002/diff/440001/content/browser/render...
> content/browser/renderer_host/render_process_host_impl.cc:1145: if
(IsOneCopyUploadEnabled())
> On 2015/05/21 19:52:22, reveman wrote:
> > Can we add a IsPartialTileUpdatesEnabled() setting instead of making
one-copy
> > and only one-copy always mean partial updates?
> > 
> > We can make it non-configurable for the browser compositor but I'd like to
be
> > able to turn it on/off for the renderer where it's not clear if it's always
the
> > best option.
> 
> I don't understand the concern for the renderer, why would it be worse to
reuse the resource if it's available?

If we have a native GpuMemoryBuffer implementation that supports MAP but not
PERSISTENT_MAP then we'd be giving up the ability to never have to perform
texture uploads in favor of partial updates.

[danakj, 2015-05-21 20:24:52]

On Thu, May 21, 2015 at 1:23 PM, <reveman@chromium.org> wrote:

> On 2015/05/21 at 19:59:30, danakj wrote:
>
>
>
https://codereview.chromium.org/1139063002/diff/440001/content/browser/render...
>
>> File content/browser/renderer_host/render_process_host_impl.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1139063002/diff/440001/content/browser/render...
>
>> content/browser/renderer_host/render_process_host_impl.cc:1145: if
>>
> (IsOneCopyUploadEnabled())
>
>> On 2015/05/21 19:52:22, reveman wrote:
>> > Can we add a IsPartialTileUpdatesEnabled() setting instead of making
>>
> one-copy
>
>> > and only one-copy always mean partial updates?
>> >
>> > We can make it non-configurable for the browser compositor but I'd like
>> to
>>
> be
>
>> > able to turn it on/off for the renderer where it's not clear if it's
>> always
>>
> the
>
>> > best option.
>>
>
>  I don't understand the concern for the renderer, why would it be worse to
>>
> reuse the resource if it's available?
>
> If we have a native GpuMemoryBuffer implementation that supports MAP but
> not
> PERSISTENT_MAP then we'd be giving up the ability to never have to perform
> texture uploads in favor of partial updates.
>

Why is this different for the renderer than the browser?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-21 20:44:44]

Added a unit test and fixed a few things vlad pointed out.

[reveman, 2015-05-21 20:48:05]

On 2015/05/21 at 20:24:52, danakj wrote:
> On Thu, May 21, 2015 at 1:23 PM, <reveman@chromium.org> wrote:
> 
> > On 2015/05/21 at 19:59:30, danakj wrote:
> >
> >
> >
https://codereview.chromium.org/1139063002/diff/440001/content/browser/render...
> >
> >> File content/browser/renderer_host/render_process_host_impl.cc (right):
> >>
> >
> >
> >
> >
https://codereview.chromium.org/1139063002/diff/440001/content/browser/render...
> >
> >> content/browser/renderer_host/render_process_host_impl.cc:1145: if
> >>
> > (IsOneCopyUploadEnabled())
> >
> >> On 2015/05/21 19:52:22, reveman wrote:
> >> > Can we add a IsPartialTileUpdatesEnabled() setting instead of making
> >>
> > one-copy
> >
> >> > and only one-copy always mean partial updates?
> >> >
> >> > We can make it non-configurable for the browser compositor but I'd like
> >> to
> >>
> > be
> >
> >> > able to turn it on/off for the renderer where it's not clear if it's
> >> always
> >>
> > the
> >
> >> > best option.
> >>
> >
> >  I don't understand the concern for the renderer, why would it be worse to
> >>
> > reuse the resource if it's available?
> >
> > If we have a native GpuMemoryBuffer implementation that supports MAP but
> > not
> > PERSISTENT_MAP then we'd be giving up the ability to never have to perform
> > texture uploads in favor of partial updates.
> >
> 
> Why is this different for the renderer than the browser?

I'm assuming that the type of situation where partial updates are useful is more
common for the browser UI. That is, relatively static contents with only small
parts animating. Eliminating texture uploads makes the most difference when you
need to move a lot of data to the GPU. Full viewport invalidations, scrolling a
page, etc. That seems much less common for the browser UI.

[danakj, 2015-05-21 20:50:24]

Patchset #24 (id:460001) has been deleted

[danakj, 2015-05-21 21:30:10]

OK I added a setting and disabled it for the renderer.

There's a lot of magic with this approach where compositor.cc sets a flag to
true on cc based on knowing that the GpuProcessTransportFactory will choose
PERSISTENT_MAP for the image texture target.

And render_widget_compositor sets it to false because RenderProcessHostImpl will
use MAP instead.

I'm not really sure how to make that better based on the PERSISTENT_MAP approach
though.

(I think I would have preferred just a MAP and a bool somewhere telling us if
it's persistent or not. And then fallback to the correct behaviour in one-copy
based on that bool. Then we could just try use it when possible, and if we
turned on native buffers and that made the bool false, we'd fallback to
non-partial updates. And I think that there'd be a bunch of less settings and
plumbing and decisions being made in many disparate places.)

[reveman, 2015-05-22 00:07:30]

On 2015/05/21 at 21:30:10, danakj wrote:
> OK I added a setting and disabled it for the renderer.
> 
> There's a lot of magic with this approach where compositor.cc sets a flag to
true on cc based on knowing that the GpuProcessTransportFactory will choose
PERSISTENT_MAP for the image texture target.

The decision for what target to choose would ideally be made at the same place
we configure the compositor (ui::Compositor ctor). Adding format and usage
parameters to the ContextFactory::GetImageTextureTarget() function will fix that
in the browser compositor case.

> 
> And render_widget_compositor sets it to false because RenderProcessHostImpl
will use MAP instead.

The current approach is that render_widget_compositor just does what
RenderProcessHostImpl tells it to do. Texture target, tile init mechanism, to
use partial updates or not, etc. Those decisions are all done by the browser.

> 
> I'm not really sure how to make that better based on the PERSISTENT_MAP
approach though.
> 
> (I think I would have preferred just a MAP and a bool somewhere telling us if
it's persistent or not. And then fallback to the correct behaviour in one-copy
based on that bool. Then we could just try use it when possible, and if we
turned on native buffers and that made the bool false, we'd fallback to
non-partial updates. And I think that there'd be a bunch of less settings and
plumbing and decisions being made in many disparate places.)

The embedder telling the compositor what update mechanism to use, if
partial-updates should be used, what GMB usage mode to use, what texture target
to use and cc then respecting those settings is a good approach imo. Less magic
inside cc and that code instead being dumb and just doing what told is good I
think.

[danakj, 2015-05-22 00:22:57]

On Thu, May 21, 2015 at 5:07 PM, <reveman@chromium.org> wrote:

> On 2015/05/21 at 21:30:10, danakj wrote:
>
>> OK I added a setting and disabled it for the renderer.
>>
>
>  There's a lot of magic with this approach where compositor.cc sets a flag
>> to
>>
> true on cc based on knowing that the GpuProcessTransportFactory will choose
> PERSISTENT_MAP for the image texture target.
>
> The decision for what target to choose would ideally be made at the same
> place
> we configure the compositor (ui::Compositor ctor). Adding format and usage
> parameters to the ContextFactory::GetImageTextureTarget() function will
> fix that
> in the browser compositor case.


Hm, okay. Doing that, it is more sane.

 And render_widget_compositor sets it to false because RenderProcessHostImpl
>>
> will use MAP instead.
>
> The current approach is that render_widget_compositor just does what
> RenderProcessHostImpl tells it to do. Texture target, tile init mechanism,
> to
> use partial updates or not, etc. Those decisions are all done by the
> browser.


True. I don't like the fact that we're disabling partial updates in the
renderer because if we try PERSISTENT_MAP we won't get native buffers and
maybe we want to have native buffers on some platforms. We're disabling
something unilaternally just in case.


>
>
>
>  I'm not really sure how to make that better based on the PERSISTENT_MAP
>>
> approach though.
>
>  (I think I would have preferred just a MAP and a bool somewhere telling
>> us if
>>
> it's persistent or not. And then fallback to the correct behaviour in
> one-copy
> based on that bool. Then we could just try use it when possible, and if we
> turned on native buffers and that made the bool false, we'd fallback to
> non-partial updates. And I think that there'd be a bunch of less settings
> and
> plumbing and decisions being made in many disparate places.)
>
> The embedder telling the compositor what update mechanism to use, if
> partial-updates should be used, what GMB usage mode to use, what texture
> target
> to use and cc then respecting those settings is a good approach imo. Less
> magic
> inside cc and that code instead being dumb and just doing what told is
> good I
> think.
>
> https://codereview.chromium.org/1139063002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-22 00:25:21]

PTAL

[reveman, 2015-05-22 00:31:45]

On 2015/05/22 at 00:22:57, danakj wrote:
> On Thu, May 21, 2015 at 5:07 PM, <reveman@chromium.org> wrote:
> 
> > On 2015/05/21 at 21:30:10, danakj wrote:
> >
> >> OK I added a setting and disabled it for the renderer.
> >>
> >
> >  There's a lot of magic with this approach where compositor.cc sets a flag
> >> to
> >>
> > true on cc based on knowing that the GpuProcessTransportFactory will choose
> > PERSISTENT_MAP for the image texture target.
> >
> > The decision for what target to choose would ideally be made at the same
> > place
> > we configure the compositor (ui::Compositor ctor). Adding format and usage
> > parameters to the ContextFactory::GetImageTextureTarget() function will
> > fix that
> > in the browser compositor case.
> 
> 
> Hm, okay. Doing that, it is more sane.
> 
>  And render_widget_compositor sets it to false because RenderProcessHostImpl
> >>
> > will use MAP instead.
> >
> > The current approach is that render_widget_compositor just does what
> > RenderProcessHostImpl tells it to do. Texture target, tile init mechanism,
> > to
> > use partial updates or not, etc. Those decisions are all done by the
> > browser.
> 
> 
> True. I don't like the fact that we're disabling partial updates in the
> renderer because if we try PERSISTENT_MAP we won't get native buffers and
> maybe we want to have native buffers on some platforms. We're disabling
> something unilaternally just in case.

That's not what I have in mind. I think this partial update mechanism is awesome
and we should try to enable it anywhere it provides a performance improvement.
We just need the same sort of control over it as we have with other settings
that might be an improvement on one platform but not on another. I'm thinking
we'll have a IsPartialTileUpdatesEnabled() function in compositor_utils.cc which
initially returns false by default but eventually becomes true by default on a
majority (if not all) platforms.

> 
> 
> >
> >
> >
> >  I'm not really sure how to make that better based on the PERSISTENT_MAP
> >>
> > approach though.
> >
> >  (I think I would have preferred just a MAP and a bool somewhere telling
> >> us if
> >>
> > it's persistent or not. And then fallback to the correct behaviour in
> > one-copy
> > based on that bool. Then we could just try use it when possible, and if we
> > turned on native buffers and that made the bool false, we'd fallback to
> > non-partial updates. And I think that there'd be a bunch of less settings
> > and
> > plumbing and decisions being made in many disparate places.)
> >
> > The embedder telling the compositor what update mechanism to use, if
> > partial-updates should be used, what GMB usage mode to use, what texture
> > target
> > to use and cc then respecting those settings is a good approach imo. Less
> > magic
> > inside cc and that code instead being dumb and just doing what told is
> > good I
> > think.
> >
> > https://codereview.chromium.org/1139063002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
>

[danakj, 2015-05-22 00:47:06]

On Thu, May 21, 2015 at 5:31 PM, <reveman@chromium.org> wrote:

> On 2015/05/22 at 00:22:57, danakj wrote:
>
>> On Thu, May 21, 2015 at 5:07 PM, <reveman@chromium.org> wrote:
>>
>
>  > On 2015/05/21 at 21:30:10, danakj wrote:
>> >
>> >> OK I added a setting and disabled it for the renderer.
>> >>
>> >
>> >  There's a lot of magic with this approach where compositor.cc sets a
>> flag
>> >> to
>> >>
>> > true on cc based on knowing that the GpuProcessTransportFactory will
>> choose
>> > PERSISTENT_MAP for the image texture target.
>> >
>> > The decision for what target to choose would ideally be made at the same
>> > place
>> > we configure the compositor (ui::Compositor ctor). Adding format and
>> usage
>> > parameters to the ContextFactory::GetImageTextureTarget() function will
>> > fix that
>> > in the browser compositor case.
>>
>
>
>  Hm, okay. Doing that, it is more sane.
>>
>
>    And render_widget_compositor sets it to false because
>> RenderProcessHostImpl
>> >>
>> > will use MAP instead.
>> >
>> > The current approach is that render_widget_compositor just does what
>> > RenderProcessHostImpl tells it to do. Texture target, tile init
>> mechanism,
>> > to
>> > use partial updates or not, etc. Those decisions are all done by the
>> > browser.
>>
>
>
>  True. I don't like the fact that we're disabling partial updates in the
>> renderer because if we try PERSISTENT_MAP we won't get native buffers and
>> maybe we want to have native buffers on some platforms. We're disabling
>> something unilaternally just in case.
>>
>
> That's not what I have in mind. I think this partial update mechanism is
> awesome
> and we should try to enable it anywhere it provides a performance
> improvement.
> We just need the same sort of control over it as we have with other
> settings
> that might be an improvement on one platform but not on another. I'm
> thinking
> we'll have a IsPartialTileUpdatesEnabled() function in compositor_utils.cc
> which
> initially returns false by default but eventually becomes true by default
> on a
> majority (if not all) platforms.


I see, ok! I've added a HavePersistentGpuMemoryBuffers() to
compositor_util.cc. I did this instead of IsPartialTileUpdatesEnabled()
because the requirements for one/zero copy are the GMB impl. For other
raster workers it will be very different, so I don't want to conflate the
two. I use this function in RenderProcessHostImpl and pass it to the
RenderThreadImpl so the renderer disables it explicitly when native buffers
are not enabled (controlled in compositor_util.cc).

For ui::Compositor i'm just turning it on and requesting PERSISTENT_MAP
always for one-copy.

PTAL!

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmpstr, 2015-05-22 01:08:57]

tiles/playback stuff lgtm

https://codereview.chromium.org/1139063002/diff/630001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_tiles.cc (right):

https://codereview.chromium.org/1139063002/diff/630001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_tiles.cc:72: class BlueYellowClient : public
ContentLayerClient {
Nice test.

[reveman, 2015-05-22 17:15:14]

This is cool. Hope we can make the ResourcePool/TileTaskRunner changes a bit
easier to understand but looks good otherwise.

Fyi, as a follow up, we can also experiment with partial copy operations once
https://codereview.chromium.org/1143373004 lands.

https://codereview.chromium.org/1139063002/diff/690001/cc/playback/picture_pi...
File cc/playback/picture_pile_impl.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/playback/picture_pi...
cc/playback/picture_pile_impl.h:43: const gfx::Rect& canvas_playback_rect,
nit: is the canvas always backed by a bitmap? maybe canvas_rect and
playback_rect if not

https://codereview.chromium.org/1139063002/diff/690001/cc/playback/raster_sou...
File cc/playback/raster_source.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/playback/raster_sou...
cc/playback/raster_source.h:45: const gfx::Rect& canvas_playback_rect,
nit: same here. maybe canvas_rect and playback_rect if not it's not always
backed by a bitmap

https://codereview.chromium.org/1139063002/diff/690001/cc/playback/raster_sou...
File cc/playback/raster_source_helper.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/playback/raster_sou...
cc/playback/raster_source_helper.h:20: const gfx::Rect& canvas_playback_rect,
nit: and here

https://codereview.chromium.org/1139063002/diff/690001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:36: };
If I understand this correctly, here's how this works (please correct me where
wrong).

1. We're basically implementing discardable resources. Each resource is assigned
an ID that uniquely identifies the content. We can then later lookup this
resource using the ID to find out if it has been discarded (reused or deleted)
or not. If not, great! We'll reuse it.

2. We keep track the dirty rect of a tile compared to the contents of the
previously rasterized tile.

3. If the old resource has not been discarded, we use it to only rasterize the
dirty rect.

Questions

Can we implement discardable resources as a content ID in the Resource class
instead? I think that would make that part of the system easier to understand as
it would be independent of our tile management system. Each Resource instance
would have a contents ID or a generation ID (think skia's pixelrefs). The
Resource class would be responsible for generating unique IDs as a result of
something like Resource::ContentsChanged() being called. Wdyt?

Do we really need to pass everything in TileTaskData to AcquireBufferForRaster?
Is the Resource that we need a raster buffer for and the contents ID not
sufficient? I think it would be more appropriate to pass the dirty rect as the
|rect| param to RasterBuffer::Playback.

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_pool.cc:67: if (!id)
hm, doesn't it make more sense for the caller to check this?

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_pool.cc:77: if (resource->format() != format ||
resource->size() != size)
how can a resource with the same ID have a different size or format?

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_pool.h:30: scoped_ptr<ScopedResource>
TryAcquireOldResource(const gfx::Size& size,
OldResource -> ResourceWithContentsId ?

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_pool.h:32: uint64 id);
Looking at this class in isolation it's hard to understand what this "id" is.
There's also Resource->id() but that's a not the same but it's hard to see why
here. I think it would be easier to understand if this "id" here moved into the
Resource class as "contents ID" with an explicit description that it uniquely
identifies the contents.

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_provider.h:324: bool AllocateGpuMemoryBuffer(bool
persistent);
To allocate with MAP_PERSISTENT usage or not is NOT an option. Please keep all
this as before and simply pass the GMB usage mode to the ResourceProvider ctor.
If the embedder tells us to use MAP_PERSISTENT then we have no choice but use
MAP_PERSISTENT. Texture target will be wrong otherwise.

https://codereview.chromium.org/1139063002/diff/690001/cc/trees/layer_tree_se...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/trees/layer_tree_se...
cc/trees/layer_tree_settings.h:76: bool have_persistent_gpu_memory_buffers;
use_persistent_map_gpu_memory_buffer_usage

As "have_persistent_gpu_memory_buffers" makes it seem like the compositor has a
choice. It doesn't. If this flag is set, the compositor MUST use MAP_PERSISTENT
mode or the texture target will be wrong.

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
content/browser/gpu/compositor_util.cc:283: }
How about we land the initial version of this with this function just returning
false and then we turn it on in a follow up patch?

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
File content/browser/gpu/compositor_util.h (right):

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
content/browser/gpu/compositor_util.h:37: CONTENT_EXPORT bool
HavePersistentGpuMemoryBuffers();
I'm not sure the name of this function makes sense. We can always support it as
we have a shmem fallback. --enable-native-gpu-memory-buffers doesn't mean that
that native type is the only implementation used. It means that we'll use it if
supported by the format and usage requested.

I think IsPersistentMapGpuMemoryBufferUsageEnabled() would be more appropriate
as the implementation of this function answers the question of whether we should
use persistent map or not.

https://codereview.chromium.org/1139063002/diff/690001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/content/public/common/...
content/public/common/content_switches.cc:854: // GpuMemoryBuffers.
"can depend on" is not accurate. "should use" is better.

https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
ui/compositor/compositor.cc:147: if (IsUIOneCopyEnabled()) {
What if IsUIZeroCopyEnabled() is also enabled? With this code we'd have to use
zero-copy with map-persistent usage too, which is fine but I'm guessing not what
you intended.

[danakj, 2015-05-26 19:02:53]

Will throw up a patch after lunch

https://codereview.chromium.org/1139063002/diff/690001/cc/playback/picture_pi...
File cc/playback/picture_pile_impl.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/playback/picture_pi...
cc/playback/picture_pile_impl.h:43: const gfx::Rect& canvas_playback_rect,
On 2015/05/22 17:15:13, reveman wrote:
> nit: is the canvas always backed by a bitmap? maybe canvas_rect and
> playback_rect if not

It is. For gpu raster the bitmap is backed by a texture, but it's always an
SkBitmap (ie we're rastering here not recording).

https://codereview.chromium.org/1139063002/diff/690001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:36: };
On 2015/05/22 17:15:13, reveman wrote:
> If I understand this correctly, here's how this works (please correct me where
> wrong).
> 
> 1. We're basically implementing discardable resources. Each resource is
assigned
> an ID that uniquely identifies the content. We can then later lookup this
> resource using the ID to find out if it has been discarded (reused or deleted)
> or not. If not, great! We'll reuse it.
> 
> 2. We keep track the dirty rect of a tile compared to the contents of the
> previously rasterized tile.
> 
> 3. If the old resource has not been discarded, we use it to only rasterize the
> dirty rect.

Correct.

> Questions
> 
> Can we implement discardable resources as a content ID in the Resource class
> instead? I think that would make that part of the system easier to understand
as
> it would be independent of our tile management system. Each Resource instance
> would have a contents ID or a generation ID (think skia's pixelrefs). The
> Resource class would be responsible for generating unique IDs as a result of
> something like Resource::ContentsChanged() being called. Wdyt?

I like the idea of calling this a "content id", that makes a lot of sense.

I don't think this should live on the Resource. This is something that is being
implemented by ResourcePool and only the pool needs to know about it. It
provides a way to reuse resources in the pool based on the id. No other users of
ResourceId need to worry about this.

> Do we really need to pass everything in TileTaskData to
AcquireBufferForRaster?
> Is the Resource that we need a raster buffer for and the contents ID not
> sufficient? I think it would be more appropriate to pass the dirty rect as the
> |rect| param to RasterBuffer::Playback.

This seems good! I was just unaware of how everything fits together. Let me try
that out.

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_pool.cc:67: if (!id)
On 2015/05/22 17:15:13, reveman wrote:
> hm, doesn't it make more sense for the caller to check this?

Sure, can do if you. It means if (id && TryBlah()) instead of if (TryBlah()) at
each call site, which I thought seemed noisier.

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_pool.cc:77: if (resource->format() != format ||
resource->size() != size)
On 2015/05/22 17:15:13, reveman wrote:
> how can a resource with the same ID have a different size or format?

I don't know. :) I guess that it can't. I'll DCHECK instead.

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_provider.h:324: bool AllocateGpuMemoryBuffer(bool
persistent);
On 2015/05/22 17:15:13, reveman wrote:
> To allocate with MAP_PERSISTENT usage or not is NOT an option. Please keep all
> this as before and simply pass the GMB usage mode to the ResourceProvider
ctor.
> If the embedder tells us to use MAP_PERSISTENT then we have no choice but use
> MAP_PERSISTENT. Texture target will be wrong otherwise.

Ah, ya okay that sounds good.

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
content/browser/gpu/compositor_util.cc:283: }
On 2015/05/22 17:15:13, reveman wrote:
> How about we land the initial version of this with this function just
returning
> false and then we turn it on in a follow up patch?

Sure, if you like. That'll make reverts smaller.

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
File content/browser/gpu/compositor_util.h (right):

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
content/browser/gpu/compositor_util.h:37: CONTENT_EXPORT bool
HavePersistentGpuMemoryBuffers();
On 2015/05/22 17:15:13, reveman wrote:
> I'm not sure the name of this function makes sense. We can always support it
as
> we have a shmem fallback. --enable-native-gpu-memory-buffers doesn't mean that
> that native type is the only implementation used. It means that we'll use it
if
> supported by the format and usage requested.
> 
> I think IsPersistentMapGpuMemoryBufferUsageEnabled() would be more appropriate
> as the implementation of this function answers the question of whether we
should
> use persistent map or not.

okay~

https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
ui/compositor/compositor.cc:147: if (IsUIOneCopyEnabled()) {
On 2015/05/22 17:15:13, reveman wrote:
> What if IsUIZeroCopyEnabled() is also enabled? With this code we'd have to use
> zero-copy with map-persistent usage too, which is fine but I'm guessing not
what
> you intended.

If zero copy is enabled, then one copy is disabled, and we'd use MAP instead.
No?

[reveman, 2015-05-26 20:12:37]

https://codereview.chromium.org/1139063002/diff/690001/cc/playback/picture_pi...
File cc/playback/picture_pile_impl.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/playback/picture_pi...
cc/playback/picture_pile_impl.h:43: const gfx::Rect& canvas_playback_rect,
On 2015/05/26 at 19:02:52, danakj wrote:
> On 2015/05/22 17:15:13, reveman wrote:
> > nit: is the canvas always backed by a bitmap? maybe canvas_rect and
> > playback_rect if not
> 
> It is. For gpu raster the bitmap is backed by a texture, but it's always an
SkBitmap (ie we're rastering here not recording).

Ok, makes sense then.

https://codereview.chromium.org/1139063002/diff/690001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:36: };
On 2015/05/26 at 19:02:52, danakj wrote:
> On 2015/05/22 17:15:13, reveman wrote:
> > If I understand this correctly, here's how this works (please correct me
where
> > wrong).
> > 
> > 1. We're basically implementing discardable resources. Each resource is
assigned
> > an ID that uniquely identifies the content. We can then later lookup this
> > resource using the ID to find out if it has been discarded (reused or
deleted)
> > or not. If not, great! We'll reuse it.
> > 
> > 2. We keep track the dirty rect of a tile compared to the contents of the
> > previously rasterized tile.
> > 
> > 3. If the old resource has not been discarded, we use it to only rasterize
the
> > dirty rect.
> 
> Correct.
> 
> > Questions
> > 
> > Can we implement discardable resources as a content ID in the Resource class
> > instead? I think that would make that part of the system easier to
understand as
> > it would be independent of our tile management system. Each Resource
instance
> > would have a contents ID or a generation ID (think skia's pixelrefs). The
> > Resource class would be responsible for generating unique IDs as a result of
> > something like Resource::ContentsChanged() being called. Wdyt?
> 
> I like the idea of calling this a "content id", that makes a lot of sense.
> 
> I don't think this should live on the Resource. This is something that is
being implemented by ResourcePool and only the pool needs to know about it. It
provides a way to reuse resources in the pool based on the id. No other users of
ResourceId need to worry about this.

I was thinking the cc::Resource class and not the ResourceId. Either way,
keeping it in the resource pool is fine if you think that's more appropriate.

> 
> > Do we really need to pass everything in TileTaskData to
AcquireBufferForRaster?
> > Is the Resource that we need a raster buffer for and the contents ID not
> > sufficient? I think it would be more appropriate to pass the dirty rect as
the
> > |rect| param to RasterBuffer::Playback.
> 
> This seems good! I was just unaware of how everything fits together. Let me
try that out.

Cool.

https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
ui/compositor/compositor.cc:147: if (IsUIOneCopyEnabled()) {
On 2015/05/26 at 19:02:52, danakj wrote:
> On 2015/05/22 17:15:13, reveman wrote:
> > What if IsUIZeroCopyEnabled() is also enabled? With this code we'd have to
use
> > zero-copy with map-persistent usage too, which is fine but I'm guessing not
what
> > you intended.
> 
> If zero copy is enabled, then one copy is disabled, and we'd use MAP instead.
No?

Ah, didn't realize that we have checks inside these functions that prevent both
from being enabled at the same time. That's a bit weird considering that
one-copy will soon be removed as an option. I'd avoid using IsUIOneCopyEnabled
for anything other than turning on one-copy.

I think we can just remove this check. If we need partial updates for good UI
compositor performance then let's always turn it on here. If it turns out that
there is some device out there where we need to disable this then we can
introduce a command line switch for it if necessary.

[danakj, 2015-05-26 20:17:37]

https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
ui/compositor/compositor.cc:147: if (IsUIOneCopyEnabled()) {
On 2015/05/26 20:12:37, reveman wrote:
> On 2015/05/26 at 19:02:52, danakj wrote:
> > On 2015/05/22 17:15:13, reveman wrote:
> > > What if IsUIZeroCopyEnabled() is also enabled? With this code we'd have to
> use
> > > zero-copy with map-persistent usage too, which is fine but I'm guessing
not
> what
> > > you intended.
> > 
> > If zero copy is enabled, then one copy is disabled, and we'd use MAP
instead.
> No?
> 
> Ah, didn't realize that we have checks inside these functions that prevent
both
> from being enabled at the same time. That's a bit weird considering that
> one-copy will soon be removed as an option. I'd avoid using IsUIOneCopyEnabled
> for anything other than turning on one-copy.
> 
> I think we can just remove this check. If we need partial updates for good UI
> compositor performance then let's always turn it on here. If it turns out that
> there is some device out there where we need to disable this then we can
> introduce a command line switch for it if necessary.

But there's no point turning it on for zero copy if the zero copy worker pool
doesn't support it. I have the check here because that's the only worker pool
that has support. I'd remove it when zero copy also supports it. I can add a
TODO? Or I can remove the check if you prefer.

[reveman, 2015-05-26 20:29:31]

On 2015/05/26 at 20:17:37, danakj wrote:
>
https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
> ui/compositor/compositor.cc:147: if (IsUIOneCopyEnabled()) {
> On 2015/05/26 20:12:37, reveman wrote:
> > On 2015/05/26 at 19:02:52, danakj wrote:
> > > On 2015/05/22 17:15:13, reveman wrote:
> > > > What if IsUIZeroCopyEnabled() is also enabled? With this code we'd have
to
> > use
> > > > zero-copy with map-persistent usage too, which is fine but I'm guessing
not
> > what
> > > > you intended.
> > > 
> > > If zero copy is enabled, then one copy is disabled, and we'd use MAP
instead.
> > No?
> > 
> > Ah, didn't realize that we have checks inside these functions that prevent
both
> > from being enabled at the same time. That's a bit weird considering that
> > one-copy will soon be removed as an option. I'd avoid using
IsUIOneCopyEnabled
> > for anything other than turning on one-copy.
> > 
> > I think we can just remove this check. If we need partial updates for good
UI
> > compositor performance then let's always turn it on here. If it turns out
that
> > there is some device out there where we need to disable this then we can
> > introduce a command line switch for it if necessary.
> 
> But there's no point turning it on for zero copy if the zero copy worker pool
doesn't support it. I have the check here because that's the only worker pool
that has support. I'd remove it when zero copy also supports it. I can add a
TODO? Or I can remove the check if you prefer.

I'd remove it to keep things simple and not make any assumptions about the
compositor implementation here. Future support for zero-copy partial updates in
the compositor would not require modifications here..

[danakj, 2015-05-26 23:37:35]

PTAL

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_pool.cc:67: if (!id)
On 2015/05/26 19:02:52, danakj wrote:
> On 2015/05/22 17:15:13, reveman wrote:
> > hm, doesn't it make more sense for the caller to check this?
> 
> Sure, can do if you. It means if (id && TryBlah()) instead of if (TryBlah())
at
> each call site, which I thought seemed noisier.

Done.

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_pool.cc:77: if (resource->format() != format ||
resource->size() != size)
On 2015/05/26 19:02:52, danakj wrote:
> On 2015/05/22 17:15:13, reveman wrote:
> > how can a resource with the same ID have a different size or format?
> 
> I don't know. :) I guess that it can't. I'll DCHECK instead.

Done.

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_pool.h:30: scoped_ptr<ScopedResource>
TryAcquireOldResource(const gfx::Size& size,
On 2015/05/22 17:15:13, reveman wrote:
> OldResource -> ResourceWithContentsId ?

Done.

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_pool.h:32: uint64 id);
On 2015/05/22 17:15:13, reveman wrote:
> Looking at this class in isolation it's hard to understand what this "id" is.
> There's also Resource->id() but that's a not the same but it's hard to see why
> here. I think it would be easier to understand if this "id" here moved into
the
> Resource class as "contents ID" with an explicit description that it uniquely
> identifies the contents.

Renamed to be explicitly content_id everywhere.

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/resources/resource_...
cc/resources/resource_provider.h:324: bool AllocateGpuMemoryBuffer(bool
persistent);
On 2015/05/26 19:02:52, danakj wrote:
> On 2015/05/22 17:15:13, reveman wrote:
> > To allocate with MAP_PERSISTENT usage or not is NOT an option. Please keep
all
> > this as before and simply pass the GMB usage mode to the ResourceProvider
> ctor.
> > If the embedder tells us to use MAP_PERSISTENT then we have no choice but
use
> > MAP_PERSISTENT. Texture target will be wrong otherwise.
> 
> Ah, ya okay that sounds good.

Done.

https://codereview.chromium.org/1139063002/diff/690001/cc/trees/layer_tree_se...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/1139063002/diff/690001/cc/trees/layer_tree_se...
cc/trees/layer_tree_settings.h:76: bool have_persistent_gpu_memory_buffers;
On 2015/05/22 17:15:13, reveman wrote:
> use_persistent_map_gpu_memory_buffer_usage
> 
> As "have_persistent_gpu_memory_buffers" makes it seem like the compositor has
a
> choice. It doesn't. If this flag is set, the compositor MUST use
MAP_PERSISTENT
> mode or the texture target will be wrong.

Done.

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
content/browser/gpu/compositor_util.cc:283: }
On 2015/05/26 19:02:52, danakj wrote:
> On 2015/05/22 17:15:13, reveman wrote:
> > How about we land the initial version of this with this function just
> returning
> > false and then we turn it on in a follow up patch?
> 
> Sure, if you like. That'll make reverts smaller.

Done.

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
File content/browser/gpu/compositor_util.h (right):

https://codereview.chromium.org/1139063002/diff/690001/content/browser/gpu/co...
content/browser/gpu/compositor_util.h:37: CONTENT_EXPORT bool
HavePersistentGpuMemoryBuffers();
On 2015/05/26 19:02:52, danakj wrote:
> On 2015/05/22 17:15:13, reveman wrote:
> > I'm not sure the name of this function makes sense. We can always support it
> as
> > we have a shmem fallback. --enable-native-gpu-memory-buffers doesn't mean
that
> > that native type is the only implementation used. It means that we'll use it
> if
> > supported by the format and usage requested.
> > 
> > I think IsPersistentMapGpuMemoryBufferUsageEnabled() would be more
appropriate
> > as the implementation of this function answers the question of whether we
> should
> > use persistent map or not.
> 
> okay~

Done.

https://codereview.chromium.org/1139063002/diff/690001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/content/public/common/...
content/public/common/content_switches.cc:854: // GpuMemoryBuffers.
On 2015/05/22 17:15:13, reveman wrote:
> "can depend on" is not accurate. "should use" is better.

Done.

https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1139063002/diff/690001/ui/compositor/composit...
ui/compositor/compositor.cc:147: if (IsUIOneCopyEnabled()) {
On 2015/05/26 20:17:37, danakj wrote:
> On 2015/05/26 20:12:37, reveman wrote:
> > On 2015/05/26 at 19:02:52, danakj wrote:
> > > On 2015/05/22 17:15:13, reveman wrote:
> > > > What if IsUIZeroCopyEnabled() is also enabled? With this code we'd have
to
> > use
> > > > zero-copy with map-persistent usage too, which is fine but I'm guessing
> not
> > what
> > > > you intended.
> > > 
> > > If zero copy is enabled, then one copy is disabled, and we'd use MAP
> instead.
> > No?
> > 
> > Ah, didn't realize that we have checks inside these functions that prevent
> both
> > from being enabled at the same time. That's a bit weird considering that
> > one-copy will soon be removed as an option. I'd avoid using
IsUIOneCopyEnabled
> > for anything other than turning on one-copy.
> > 
> > I think we can just remove this check. If we need partial updates for good
UI
> > compositor performance then let's always turn it on here. If it turns out
that
> > there is some device out there where we need to disable this then we can
> > introduce a command line switch for it if necessary.
> 
> But there's no point turning it on for zero copy if the zero copy worker pool
> doesn't support it. I have the check here because that's the only worker pool
> that has support. I'd remove it when zero copy also supports it. I can add a
> TODO? Or I can remove the check if you prefer.

Done.

[danakj, 2015-05-26 23:38:11]

Patchset #36 (id:700001) has been deleted

[danakj, 2015-05-26 23:48:25]

I can make this patch a teeny bit smaller by moving the ContextFactory API
change to another CL (doing so).

[danakj, 2015-05-26 23:57:13]

If we're not turning on the renderer here, I can drop those pieces to another CL
too (doing so).

[danakj, 2015-05-27 00:02:41]

Woot ok even smaller. This is just a cc/ change now plus enabling it in
ui/compositor.

[reveman, 2015-05-28 05:07:25]

https://codereview.chromium.org/1139063002/diff/790001/cc/playback/raster_sou...
File cc/playback/raster_source.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/playback/raster_sou...
cc/playback/raster_source.h:45: const gfx::Rect& canvas_playback_rect,
Instead of adding another rect here, can we use SkCanvas::getBaseLayerSize() to
determine if playback_rect represents the whole canvas and optimized clear
functions can be used?

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (left):

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:52: if (raster_resource_)
I guess the removal of this is just a leftover from my partial copy patch,
correct?

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:55: bool did_playback = !lock_;
you can use |sequence_| to determine if raster_resource_ was updated

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:385: gfx::Rect(0, y,
raster_resource->size().width(), rows_to_copy))));
hard to see if this code was changed. looks like it's only renaming. would be
nice to avoid that unless it's important somehow?

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.h:68: const Resource* upload_resource,
s/upload/copy/. what's wrong with |write_lock|, |src| and |dst| names? is it
necessary to change these names as part of this patch?

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.h:102:
scoped_ptr<ResourceProvider::ScopedWriteLockGpuMemoryBuffer> src_write_lock;
is this name change necessary?

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:14: #include "ui/gfx/geometry/rect.h"
don't think you need this anymore

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:22: struct TileTaskData {
not a fan of things named "data", "info", etc. can we instead just pass what is
needed as parameters to AcquireBufferForRaster?

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:39: virtual void
ReleaseBufferForRaster(scoped_ptr<RasterBuffer> buffer) = 0;
Discussed this with Vlad and I think there are some minor changes that can be
made to make this API easier to understand from the tile manager's point of
view. What if the interface and the tile manager usage looked like this:

// Acquires a buffer for raster to |resource|. The buffer should preferably
// be populated with contents matching |preferred_content_id|. The actual
// contents of the buffer is returned in |actual_content_id|.
virtual scoped_ptr<RasterBuffer> AcquireBufferForRaster(
    const Resource* resource,
    uint64_t preferred_content_id,
    uint64_t* actual_content_id);

// Releases a previously acquired buffer. |content_id| uniquely identifies the
// contents of the buffer.
virtual void ReleaseBufferForRaster(scoped_ptr<RasterBuffer> buffer,
                                    uint64_t content_id);


RasterBufferImpl in tile manager would do:

void ScheduleOnOriginThread(TileTaskClient* client) override {
  raster_buffer_ = client->AcquireBufferForRaster(
      resource(), previous_content_id_, &raster_buffer_content_id_);
}
void Raster(const RasterSource* raster_source) {
  gfx::Rect playback_rect =
      raster_buffer_content_id_ == previous_content_id_ ?
      dirty_content_rect_ : content_rect_;
  
  raster_buffer_->Playback(raster_source_.get(),
                           playback_rect,
                           contents_scale_);
}
void CompleteOnOriginThread(TileTaskClient* client) override {
  client->ReleaseBufferForRaster(raster_buffer_.Pass(), new_content_id_);
}

The idea is that the flow would become a bit easier to understand. We acquire a
buffer, decide what part of the raster source to playback based on the result of
that and then release the raster buffer when done while also providing a new
content id the represents the result of the raster done to the buffer.

Wdyt?

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
cc/resources/resource_pool.cc:70: [content_id](const PoolResource& pr) {
nit: s/pr/pool_resource/

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
cc/resources/resource_pool.cc:79: DCHECK_EQ(size.ToString(),
resource->size().ToString());
I don't think we should have parameters that are only used for DCHECKs. I would
prefer if size+format params were removed and these DCHECKs were moved to the
caller if this function is kept as is.

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
cc/resources/resource_pool.h:33: uint64 content_id);
As we don't have two different versions of ReleaseResource, should we also use a
single version of AcquireResource with an optional content_id parameter?

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
cc/resources/resource_pool.h:80: typedef std::deque<PoolResource> ResourceList;
I would still prefer if this list -> deque change was done in a separate patch.

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
cc/resources/resource_provider.h:608: bool use_persistent_map_;
There are 4 different ways of referring to this in this patch:

- have_persistent_gpu_memory_buffers
- use_persistent_map_for_gpu_memory_buffers
- use_persistent_map
- use_persistent_map_gpu_memory_buffer_usage

use_persistent_map_gpu_memory_buffer_usage exists since before and is the most
appropriate term IMO. Would be nice if we could use one term consistently
throughout the code.

https://codereview.chromium.org/1139063002/diff/790001/cc/tiles/picture_layer...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/tiles/picture_layer...
cc/tiles/picture_layer_tiling.cc:270: base::SmallMap<base::hash_map<TileMapKey,
gfx::Rect>, 16> remove_tiles;
why 16? can we remove this magic value or add a comment that explains why 16
makes sense here?

https://codereview.chromium.org/1139063002/diff/790001/cc/tiles/picture_layer...
File cc/tiles/picture_layer_tiling.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/tiles/picture_layer...
cc/tiles/picture_layer_tiling.h:281: bool RemoveTileAt(int i, int j, Tile::Id*
id);
adding an 'id' output param seems a bit too specific for the exact usage of this
patch. I think it would be nicer to add an alternative TakeTileAt function that
allows you to get the id when needed and the existing RemoveTileAt function can
simply be implemented on top of it.

[danakj, 2015-05-28 18:54:45]

PTAL

https://codereview.chromium.org/1139063002/diff/790001/cc/playback/raster_sou...
File cc/playback/raster_source.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/playback/raster_sou...
cc/playback/raster_source.h:45: const gfx::Rect& canvas_playback_rect,
On 2015/05/28 05:07:24, reveman wrote:
> Instead of adding another rect here, can we use SkCanvas::getBaseLayerSize()
to
> determine if playback_rect represents the whole canvas and optimized clear
> functions can be used?

I don't like this, it really limits what the caller can do with the SkCanvas. It
could have a larger canvas (a 256x256 canvas?) for a smaller tile (an edge
tile?).

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (left):

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:52: if (raster_resource_)
On 2015/05/28 05:07:24, reveman wrote:
> I guess the removal of this is just a leftover from my partial copy patch,
> correct?

Yep

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:55: bool did_playback = !lock_;
On 2015/05/28 05:07:24, reveman wrote:
> you can use |sequence_| to determine if raster_resource_ was updated

Done.

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:385: gfx::Rect(0, y,
raster_resource->size().width(), rows_to_copy))));
On 2015/05/28 05:07:24, reveman wrote:
> hard to see if this code was changed. looks like it's only renaming. would be
> nice to avoid that unless it's important somehow?

this is just renaming, to make it clear what resource we're rastering vs
uploading to. They are still src/dst inside CopyOperation (it's a src->dst copy)
but over here we're rastering some resource and rastering to the "src" was super
unclear. This helped me get my head around what was going on in this class, so
I'd like to keep it?

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.h:68: const Resource* upload_resource,
On 2015/05/28 05:07:24, reveman wrote:
> s/upload/copy/. what's wrong with |write_lock|, |src| and |dst| names? is it
> necessary to change these names as part of this patch?

see previous comment?

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:14: #include "ui/gfx/geometry/rect.h"
On 2015/05/28 05:07:25, reveman wrote:
> don't think you need this anymore

Done.

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:22: struct TileTaskData {
On 2015/05/28 05:07:24, reveman wrote:
> not a fan of things named "data", "info", etc. can we instead just pass what
is
> needed as parameters to AcquireBufferForRaster?

I can. Every time we add a new parameter it requires touching like 20 files
(look at this CL) whereas a struct would let us touch a lot less - just the
places that use it. I agree "Data" isn't a generally useful name, this is just
the parameters for AcquireBufferForRaster. I could call it AcquireBufferData?
AcquireBufferParams? Something else?

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:39: virtual void
ReleaseBufferForRaster(scoped_ptr<RasterBuffer> buffer) = 0;
On 2015/05/28 05:07:25, reveman wrote:
> Discussed this with Vlad and I think there are some minor changes that can be
> made to make this API easier to understand from the tile manager's point of
> view. What if the interface and the tile manager usage looked like this:
> 
> // Acquires a buffer for raster to |resource|. The buffer should preferably
> // be populated with contents matching |preferred_content_id|. The actual
> // contents of the buffer is returned in |actual_content_id|.
> virtual scoped_ptr<RasterBuffer> AcquireBufferForRaster(
>     const Resource* resource,
>     uint64_t preferred_content_id,
>     uint64_t* actual_content_id);
> 
> // Releases a previously acquired buffer. |content_id| uniquely identifies the
> // contents of the buffer.
> virtual void ReleaseBufferForRaster(scoped_ptr<RasterBuffer> buffer,
>                                     uint64_t content_id);
> 
> 
> RasterBufferImpl in tile manager would do:
> 
> void ScheduleOnOriginThread(TileTaskClient* client) override {
>   raster_buffer_ = client->AcquireBufferForRaster(
>       resource(), previous_content_id_, &raster_buffer_content_id_);
> }
> void Raster(const RasterSource* raster_source) {
>   gfx::Rect playback_rect =
>       raster_buffer_content_id_ == previous_content_id_ ?
>       dirty_content_rect_ : content_rect_;

This doesn't work. Tile manager doesn't know if the ResourcePool has a resource
with the old content ID. The worker pool needs both rects and needs to decide
which one to raster based on if it finds the old content ID resource or not.

>   raster_buffer_->Playback(raster_source_.get(),
>                            playback_rect,
>                            contents_scale_);
> }
> void CompleteOnOriginThread(TileTaskClient* client) override {
>   client->ReleaseBufferForRaster(raster_buffer_.Pass(), new_content_id_);
> }
> 
> The idea is that the flow would become a bit easier to understand. We acquire
a
> buffer, decide what part of the raster source to playback based on the result
of
> that and then release the raster buffer when done while also providing a new
> content id the represents the result of the raster done to the buffer.
> 
> Wdyt? 

I think that this doesn't really work and we should figure out what bitmap and
gpu raster partial raster works before trying to rewrite APIs.

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
cc/resources/resource_pool.cc:70: [content_id](const PoolResource& pr) {
On 2015/05/28 05:07:25, reveman wrote:
> nit: s/pr/pool_resource/

Done.

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
cc/resources/resource_pool.cc:79: DCHECK_EQ(size.ToString(),
resource->size().ToString());
On 2015/05/28 05:07:25, reveman wrote:
> I don't think we should have parameters that are only used for DCHECKs. I
would
> prefer if size+format params were removed and these DCHECKs were moved to the
> caller if this function is kept as is.

Done.

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
cc/resources/resource_pool.h:33: uint64 content_id);
On 2015/05/28 05:07:25, reveman wrote:
> As we don't have two different versions of ReleaseResource, should we also use
a
> single version of AcquireResource with an optional content_id parameter?

I don't think so. That would complicate the caller code. It would need another
bool outparam for if it's the old resource found or not.

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
cc/resources/resource_pool.h:80: typedef std::deque<PoolResource> ResourceList;
On 2015/05/28 05:07:25, reveman wrote:
> I would still prefer if this list -> deque change was done in a separate
patch.

It seems I'm no longer using deque APIs in this CL. Removed it.

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/resources/resource_...
cc/resources/resource_provider.h:608: bool use_persistent_map_;
On 2015/05/28 05:07:25, reveman wrote:
> There are 4 different ways of referring to this in this patch:
> 
> - have_persistent_gpu_memory_buffers
> - use_persistent_map_for_gpu_memory_buffers
> - use_persistent_map
> - use_persistent_map_gpu_memory_buffer_usage
> 
> use_persistent_map_gpu_memory_buffer_usage exists since before and is the most
> appropriate term IMO. Would be nice if we could use one term consistently
> throughout the code.

I think I disagree. The workerpool isn't being told to to "use persistent map".
It's being told that its gpu memory buffers will be persistent, ie "it has
persistent buffers".

I have consolidated the last 3 as use_persistent_map_for_gpu_memory_buffers,
which means exactly what it says.

https://codereview.chromium.org/1139063002/diff/790001/cc/tiles/picture_layer...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/tiles/picture_layer...
cc/tiles/picture_layer_tiling.cc:270: base::SmallMap<base::hash_map<TileMapKey,
gfx::Rect>, 16> remove_tiles;
On 2015/05/28 05:07:25, reveman wrote:
> why 16? can we remove this magic value or add a comment that explains why 16
> makes sense here?

Done.

https://codereview.chromium.org/1139063002/diff/790001/cc/tiles/picture_layer...
File cc/tiles/picture_layer_tiling.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/tiles/picture_layer...
cc/tiles/picture_layer_tiling.h:281: bool RemoveTileAt(int i, int j, Tile::Id*
id);
On 2015/05/28 05:07:25, reveman wrote:
> adding an 'id' output param seems a bit too specific for the exact usage of
this
> patch. I think it would be nicer to add an alternative TakeTileAt function
that
> allows you to get the id when needed and the existing RemoveTileAt function
can
> simply be implemented on top of it.

I think we're making perfect the enemy of the good.

Done. I didn't make RemoveTileAt use TakeTileAt. That would just be more pointer
moving around for no use.

[danakj, 2015-05-28 21:29:05]

I got rid of the TileTaskData.

[reveman, 2015-05-29 14:50:27]

https://codereview.chromium.org/1139063002/diff/790001/cc/playback/raster_sou...
File cc/playback/raster_source.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/playback/raster_sou...
cc/playback/raster_source.h:45: const gfx::Rect& canvas_playback_rect,
On 2015/05/28 18:54:46, danakj wrote:
> On 2015/05/28 05:07:24, reveman wrote:
> > Instead of adding another rect here, can we use SkCanvas::getBaseLayerSize()
> to
> > determine if playback_rect represents the whole canvas and optimized clear
> > functions can be used?
> 
> I don't like this, it really limits what the caller can do with the SkCanvas.
It
> could have a larger canvas (a 256x256 canvas?) for a smaller tile (an edge
> tile?).

I'm sure you're right. It's just hard for me to see why canvas+rect is not
enough to satisfy both these use cases and canvas+rect+subrect is necessary. I
thought this was what PlaybackToSharedCanvas is already doing (playback into a
larger canvas that already contains some content) and I would have expected the
SharedCanvas version to be used for partial updates based on the description of
that function. Is the additional rect just so we can compute "bool
partial_update = canvas_bitmap_rect != canvas_playback_rect;" in
::PrepareForPlaybackToCanvas or also something else that I'm failing to see?

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:385: gfx::Rect(0, y,
raster_resource->size().width(), rows_to_copy))));
On 2015/05/28 at 18:54:46, danakj wrote:
> On 2015/05/28 05:07:24, reveman wrote:
> > hard to see if this code was changed. looks like it's only renaming. would
be
> > nice to avoid that unless it's important somehow?
> 
> this is just renaming, to make it clear what resource we're rastering vs
uploading to. They are still src/dst inside CopyOperation (it's a src->dst copy)
but over here we're rastering some resource and rastering to the "src" was super
unclear. This helped me get my head around what was going on in this class, so
I'd like to keep it?

OK, I see, just some renaming that makes sense independent of partial updates..

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1139063002/diff/790001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:39: virtual void
ReleaseBufferForRaster(scoped_ptr<RasterBuffer> buffer) = 0;
On 2015/05/28 at 18:54:47, danakj wrote:
> On 2015/05/28 05:07:25, reveman wrote:
> > Discussed this with Vlad and I think there are some minor changes that can
be
> > made to make this API easier to understand from the tile manager's point of
> > view. What if the interface and the tile manager usage looked like this:
> > 
> > // Acquires a buffer for raster to |resource|. The buffer should preferably
> > // be populated with contents matching |preferred_content_id|. The actual
> > // contents of the buffer is returned in |actual_content_id|.
> > virtual scoped_ptr<RasterBuffer> AcquireBufferForRaster(
> >     const Resource* resource,
> >     uint64_t preferred_content_id,
> >     uint64_t* actual_content_id);
> > 
> > // Releases a previously acquired buffer. |content_id| uniquely identifies
the
> > // contents of the buffer.
> > virtual void ReleaseBufferForRaster(scoped_ptr<RasterBuffer> buffer,
> >                                     uint64_t content_id);
> > 
> > 
> > RasterBufferImpl in tile manager would do:
> > 
> > void ScheduleOnOriginThread(TileTaskClient* client) override {
> >   raster_buffer_ = client->AcquireBufferForRaster(
> >       resource(), previous_content_id_, &raster_buffer_content_id_);
> > }
> > void Raster(const RasterSource* raster_source) {
> >   gfx::Rect playback_rect =
> >       raster_buffer_content_id_ == previous_content_id_ ?
> >       dirty_content_rect_ : content_rect_;
> 
> This doesn't work. Tile manager doesn't know if the ResourcePool has a
resource with the old content ID. The worker pool needs both rects and needs to
decide which one to raster based on if it finds the old content ID resource or
not.

The whole point of the interface suggestion above would be to let the tile
manager code know what the content of the raster buffer is so it can make the
right decision for what needs to be done in terms of raster.

> 
> >   raster_buffer_->Playback(raster_source_.get(),
> >                            playback_rect,
> >                            contents_scale_);
> > }
> > void CompleteOnOriginThread(TileTaskClient* client) override {
> >   client->ReleaseBufferForRaster(raster_buffer_.Pass(), new_content_id_);
> > }
> > 
> > The idea is that the flow would become a bit easier to understand. We
acquire a
> > buffer, decide what part of the raster source to playback based on the
result of
> > that and then release the raster buffer when done while also providing a new
> > content id the represents the result of the raster done to the buffer.
> > 
> > Wdyt? 
> 
> I think that this doesn't really work and we should figure out what bitmap and
gpu raster partial raster works before trying to rewrite APIs.

Ok, let's discuss that today if you have time.

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:44: DCHECK_EQ(resource_format,
raster_resource_->format());
what if raster_resource_ == nullptr?

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:71:
resource_pool_->ReleaseResource(raster_resource_.Pass(), id);
should we keep the old content id if raster buffer was not modified?

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:386: // |write_lock| is passed to
the first copy operation as it needs to be
|raster_resource_write_lock|

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.h (right):

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.h:63: // |data|'s resource. Returns a
non-zero sequence number for this copy
nit: update comment, |data| -> X

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.h:70: const Resource* upload_resource,
nit: kill 'upload_' prefix. 'resource' alone is good imo or maybe dst_resource
or output_resource if you don't like that.

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.h:104:
scoped_ptr<ResourceProvider::ScopedWriteLockGpuMemoryBuffer> src_write_lock;
nit: raster_resource_write_lock like the ctor?

https://codereview.chromium.org/1139063002/diff/890001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1139063002/diff/890001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:598:
resource_pool_->ReleaseResource(draw_info.resource_.Pass(), tile->id());
just to make sure I understand the code. we could pass 0 instead tile->id() and
one-copy partial raster would still work, right?

[danakj, 2015-05-29 17:16:07]

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:44: DCHECK_EQ(resource_format,
raster_resource_->format());
On 2015/05/29 14:50:26, reveman wrote:
> what if raster_resource_ == nullptr?

Done.

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:71:
resource_pool_->ReleaseResource(raster_resource_.Pass(), id);
On 2015/05/29 14:50:26, reveman wrote:
> should we keep the old content id if raster buffer was not modified?

we could but it'll never be used. the tile id will be the new id now.

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:386: // |write_lock| is passed to
the first copy operation as it needs to be
On 2015/05/29 14:50:27, reveman wrote:
> |raster_resource_write_lock|

Done.

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.h (right):

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.h:63: // |data|'s resource. Returns a
non-zero sequence number for this copy
On 2015/05/29 14:50:27, reveman wrote:
> nit: update comment, |data| -> X

Done.

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.h:70: const Resource* upload_resource,
On 2015/05/29 14:50:27, reveman wrote:
> nit: kill 'upload_' prefix. 'resource' alone is good imo or maybe dst_resource
> or output_resource if you don't like that.

ok output_resource everywhere.

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.h:104:
scoped_ptr<ResourceProvider::ScopedWriteLockGpuMemoryBuffer> src_write_lock;
On 2015/05/29 14:50:27, reveman wrote:
> nit: raster_resource_write_lock like the ctor?

meant to leave CopyOperation as src/dst, fixed the ctor.

https://codereview.chromium.org/1139063002/diff/890001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1139063002/diff/890001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:598:
resource_pool_->ReleaseResource(draw_info.resource_.Pass(), tile->id());
On 2015/05/29 14:50:27, reveman wrote:
> just to make sure I understand the code. we could pass 0 instead tile->id()
and
> one-copy partial raster would still work, right?

yes. this is a different pool.

[reveman, 2015-05-29 18:42:43]

lgtm

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:71:
resource_pool_->ReleaseResource(raster_resource_.Pass(), id);
On 2015/05/29 at 17:16:06, danakj wrote:
> On 2015/05/29 14:50:26, reveman wrote:
> > should we keep the old content id if raster buffer was not modified?
> 
> we could but it'll never be used. the tile id will be the new id now.

I'm thinking of cases where tasks are canceled and re-scheduled. I can see this
happening if the user scrolls the page and some content is animating. If I
understand this correctly, partial updates won't work in that case unless you
kept the old content id. Maybe that's fine?

[danakj, 2015-05-29 18:59:48]

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1139063002/diff/890001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:71:
resource_pool_->ReleaseResource(raster_resource_.Pass(), id);
On 2015/05/29 18:42:43, reveman wrote:
> On 2015/05/29 at 17:16:06, danakj wrote:
> > On 2015/05/29 14:50:26, reveman wrote:
> > > should we keep the old content id if raster buffer was not modified?
> > 
> > we could but it'll never be used. the tile id will be the new id now.
> 
> I'm thinking of cases where tasks are canceled and re-scheduled. I can see
this
> happening if the user scrolls the page and some content is animating. If I
> understand this correctly, partial updates won't work in that case unless you
> kept the old content id. Maybe that's fine?

Ya, but well, it won't hurt. I'll make it use the old id. We'll partial raster
whenever possible then :)

[danakj, 2015-05-29 19:02:49]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-05-29 19:02:50]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
vmpstr@chromium.org, reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1139063002/#ps930001
(title: "monocle: previousid")

[commit-bot: I haz the power, 2015-05-29 19:04:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139063002/930001

[danakj, 2015-05-29 19:50:04]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2015-05-29 19:51:03]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-05-29 19:51:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, vmpstr@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1139063002/#ps950001
(title: "monocle: .")

[commit-bot: I haz the power, 2015-05-29 19:52:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139063002/950001

[danakj, 2015-05-29 20:55:28]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-05-29 20:55:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, vmpstr@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1139063002/#ps970001
(title: "monocle: rebase")

[commit-bot: I haz the power, 2015-05-29 20:56:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139063002/970001

[commit-bot: I haz the power, 2015-05-29 22:48:04]

Committed patchset #49 (id:970001)

[commit-bot: I haz the power, 2015-05-29 22:48:46]

Patchset 49 (id:??) landed as
https://crrev.com/e0b5478e9fd582c904d1968726c437fd365ab3ea
Cr-Commit-Position: refs/heads/master@{#332069}