Issue 1126813002: cc: Pass priority rect information from iterators to tilings.

Issue 1126813002: cc: Pass priority rect information from iterators to tilings. (Closed)

Created:
5 years, 7 months ago by USE eero AT chromium.org

Modified:
5 years, 7 months ago

Reviewers:
hendrikw, vmpstr, enne (OOO)

CC:
chromium-reviews, cc-bugs_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

cc: Pass priority rect information from iterators to tilings. As each tiling iterator in raster and eviction queues iterates one priority rect, those iterators know the priority rect for tiles. Thus picture layer tilings compute priority rects only because iterators do not pass that information to tilings. Therefore, unnecessary priority rect computation can be avoided by passing priority rect information from iterators to tilings. Committed: https://crrev.com/1b9c87ec0f730691b2bcfd1023eb8970535495d3 Cr-Commit-Position: refs/heads/master@{#330061}

Patch Set 1 #

Total comments: 18

Patch Set 2 : Clean up, naming, etc. #

Patch Set 3 : Rebase #

Total comments: 6

Patch Set 4 : Rebase #

Patch Set 5 : Rebase fix #

Created: 5 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+167 lines, -92 lines)			Patch
M	cc/resources/picture_layer_tiling.h	View	1 2 3	2 chunks	+20 lines, -2 lines	0 comments	Download
M	cc/resources/picture_layer_tiling.cc	View	1 2 3 4	3 chunks	+55 lines, -22 lines	0 comments	Download
M	cc/resources/tiling_set_eviction_queue.h	View	1 2	2 chunks	+5 lines, -4 lines	0 comments	Download
M	cc/resources/tiling_set_eviction_queue.cc	View	1 2	7 chunks	+13 lines, -15 lines	0 comments	Download
M	cc/resources/tiling_set_raster_queue_all.h	View	1 2 3	4 chunks	+11 lines, -19 lines	0 comments	Download
M	cc/resources/tiling_set_raster_queue_all.cc	View	1 2	20 chunks	+59 lines, -28 lines	0 comments	Download
M	cc/resources/tiling_set_raster_queue_required.cc	View	1 2	2 chunks	+4 lines, -2 lines	0 comments	Download

Messages

Total messages: 17 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

USE eero AT chromium.org

PTAL. This removes most unneeded gfx::Rect::Intersects calls from tile priority update paths.

5 years, 7 months ago (2015-05-05 16:18:26 UTC) #2

vmpstr

This is pretty cool! https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_tiling.cc File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_tiling.cc#newcode867 cc/resources/picture_layer_tiling.cc:867: PictureLayerTiling::PriorityRect PictureLayerTiling::ComputePriorityRectForTile( Is this function ...

5 years, 7 months ago (2015-05-05 18:04:02 UTC) #3

USE eero AT chromium.org

https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_tiling.cc File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_tiling.cc#newcode867 cc/resources/picture_layer_tiling.cc:867: PictureLayerTiling::PriorityRect PictureLayerTiling::ComputePriorityRectForTile( On 2015/05/05 18:04:01, vmpstr wrote: > Is ...

5 years, 7 months ago (2015-05-06 12:05:21 UTC) #4

