Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
541843002:
cc: Optimise shared raster tile handling in raster tile priority queue. (Closed)
|
Created:
6 years, 3 months ago by USE eero AT chromium.org Modified:
6 years, 3 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptioncc: Optimise shared raster tile handling in raster tile priority queue.
Avoid keeping track of returned tiles. Instead of that, determine the
tree which should return shared tiles and use that information to skip
shared tiles in the other tree.
Committed: https://crrev.com/fd6ef28080b3761392625e3e39432232cb60ef1c
Cr-Commit-Position: refs/heads/master@{#294795}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Change indentation, variable name. Add comments. #Patch Set 3 : Restored dropped final break. #
Total comments: 12
Patch Set 4 : DCHECK cleanups #
Total comments: 2
Patch Set 5 : Split a helper function, fix a special case. #Patch Set 6 : Stop special casing the same tile case. #
Total comments: 13
Patch Set 7 : Split common cases into common function/constants #
Total comments: 9
Patch Set 8 : Less code duplication #
Messages
Total messages: 28 (2 generated)
e.hakkinen@samsung.com changed reviewers: + reveman@chromium.org, vmpstr@chromium.org
PTAL. This replaces the use of the returned_shared_tiles vector with tree and priority comparison. This improves related cc_perftest results: * manage_tiles results by 0.4..2.4 % * tile_manager_raster_tile_queue_construct_and_iterate results by 2.4..20.8 % These measurements were done with the patch set 18 from the issue https://codereview.chromium.org/367833003/ I also tried to do shared tile recognition in cc::PictureLayerTiling::TilingRasterTileIterator (as seemed more natural to me not to return shared tiles twice in the first place). That did not work (it made the performance worse). I suspect that it made tiling raster tile iterator constructor to be too heavy. returned_shared_tiles related DCHECKs should probably be removed and replaced with unit tests, or what do you think?
Thanks for doing this. Can you post raw numbers of performance improvement? I'm curious which cases see more improvement https://codereview.chromium.org/541843002/diff/1/cc/resources/raster_tile_pri... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/1/cc/resources/raster_tile_pri... cc/resources/raster_tile_priority_queue.cc:170: if (has_both_layers) { nit: do if (!has_both_layers) break; Tile* tile = **next_iterator; if (!tile->is_shared()) break; So that it's not as indented. Also make a comment above the first if saying something along the lines of "We have to do an explicit check that we have both layers, since tile shared could mean that it is shared with a recycled tree" https://codereview.chromium.org/541843002/diff/1/cc/resources/raster_tile_pri... cc/resources/raster_tile_priority_queue.cc:193: WhichTree main_tree = nit: Name this something like "higher_priority_tree" or something else. It's hard to understand what main_tree means in the context.
https://codereview.chromium.org/541843002/diff/1/cc/resources/raster_tile_pri... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/1/cc/resources/raster_tile_pri... cc/resources/raster_tile_priority_queue.cc:170: if (has_both_layers) { On 2014/09/04 18:26:01, vmpstr wrote: > nit: do > > if (!has_both_layers) > break; > > Tile* tile = **next_iterator; > if (!tile->is_shared()) > break; > > So that it's not as indented. Also make a comment above the first if saying > something along the lines of "We have to do an explicit check that we have both > layers, since tile shared could mean that it is shared with a recycled tree" Done. In addition, it is possible that a shared tile is shared between an active and a pending layer and either or them does not have valid tile priorities. https://codereview.chromium.org/541843002/diff/1/cc/resources/raster_tile_pri... cc/resources/raster_tile_priority_queue.cc:193: WhichTree main_tree = On 2014/09/04 18:26:01, vmpstr wrote: > nit: Name this something like "higher_priority_tree" or something else. It's > hard to understand what main_tree means in the context. Done.
On 2014/09/04 18:26:01, vmpstr wrote:
> Thanks for doing this. Can you post raw numbers of performance improvement?
I'm
> curious which cases see more improvement
Here you are. I think these are relevant.
I367833003 I367833003 + I541843002 Median improvement
manage_tiles: 2_100= runs/s 26007
26738 26676 26561 27128 27013 1,26%
manage_tiles: 2_500= runs/s 20709
21382 21265 21124 21616 21658 1,65%
manage_tiles: 2_1000= runs/s 20953
21602 21347 21443 21999 21848 2,35%
manage_tiles: 10_100= runs/s 22141
22620 22746 22456 23003 22975 1,57%
manage_tiles: 10_500= runs/s 17728
18287 18342 18315 18615 18436 0,82%
manage_tiles: 10_1000= runs/s 18279
18555 18741 18770 18947 19106 2,11%
manage_tiles: 50_100= runs/s 10207
10249 10371 10366 10343 10383 1,14%
manage_tiles: 50_500= runs/s 9088
9152 9141 9161 9250 9236 1,04%
manage_tiles: 50_1000= runs/s 9194
9302 9314 9341 9333 9455 0,42%
tile_manager_raster_tile_queue_construct: 2= runs/s 1002800
1000701 1012040 985379 1015779 1019792 1,29%
tile_manager_raster_tile_queue_construct: 10= runs/s 271073
273203 270755 273661 272715 275904 0,95%
tile_manager_raster_tile_queue_construct: 50= runs/s 57439
57149 56457 56183 57205 57670 0,10%
tile_manager_raster_tile_queue_construct_and_iterate: 2_16= runs/s 207012
207091 197376 220318 221189 223802 6,85%
tile_manager_raster_tile_queue_construct_and_iterate: 2_32= runs/s 123759
124408 123169 136647 136696 136893 10,45%
tile_manager_raster_tile_queue_construct_and_iterate: 2_64= runs/s 68058
68229 67984 78398 78196 78162 14,90%
tile_manager_raster_tile_queue_construct_and_iterate: 2_128= runs/s 29757
30132 29925 36176 36084 36137 20,76%
tile_manager_raster_tile_queue_construct_and_iterate: 10_16= runs/s 117500
118046 117916 121143 125761 126147 6,65%
tile_manager_raster_tile_queue_construct_and_iterate: 10_32= runs/s 76538
76636 76132 81765 83141 83656 8,63%
tile_manager_raster_tile_queue_construct_and_iterate: 10_64= runs/s 44660
44489 44470 48448 49157 49096 10,36%
tile_manager_raster_tile_queue_construct_and_iterate: 10_128= runs/s 22212
22257 22314 24776 24987 25038 12,26%
tile_manager_raster_tile_queue_construct_and_iterate: 50_16= runs/s 42771
42523 42442 41595 43735 43523 2,35%
tile_manager_raster_tile_queue_construct_and_iterate: 50_32= runs/s 33934
33402 33878 33894 35042 35070 3,44%
tile_manager_raster_tile_queue_construct_and_iterate: 50_64= runs/s 24729
24699 24750 25491 26191 26169 5,82%
tile_manager_raster_tile_queue_construct_and_iterate: 50_128= runs/s 14359
14344 14398 15369 15459 15675 7,66%
On 2014/09/05 08:15:32, e_hakkinen wrote: > On 2014/09/04 18:26:01, vmpstr wrote: > > Thanks for doing this. Can you post raw numbers of performance improvement? > I'm > > curious which cases see more improvement > > Here you are. I think these are relevant. Not very readable. See https://docs.google.com/spreadsheets/d/1tdRRn5mzugIdZeSSd3_ysqS7OyBQsCqDFM0OG...
Update the description please. Instead of WIP put cc. Also I'm not sure I agree with the second part of that comment. If both trees (or even one tree) returns a tile, that should mean that the priority was updated on it. That's true when we start using these iterators, but it should also be true right now. https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.cc:126: has_both_layers(layer_pair.active && layer_pair.pending) { nit: member variables should end with _
On 2014/09/05 16:54:34, vmpstr wrote: > Update the description please. Instead of WIP put cc. Also I'm not sure I agree > with the second part of that comment. If both trees (or even one tree) returns a > tile, that should mean that the priority was updated on it. That's true when we > start using these iterators, but it should also be true right now. > > https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... > File cc/resources/raster_tile_priority_queue.cc (right): > > https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... > cc/resources/raster_tile_priority_queue.cc:126: > has_both_layers(layer_pair.active && layer_pair.pending) { > nit: member variables should end with _ After addressing the comments, this lgtm. Please wait for reveman to review as well before submitting.
Just some DCHECK cleanup suggestions. Looks great otherwise. https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.cc:162: returned_shared_tiles.push_back(**next_iterator); If returned_shared_tiles is a std::set then you could do something like: DCHECK(returned_shared_tiles.insert(**next_iterator)) https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.cc:222: if (!IsEmpty()) { is this check necessary? https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... File cc/resources/raster_tile_priority_queue.h (right): https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.h:35: #if DCHECK_IS_ON It would be better to always compile this so that it would work with --enable-dchecks. Does having an always empty container here impact performance? https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.h:36: // TODO(vmpstr): Investigate removing this. This TODO is not valid anymore, right? How about we update this to a comment that explains what it is now used for? https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.h:37: std::vector<Tile*> returned_shared_tiles; Now that this is only used for debug checks, can we make it a std::set? Makes the intent more clear and the code a bit cleaner, I think.
https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.cc:126: has_both_layers(layer_pair.active && layer_pair.pending) { On 2014/09/05 16:54:34, vmpstr wrote: > nit: member variables should end with _ Disregard this, I forgot that it's a struct with public members
On 2014/09/05 16:54:34, vmpstr wrote: > Also I'm not sure I agree with the second part of that comment. > If both trees (or even one tree) returns a tile, that should mean that > the priority was updated on it. Yes, it is true, that if a layer raster tile iterator returns a tile, the priorities of the tile (i.e. tile->priority(ACTIVE_TREE) and tile->priority(PENDING_TREE)) have been updated. However, that is not so interesting and that is not what the comment is about. It is about whether a layer (not a tile) has valid tile priorities in cc::PictureLayerImpl::HasValidTilePriorities sense. If either an active or a pending layer has valid tile priorities, that layer has a pending or an active twin layer but that twin layer does not have valid tile priorities, cc::LayerTreeHostImpl::GetPictureLayerImplPairs creates a picture layer impl pair which contains only an active or only a pending layer but not both. Then cc::RasterTilePriorityQueue::Build function passes that pair to the constructor of cc::RasterTilePriorityQueue::PairedPictureLayerQueue which when creates only one valid layer raster tile iterator. In that case, it is still possible that a tile->is_shared() between an active and a pending layer. However, because we only have one valid layer raster tile iterator (for the layer which HasValidTilePriorities but not for its twin layer which does not have valid tile priorities), we cannot encounter those shared tiles twice as we do not have that other valid layer raster tile iterator which could yield those shared tiles on the second time. You should be very familiar with all of this as you have implemented most of it. So I think that the content of the comment is correct, but please suggest a better wording if you like.
https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.cc:162: returned_shared_tiles.push_back(**next_iterator); On 2014/09/05 17:08:31, reveman wrote: > If returned_shared_tiles is a std::set then you could do something like: > DCHECK(returned_shared_tiles.insert(**next_iterator)) It needs a .second. Without that it errors with .invalid argument type 'std::pair<iterator, bool>' to unary expression. https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.cc:222: if (!IsEmpty()) { On 2014/09/05 17:08:31, reveman wrote: > is this check necessary? Yes, although it can be put inside DCHECK. If the paired queue is empty, *next_iterator is not valid (its operator bool() returns false) and **next_iterator must not be called. https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... File cc/resources/raster_tile_priority_queue.h (right): https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.h:35: #if DCHECK_IS_ON On 2014/09/05 17:08:32, reveman wrote: > It would be better to always compile this so that it would work with > --enable-dchecks. Done. > Does having an always empty container here impact performance? Not measurable impact. https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.h:36: // TODO(vmpstr): Investigate removing this. On 2014/09/05 17:08:32, reveman wrote: > This TODO is not valid anymore, right? How about we update this to a comment > that explains what it is now used for? Done. https://codereview.chromium.org/541843002/diff/40001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.h:37: std::vector<Tile*> returned_shared_tiles; On 2014/09/05 17:08:32, reveman wrote: > Now that this is only used for debug checks, can we make it a std::set? Yes, consumes more memory than std::vector but compared to the memory used by the two layer raster tile iterators that is nothing. > Makes the intent more clear and the code a bit cleaner, I think. If you say so.
PTAL if there is anything left to do.
https://codereview.chromium.org/541843002/diff/60001/cc/resources/raster_tile... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/60001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.cc:177: Tile* tile = **next_iterator; Consider adding a helper function for the rest of the body of this loop. ie. bool HasTileAlreadyBeenReturned(Tile* tile, WhichTree tree, TreePriority tree_priority) I think that would make the code a bit cleaner as you don't need these multiple levels of break statements.
On 2014/09/06 20:39:44, e_hakkinen_ wrote: > On 2014/09/05 16:54:34, vmpstr wrote: > > Also I'm not sure I agree with the second part of that comment. > > If both trees (or even one tree) returns a tile, that should mean that > > the priority was updated on it. > > Yes, it is true, that if a layer raster tile iterator returns a tile, the > priorities of the tile (i.e. tile->priority(ACTIVE_TREE) and > tile->priority(PENDING_TREE)) have been updated. > > However, that is not so interesting and that is not what the comment is about. > > It is about whether a layer (not a tile) has valid tile priorities in > cc::PictureLayerImpl::HasValidTilePriorities sense. > > If either an active or a pending layer has valid tile priorities, that layer has > a pending or an active twin layer but that twin layer does not have valid tile > priorities, cc::LayerTreeHostImpl::GetPictureLayerImplPairs creates a picture > layer impl pair which contains only an active or only a pending layer but not > both. Then cc::RasterTilePriorityQueue::Build function passes that pair > to the constructor of cc::RasterTilePriorityQueue::PairedPictureLayerQueue which > when creates only one valid layer raster tile iterator. > > In that case, it is still possible that a tile->is_shared() between an active > and a pending layer. However, because we only have one valid layer raster tile > iterator (for the layer which HasValidTilePriorities but not for its twin layer > which does not have valid tile priorities), we cannot encounter those shared > tiles twice as we do not have that other valid layer raster tile iterator which > could yield those shared tiles on the second time. > > You should be very familiar with all of this as you have implemented most of it. > > So I think that the content of the comment is correct, but please suggest a > better wording if you like. Ah, that makes sense. I forgot about the HasValidTilePriorities case. Thanks for the explanation.
I noticed that the logic I made does not handle the special case in which iterators yield the same shared tile at the same time (it is special cased in NextTileIteratorTree). https://codereview.chromium.org/541843002/diff/60001/cc/resources/raster_tile... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/60001/cc/resources/raster_tile... cc/resources/raster_tile_priority_queue.cc:177: Tile* tile = **next_iterator; On 2014/09/08 14:38:42, reveman wrote: > Consider adding a helper function for the rest of the body of this loop. ie. > bool HasTileAlreadyBeenReturned(Tile* tile, WhichTree tree, TreePriority > tree_priority) I am bit hesitated to do that, because the code does not actually test, whether the tile has already been returned but whether is should have been already been returned. The difference is that tiling raster tile iterators may return tiles a bit out of priority order due to the use of spiral iterators. > I think that would make the code a bit cleaner as you don't need these multiple > levels of break statements. True.
PTAL. I split a helper function so that similarities with NextTileIteratorTree are obvious, cleaned up comments and fixed a special case.
https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:165: if (has_both_layers) { doesn't seem to make sense to enter this loop unless this is true. put the whole loop inside a "if (!has_both_layers)" block? https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:177: break; I think it would be less awkward to keep iterating for the base case and instead break out of the loop when you reach a tile that should be returned. https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:225: RasterTilePriorityQueue::PairedPictureLayerQueue::PrimarySharedTileTree( PrimaryTreeForSharedTile? https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:230: // Keep in sync with NextTileIteratorTree. Instead of having to keep these functions in sync, could we add something like a: WhichTree PrimaryTree(TreePriority tree_priority, const Tile* active_tile, const Tile* pending_tile) const; function that can be used in both cases? https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:255: NOTREACHED(); You're just being consistent here and that's good but feel free to remove one of these NOTREACHED statements from here and NextTileIteratorTree if you like as two of them like this doesn't make much sense.
https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:165: if (has_both_layers) { On 2014/09/09 18:26:47, reveman wrote: > doesn't seem to make sense to enter this loop unless this is true. put the whole > loop inside a "if (!has_both_layers)" block? Yes, that could be done. It is just that the DCHECK in the end of the function needs the above next_iterator update thus it must be duplicated if the loop is inside an if block. https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:177: break; On 2014/09/09 18:26:47, reveman wrote: > I think it would be less awkward to keep iterating for the base case and instead > break out of the loop when you reach a tile that should be returned. Done. https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:225: RasterTilePriorityQueue::PairedPictureLayerQueue::PrimarySharedTileTree( On 2014/09/09 18:26:47, reveman wrote: > PrimaryTreeForSharedTile? Done. https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:230: // Keep in sync with NextTileIteratorTree. On 2014/09/09 18:26:47, reveman wrote: > Instead of having to keep these functions in sync, could we add something like > a: > > WhichTree PrimaryTree(TreePriority tree_priority, > const Tile* active_tile, > const Tile* pending_tile) const; > > function that can be used in both cases? The only but is that for the first two cases there is no need to return a tile from iterators. I could check if it has any real impact on performance. If not then do that. If yes then the SAME_PRIORITY_FOR_BOTH_TREES case could put to a helper function. The first two cases are so simple that there is not anything special to keep in sync. https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:255: NOTREACHED(); On 2014/09/09 18:26:47, reveman wrote: > You're just being consistent here and that's good but feel free to remove one of > these NOTREACHED statements from here and NextTileIteratorTree if you like as > two of them like this doesn't make much sense. Done.
https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:165: if (has_both_layers) { On 2014/09/10 07:38:23, e_hakkinen wrote: > It is just that the DCHECK in the end of the function needs the above > next_iterator update. It turned out to be quite easy to work-a-round that problem. So I put the loop inside an if statement instead of putting the if statement inside the loop. https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:230: // Keep in sync with NextTileIteratorTree. On 2014/09/10 07:38:23, e_hakkinen wrote: > On 2014/09/09 18:26:47, reveman wrote: > > Instead of having to keep these functions in sync, could we add something like > > a [...] function that can be used in both cases? > > The only but is that for the first two cases there is no need to return a tile > from iterators. > I could check if it has any real impact on performance. It does have a major impact. Half of the improvements achieved by replacing the vector based shared tile handling is lost, if the whole switch block is put to a common helper function. So cc::PictureLayerImpl::LayerRasterTileIterator::operator* is not very fast and that has an effect on NextTileIteratorTree which is called quite a lot. It is thus good to avoid calling LayerRasterTileIterator::operator* when reasonably possible. > If yes then the SAME_PRIORITY_FOR_BOTH_TREES case could put to a helper > function. So I'll split the switch cases (instead of the whose switch block) into helper functions and constants.
https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/100001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:230: // Keep in sync with NextTileIteratorTree. On 2014/09/10 14:08:26, e_hakkinen wrote: > On 2014/09/10 07:38:23, e_hakkinen wrote: > > On 2014/09/09 18:26:47, reveman wrote: > > > Instead of having to keep these functions in sync, could we add something > like > > > a [...] function that can be used in both cases? > > > > The only but is that for the first two cases there is no need to return a tile > > from iterators. > > I could check if it has any real impact on performance. > > It does have a major impact. Half of the improvements achieved by replacing the > vector based shared tile handling is lost, if the whole switch block is put to a > common helper function. > So cc::PictureLayerImpl::LayerRasterTileIterator::operator* is not very fast and > that has an effect on NextTileIteratorTree which is called quite a lot. It is > thus good to avoid calling LayerRasterTileIterator::operator* when reasonably > possible. I'm not sure I understand the reasoning here. I'm just asking for the code duplication to be reduced. There's no reason that should have to make things slower. If anything, less code should make it faster. > > > If yes then the SAME_PRIORITY_FOR_BOTH_TREES case could put to a helper > > function. > > So I'll split the switch cases (instead of the whose switch block) into helper > functions and constants. https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:71: const Tile* pending_tile) { I don't understand why this can't include the tree_priority switch statement instead of having that duplicated in two places. Are you worried that a function call in NextTileIteratorTree to a function that can easily be inlined by the compiler might have a negative performance impact? https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:193: if (has_both_layers) { Not sure it's worth DCHECKing when this is not the case. It's fine to return early here if !has_both_layers imo. Whatever you prefer. https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:199: next_tree == ACTIVE_TREE ? &active_iterator : &pending_iterator; I thinks it's a bit ugly that we compute these values again here after having done it above. Consider using a do-while loop instead. ie. something like this: do { const Tile* tile = **next_iterator; if (!tile->is_shared()) break; if (PrimaryTreeForSharedTile(tree_priority, tile) == next_tree) break; next_tree = NextTileIteratorTree(tree_priority); next_iterator = next_tree == ACTIVE_TREE ? &active_iterator : &pending_iterator; } while(!IsEmpty());
https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:71: const Tile* pending_tile) { On 2014/09/10 18:53:35, reveman wrote: > I don't understand why this can't include the tree_priority switch statement > instead of having that duplicated in two places. Are you worried that a function > call in NextTileIteratorTree to a function that can easily be inlined by the > compiler might have a negative performance impact? If HigherPriorityTree included the tree_priority switch statement and NextPriorityTree called it unconditionally like in WhichTree RasterTilePriorityQueue::PairedPictureLayerQueue::NextTileIteratorTree( TreePriority tree_priority) const { if (!active_iterator) return PENDING_TREE; if (!pending_iterator) return ACTIVE_TREE; return HigherPriorityTree( tree_priority, *active_iterator, *pending_iterator); } then NextTileIteratorTree would execute *active_iterator and *pending_iterator statements unconditionally and THAT is not fast and is not needed unless tree_priority is SAME_PRIORITY_FOR_BOTH_TREES. https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:191: ++(*next_iterator); Here the next_iterator is advanced, which change the value returned by NextTileIteratorTree. https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:199: next_tree == ACTIVE_TREE ? &active_iterator : &pending_iterator; On 2014/09/10 18:53:35, reveman wrote: > I thinks it's a bit ugly that we compute these values again here after having > done it above. We do not compute the same values again. Instead of that, we compute new values for the new next_iterator. Note that we advanced the previous next_iterator on the line 191, thus the newly computed next_iterator may be different than the previous one. > Consider using a do-while loop instead. ie. something like this: > > do { > const Tile* tile = **next_iterator; This would be incorrect. The next_iterator should not be used after it is advanced (on line 191) unless the next_iterator is computed again. > if (!tile->is_shared()) > break; > if (PrimaryTreeForSharedTile(tree_priority, tile) == next_tree) > break; > > next_tree = NextTileIteratorTree(tree_priority); > next_iterator = > next_tree == ACTIVE_TREE ? &active_iterator : &pending_iterator; > } while(!IsEmpty());
PTAL. Moved tree_priority switch statements into the HigherPriorityTree function. This avoids code duplication. Changed the HigherPriorityTree function to take active and pending iterators or a shared tile instead of active and pending tiles. This avoids iterator reads when they are not needed (when tree_priority is not SAME_PRIORITY_FOR_BOTH_TREES or when the a shared tile is already been read from an iterator).
lgtm https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... File cc/resources/raster_tile_priority_queue.cc (right): https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:71: const Tile* pending_tile) { On 2014/09/11 06:55:24, e_hakkinen wrote: > On 2014/09/10 18:53:35, reveman wrote: > > I don't understand why this can't include the tree_priority switch statement > > instead of having that duplicated in two places. Are you worried that a > function > > call in NextTileIteratorTree to a function that can easily be inlined by the > > compiler might have a negative performance impact? > > If HigherPriorityTree included the tree_priority switch statement and > NextPriorityTree called it unconditionally like in > > WhichTree > RasterTilePriorityQueue::PairedPictureLayerQueue::NextTileIteratorTree( > TreePriority tree_priority) const { > if (!active_iterator) > return PENDING_TREE; > if (!pending_iterator) > return ACTIVE_TREE; > return HigherPriorityTree( > tree_priority, *active_iterator, *pending_iterator); > } > > then NextTileIteratorTree would execute *active_iterator and *pending_iterator > statements unconditionally and THAT is not fast and is not needed unless > tree_priority is SAME_PRIORITY_FOR_BOTH_TREES. Ok, thanks for explaining. Maybe the code in your previous patch is better then. Your call. https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:191: ++(*next_iterator); On 2014/09/11 06:55:24, e_hakkinen wrote: > Here the next_iterator is advanced, which change the value returned by > NextTileIteratorTree. you'd have to move this into the loop. https://codereview.chromium.org/541843002/diff/120001/cc/resources/raster_til... cc/resources/raster_tile_priority_queue.cc:199: next_tree == ACTIVE_TREE ? &active_iterator : &pending_iterator; On 2014/09/11 06:55:25, e_hakkinen wrote: > On 2014/09/10 18:53:35, reveman wrote: > > I thinks it's a bit ugly that we compute these values again here after having > > done it above. > > We do not compute the same values again. > Instead of that, we compute new values for the new next_iterator. > Note that we advanced the previous next_iterator on the line 191, thus the newly > computed next_iterator may be different than the previous one. I see now that your code is consistent with the old code so looks good to me from that pov. For a function that is supposed to do a simple advance-at-least-one-until-reaching-a-valid-value this function looks a bit unconventional but there's no need to improve that in this patch. > > > Consider using a do-while loop instead. ie. something like this: > > > > do { > > const Tile* tile = **next_iterator; > > This would be incorrect. The next_iterator should not be used after it is > advanced (on line 191) unless the next_iterator is computed again. > > > if (!tile->is_shared()) > > break; > > if (PrimaryTreeForSharedTile(tree_priority, tile) == next_tree) > > break; > > > > next_tree = NextTileIteratorTree(tree_priority); > > next_iterator = > > next_tree == ACTIVE_TREE ? &active_iterator : &pending_iterator; > > } while(!IsEmpty());
The CQ bit was checked by e.hakkinen@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/541843002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 9d0bb72416acf5b88ad70528626f955b7d19471f
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/fd6ef28080b3761392625e3e39432232cb60ef1c Cr-Commit-Position: refs/heads/master@{#294795} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