https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_...
cc/resources/picture_layer_tiling.cc:867: PictureLayerTiling::PriorityRect
PictureLayerTiling::ComputePriorityRectForTile(
On 2015/05/05 18:04:01, vmpstr wrote:
> Is this function only used in DCHECKs and tracing?

It is used also by TilingSetRasterQueueRequired.

If raster tile priority queue is REQUIRED_FOR_ACTIVATION, TSRQR iterates over
pending visible rect tiles without iterating over visible rect tiles first.
Therefore, some tiles reached while iterating over pending visible rect belongs
to VISIBLE_RECT (instead of PENDING_VISIBLE_RECT). TSRQR uses this function to
differentiate between VISIBLE_RECT and PENDING_VISIBLE_RECT tiles.

https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_...
cc/resources/picture_layer_tiling.h:221: enum PriorityRect {
On 2015/05/05 18:04:01, vmpstr wrote:
> nit: PriorityRectType

Done.

https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_...
cc/resources/picture_layer_tiling.h:291: PriorityRect
ComputePriorityRectForTile(const Tile* tile) const;
On 2015/05/05 18:04:01, vmpstr wrote:
> nit: ComputePriorityRectTypeForTile

Done.

https://codereview.chromium.org/1126813002/diff/1/cc/resources/tiling_set_evi...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/1126813002/diff/1/cc/resources/tiling_set_evi...
cc/resources/tiling_set_eviction_queue.cc:232: if (priority_rect_ >
PictureLayerTiling::PriorityRect::PENDING_VISIBLE_RECT &&
On 2015/05/05 18:04:02, vmpstr wrote:
> Can you leave a comment here, just something explaining that we need to skip
> this rect for all priorities after we process the pending visible rect. 

Done.

https://codereview.chromium.org/1126813002/diff/1/cc/resources/tiling_set_evi...
cc/resources/tiling_set_eviction_queue.cc:299:
PictureLayerTiling::PriorityRect::SOON_BORDER_RECT) {
On 2015/05/05 18:04:01, vmpstr wrote:
> The PriorityRect:: part is not needed here, unless you wanna make it an enum
> class

I do not want to make it an enum. But I thought that I have to in order to be
able to use PriorityRectType as a TilingSetRasterQueueAll TilingIterator phase.
Apparently I do not have to, but I forget to remove PriorityRect:: part. Sorry
for that.

Thus, removed.

https://codereview.chromium.org/1126813002/diff/1/cc/resources/tiling_set_ras...
File cc/resources/tiling_set_raster_queue_all.cc (left):

https://codereview.chromium.org/1126813002/diff/1/cc/resources/tiling_set_ras...
cc/resources/tiling_set_raster_queue_all.cc:319: if
(tile_->content_rect().Intersects(pending_visible_rect_))
On 2015/05/05 18:04:02, vmpstr wrote:
> I see that this is less code, but can you maybe do this as a separate patch?

Yes, of course.

> I don't think your main change here to not intersect in priority rects depends
> on this, right? That is, we still have this intersect check in GetFirstTile...
> function

My main change does not depend on removing these code blocks which lie after
GetFirstTileAndCheckIfValid and AdvanceToNextTile calls. However, my main change
does depend on adding the skipping logic to GetFirstTileAndCheckIfValid.

Previously, tile priories for pending visible tiles which were not visible tiles
were calculated twice. First as pending visible tiles and then as
skewport/soon-border/eventually tiles.

But now I want DCHECK in PictureLayerTiling::ComputePriorityForTile that the
function is called with a correct priority rect type (see
https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_...).
For that DCHECK to pass, the tile priorities must be computed only once, thus
GetFirstTileAndCheckIfValid must skip pending visible tiles before updating tile
priorities.

You are correct that these code removals are not mandatory. So I left them
intact and added only TODO comments. Let's clean up them later.

https://codereview.chromium.org/1126813002/diff/1/cc/resources/tiling_set_ras...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1126813002/diff/1/cc/resources/tiling_set_ras...
cc/resources/tiling_set_raster_queue_all.cc:168: pending_twin_tiling_(
On 2015/05/05 18:04:02, vmpstr wrote:
> Why are you adding this here? Why is getting tiling->pending_visible_rect not
> sufficient? It should return the same value that you need if this is an active
> tree, and it would return empty rect if this is the pending tree.

As a premature (size) optimisation. I removed it.

https://codereview.chromium.org/1126813002/diff/1/cc/resources/tiling_set_ras...
File cc/resources/tiling_set_raster_queue_all.h (right):

https://codereview.chromium.org/1126813002/diff/1/cc/resources/tiling_set_ras...
cc/resources/tiling_set_raster_queue_all.h:151: // PENDING VISIBLE RECT refers
to the visible rect that will become current
On 2015/05/05 18:04:02, vmpstr wrote:
> Can you move this comment to where the enum is actually defined now?

Done.

https://codereview.chromium.org/1126813002/diff/1/cc/resources/tiling_set_ras...
cc/resources/tiling_set_raster_queue_all.h:156: typedef
PictureLayerTiling::PriorityRect Phase;
On 2015/05/05 18:04:02, vmpstr wrote:
> nit: using Phase = PictureLayerTiling::PriorityRect;

I knew that such a statement exists but I could not find it while looking for
using declaration syntax (because it is not a using declaration but a type
alias). Thanks.

vmpstr

+hendrikw. This looks like fine to me, but it might conflict with some stuff you're ...

5 years, 7 months ago (2015-05-11 16:19:29 UTC) #6

hendrikw

On 2015/05/11 16:19:29, vmpstr wrote: > +hendrikw. This looks like fine to me, but it ...

5 years, 7 months ago (2015-05-11 21:22:08 UTC) #7

USE eero AT chromium.org

On 2015/05/11 21:22:08, hendrikw wrote: > e_hakkinen, would you mind rebasing on top of that? ...

5 years, 7 months ago (2015-05-12 11:29:54 UTC) #8

hendrikw

Can you give me an estimate of how many intersects you will save per tile? ...

5 years, 7 months ago (2015-05-12 22:23:49 UTC) #9

USE eero AT chromium.org

On 2015/05/12 22:23:49, hendrikw wrote: > Can you give me an estimate of how many ...

5 years, 7 months ago (2015-05-13 07:48:01 UTC) #10

On 2015/05/12 22:23:49, hendrikw wrote:
> Can you give me an estimate of how many intersects you will save per tile? 

For visible rect tiles, this saves 1 intersection (out of 1, i.e. 100 %) per
tile.
For pending visible tiles in skewport rect, this saves 6 intersections (out of
7, i.e. 86 %) per tile.
For pending visible tiles in soon border rect, this saves 5 intersections (out
of 6, i.e. 83 %) per tile.
For pending visible tiles in eventually rect, this save 6 intersections (out of
7, i.e. 86 %) per tile.
For non-pending-visible skewport rect tiles, this saves 4 intersections (out of
5, i.e. 80 %) per tile.
For non-pending-visible soon border rect tiles, this saves 3 intersections (out
of 4, i.e. 75 %) per tile.
For non-pending-visible eventually rect tiles, this save 4 intersections (out of
5, i.e. 80 %) per tile.

The number of intersections are doubled for tiles which are returned by both a
raster and an eviction tiling iterator. That does not happen for many tiles,
though.

The intersections are:
 * intersection with pending visible rect in iterators
   (only for skewport, soon border and eventually rect tiles, not removed)
 * intersection with visible rect in PLT (for all tiles, removed)
 * intersection pending visible rect in PLT
   (for pending visible, skewport, soon border and eventually rect tiles,
removed)
 * intersection with soon border rect in PLT
   (for skewport, soon border and eventually rect tiles, removed)
 * intersection with skewport rect in PLT
   (for skewport and eventually rect tiles, removed)

Additionally, tile priorities pending visible rect tiles are computed only then
needed (then returned as pending visible rect tiles but not then not returned as
skewport, soon border or eventually tiles as it does not make sense to compute
tile priorities for tiles which are not returned).

> You're passing more stuff around, which increases the code complexity a bit,
is
> this something that you would expect to see show up on telemetry?

It should be visible. Though, I do not know how many repeats are needed so that
variation becomes smaller than the difference caused by this patch.

USE eero AT chromium.org

https://codereview.chromium.org/1126813002/diff/40001/cc/resources/picture_layer_tiling.cc File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/1126813002/diff/40001/cc/resources/picture_layer_tiling.cc#newcode862 cc/resources/picture_layer_tiling.cc:862: return TilePriority(resolution_, priority_bin, 0); On 2015/05/12 22:23:49, hendrikw wrote: ...

5 years, 7 months ago (2015-05-13 07:48:14 UTC) #11

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

https://codereview.chromium.org/1126813002/diff/40001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:862: return TilePriority(resolution_,
priority_bin, 0);
On 2015/05/12 22:23:49, hendrikw wrote:
> Why don't we need to compare max_tile_priority_bind and TP::NOW here like we
did
> before?

Because max_tile_priority cannot be higher than TP::NOW (i.e. negative).

In the case when max_tile_priority_bin is TP::NOW,
then we used to return TilePriority(resolution_, TilePriority::NOW, 0).
Now we return TilePriority(resolution_, priority_bin, 0) where priority_bin is
TP::NOW.
Thus we return the same as before.

In the case when max_tile_priority_bin is TP::EVENTUALLY,
then we used to compute distance_to_visible and return TilePriority(resolution_,
TilePriority::EVENTUALLY, distance_to_visible) where distance_to_visible is
always 0 (for visible rect tiles).
Now re return TilePriority(resolution_, priority_bin, 0) where priority_bin is
TP::EVENTUALLY.
Thus we return the same as before.

> Also, we used to pass NOW to the returned TilePriority (I just want to
> make sure that if we're changing logic, we're doing it on purpose)

I deliberately did not change the logic as part of this patch.

https://codereview.chromium.org/1126813002/diff/40001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1126813002/diff/40001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_all.cc:340: if
(current_tile_.tile()->content_rect().Intersects(pending_visible_rect_))
On 2015/05/12 22:23:49, hendrikw wrote:
> If we know this is not needed, why aren't we removing them now?

Because vmpstr asked to do that as a separate patch.

On 2015/05/05 18:04:02, vmpstr wrote:
> I see that this is less code, but can you maybe do this as a separate patch?
I'd
> rather be a bit more explicit about checking pending visible rect in each
> iterator for now.

https://codereview.chromium.org/1126813002/diff/40001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_all.cc:350: while (!done()) {
On 2015/05/12 22:23:49, hendrikw wrote:
> same

Ditto.

hendrikw

On 2015/05/13 07:48:14, e_hakkinen wrote: > https://codereview.chromium.org/1126813002/diff/40001/cc/resources/picture_layer_tiling.cc > File cc/resources/picture_layer_tiling.cc (right): > > https://codereview.chromium.org/1126813002/diff/40001/cc/resources/picture_layer_tiling.cc#newcode862 > ...

5 years, 7 months ago (2015-05-13 17:48:22 UTC) #12

On 2015/05/13 07:48:14, e_hakkinen wrote:
>
https://codereview.chromium.org/1126813002/diff/40001/cc/resources/picture_la...
> File cc/resources/picture_layer_tiling.cc (right):
> 
>
https://codereview.chromium.org/1126813002/diff/40001/cc/resources/picture_la...
> cc/resources/picture_layer_tiling.cc:862: return TilePriority(resolution_,
> priority_bin, 0);
> On 2015/05/12 22:23:49, hendrikw wrote:
> > Why don't we need to compare max_tile_priority_bind and TP::NOW here like we
> did
> > before?
> 
> Because max_tile_priority cannot be higher than TP::NOW (i.e. negative).
> 
> In the case when max_tile_priority_bin is TP::NOW,
> then we used to return TilePriority(resolution_, TilePriority::NOW, 0).
> Now we return TilePriority(resolution_, priority_bin, 0) where priority_bin is
> TP::NOW.
> Thus we return the same as before.
> 
> In the case when max_tile_priority_bin is TP::EVENTUALLY,
> then we used to compute distance_to_visible and return
TilePriority(resolution_,
> TilePriority::EVENTUALLY, distance_to_visible) where distance_to_visible is
> always 0 (for visible rect tiles).
> Now re return TilePriority(resolution_, priority_bin, 0) where priority_bin is
> TP::EVENTUALLY.
> Thus we return the same as before.
> 
> > Also, we used to pass NOW to the returned TilePriority (I just want to
> > make sure that if we're changing logic, we're doing it on purpose)
> 
> I deliberately did not change the logic as part of this patch.
> 
>
https://codereview.chromium.org/1126813002/diff/40001/cc/resources/tiling_set...
> File cc/resources/tiling_set_raster_queue_all.cc (right):
> 
>
https://codereview.chromium.org/1126813002/diff/40001/cc/resources/tiling_set...
> cc/resources/tiling_set_raster_queue_all.cc:340: if
> (current_tile_.tile()->content_rect().Intersects(pending_visible_rect_))
> On 2015/05/12 22:23:49, hendrikw wrote:
> > If we know this is not needed, why aren't we removing them now?
> 
> Because vmpstr asked to do that as a separate patch.
> 
> On 2015/05/05 18:04:02, vmpstr wrote:
> > I see that this is less code, but can you maybe do this as a separate patch?
> I'd
> > rather be a bit more explicit about checking pending visible rect in each
> > iterator for now.
> 
>
https://codereview.chromium.org/1126813002/diff/40001/cc/resources/tiling_set...
> cc/resources/tiling_set_raster_queue_all.cc:350: while (!done()) {
> On 2015/05/12 22:23:49, hendrikw wrote:
> > same
> 
> Ditto.

LGTM

commit-bot: I haz the power

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

5 years, 7 months ago (2015-05-15 07:12:19 UTC) #15

commit-bot: I haz the power

5 years, 7 months ago (2015-05-15 08:08:32 UTC) #17

Message was sent while issue was closed.

Patchset 5 (id:??) landed as
https://crrev.com/1b9c87ec0f730691b2bcfd1023eb8970535495d3
Cr-Commit-Position: refs/heads/master@{#330061}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages