Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
428533008:
cc: Remove vectors from tiling eviction tile iterator. (Closed)
Descriptioncc: Remove vectors from tiling eviction tile iterator.
This patch removes vectors from the eviction tile
iterator at the layer level. It reworks the code
a bit to use the underlying layer's tilings
directly using indecies and ranges, instead of
constructing a separate vector to hold the
values.
As well, this ensures that the iterators
are only created when they are visited.
R=reveman
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287688
Patch Set 1 #Patch Set 2 : \ #
Total comments: 3
Patch Set 3 : update #
Total comments: 5
Patch Set 4 : update #
Total comments: 38
Patch Set 5 : update #
Total comments: 2
Patch Set 6 : update #
Total comments: 5
Patch Set 7 : update #Patch Set 8 : #
Total comments: 15
Patch Set 9 : update #Patch Set 10 : rebase #Patch Set 11 : #Patch Set 12 : removed unused var #
Messages
Total messages: 35 (0 generated)
reveman@, this is a very rough draft of removing vectors from the layer eviction iterator. I don't know if this is what you meant but I'm not really comfortable with this patch. Even as I was writing it, I kind of had difficulty keeping track about what tiling to go to next... Let me know if I misunderstood what you meant. https://codereview.chromium.org/428533008/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1669: bool PictureLayerImpl::LayerEvictionTileIterator::AdvanceNextTilingIndex() { This function is what I'm uncomfortable about. https://codereview.chromium.org/428533008/diff/20001/cc/resources/picture_lay... File cc/resources/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/428533008/diff/20001/cc/resources/picture_lay... cc/resources/picture_layer_tiling_unittest.cc:1027: #if !1 I'll fix this if this is the approach we want to take.
https://codereview.chromium.org/428533008/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:87: }; Can you describe all the phases that this iterator goes through. Is it each one of these directions for each iteration stage? Btw, why is it called a direction? rather then say a phase, or stage?
PTAL. After discussing this offline, I put in some changes. I think this looks better. It's probably in need of some more testing, but I just wanted to make sure that we're on the same page here. Also, I haven't fixed the perftests yet.
On 2014/07/29 07:31:40, vmpstr wrote: > PTAL. After discussing this offline, I put in some changes. I think this looks > better. It's probably in need of some more testing, but I just wanted to make > sure that we're on the same page here. > > Also, I haven't fixed the perftests yet. Note that it's probably easier to read the patch as a new version rather than an update to the old version.
https://codereview.chromium.org/428533008/diff/40001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/40001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:95: TilingIterationRange ranges_[5]; Some enums representing each stage and each range might help make the code a bit easier to read. https://codereview.chromium.org/428533008/diff/40001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:101: }; If I understand this iterator correctly. This is what it does: it iterates through all tiles, for each tiling of each range of each stage. Seems like what we really have here is a number of iterators baked into one. Would it be possible to separate this into different iterators. I think that might make it easier to understand and reduce the complexity of the code a bit. We'd have a StageIterator and a RangeIterator. Wdyt?
https://codereview.chromium.org/428533008/diff/40001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/40001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:95: TilingIterationRange ranges_[5]; On 2014/07/29 16:50:49, reveman wrote: > Some enums representing each stage and each range might help make the code a bit > easier to read. Ok, I will do this. https://codereview.chromium.org/428533008/diff/40001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:101: }; On 2014/07/29 16:50:49, reveman wrote: > If I understand this iterator correctly. This is what it does: it iterates > through all tiles, for each tiling of each range of each stage. > > Seems like what we really have here is a number of iterators baked into one. > Would it be possible to separate this into different iterators. I think that > might make it easier to understand and reduce the complexity of the code a bit. > We'd have a StageIterator and a RangeIterator. Wdyt? That's kind of what we have with AdvanceTiling/AdvanceRange/AdvanceStage.. Those are the operator++ of the iterators you propose. Are you suggesting making this an explicit separate class? My initial thought is that it's a bit of an overkill, but I can try and see if it makes it better. Let me know if that's what you meant.
PTAL. I ran the perftests on this and the gains are similar to gains on https://codereview.chromium.org/416403007/ (some slightly better, some slightly worse). The tile manager queue construct/iterate went up slightly, probably from the fact that there are no vectors.
https://codereview.chromium.org/428533008/diff/40001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/40001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:101: }; On 2014/07/29 17:08:49, vmpstr wrote: > On 2014/07/29 16:50:49, reveman wrote: > > If I understand this iterator correctly. This is what it does: it iterates > > through all tiles, for each tiling of each range of each stage. > > > > Seems like what we really have here is a number of iterators baked into one. > > Would it be possible to separate this into different iterators. I think that > > might make it easier to understand and reduce the complexity of the code a > bit. > > We'd have a StageIterator and a RangeIterator. Wdyt? > > That's kind of what we have with AdvanceTiling/AdvanceRange/AdvanceStage.. Those > are the operator++ of the iterators you propose. Are you suggesting making this > an explicit separate class? My initial thought is that it's a bit of an > overkill, but I can try and see if it makes it better. Let me know if that's > what you meant. Yea, that's what I was thinking. Maybe the RangeIterator should be a TilingSet::EvictionTileIterator that takes a range.
Only had a chance to look at *_layer_impl.cc/h yet. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1683: } I think I'd like to see this function removed and the check always be as simple as "current_tiling_ < current_tiling_range_.count". https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1694: break; hm, doesn't this need to be a nested loop? ie. we need to keep advancing the range until we find one with tilings and same for advancing the stage.. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1706: if (!CurrentTilingIndexInRange()) Seems odd that we check this both before and after we increment the index. Would setting the current_range_ to { 0, 0 } and current_tiling_ to -1 in the ctor help? https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1712: : -1; I think it would be cleaner to adjust the index based on direction using an utility function with a switch statement right before calling layer_->tilings_->tiling_at(). ie. layer_->tilings_->tiling_at(RangeOffsetToTilingSetIndex(current_tiling_, current_range_)) https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1720: bool wrapped_around = false; I think this function would be a bit cleaner with a switch statement. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1732: bool PictureLayerImpl::LayerEvictionTileIterator::AdvanceStage() { Think this function would also be cleaner with a switch statement. It's hard to understand the different stages we go through as it is now. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1749: return iterator_; !!iterator_? https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:101: int tiling_index_; nit: current_tiling_ btw, might be cleaner to maintain this as an offset in the tiling range rather than offset from first tiling in the tiling set https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:102: TilePriority::PriorityBin iteration_stage_; Maybe add something like this to make it clear that required_for_activation is part of the stage. struct IterationStage { bool required_for_activation; TilePriority::PriorityBin tile_type; }; IterationStage current_stage_; https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:104: int range_index_; nit: current_range_ and "TilingSet::TilingRangeType current_range_" when rebased on my patch.
PTAL https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1683: } On 2014/07/29 21:41:32, reveman wrote: > I think I'd like to see this function removed and the check always be as simple > as "current_tiling_ < current_tiling_range_.count". I'm still a little bit confused about how we would represent reverse ranges. As well, I think accessing the tiling is far more clear if we use "tiling_index_" as the index instead of "tiling_index_ + current_range_.start_index". https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1694: break; On 2014/07/29 21:41:32, reveman wrote: > hm, doesn't this need to be a nested loop? ie. we need to keep advancing the > range until we find one with tilings and same for advancing the stage.. AdvanceTiling will advance to the next tiling if one is available and return false otherwise AdvanceRange will advance to the next non empty range (always) and return false if it looped around to the beginning. False here kind of means that it "failed" to find a non-empty range and had to look through the ranges again. AdvanceStage will advance the stage if one is available and return false otherwise. So if we get below this line, we can get a new iterator for a new spot that we haven't been before. If that iterator doesn't produce any tiles we start again. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1706: if (!CurrentTilingIndexInRange()) On 2014/07/29 21:41:33, reveman wrote: > Seems odd that we check this both before and after we increment the index. Would > setting the current_range_ to { 0, 0 } and current_tiling_ to -1 in the ctor > help? I don't think this was required. I replaced it with a DCHECK. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1712: : -1; On 2014/07/29 21:41:32, reveman wrote: > I think it would be cleaner to adjust the index based on direction using an > utility function with a switch statement right before calling > layer_->tilings_->tiling_at(). ie. > layer_->tilings_->tiling_at(RangeOffsetToTilingSetIndex(current_tiling_, > current_range_)) I'm not sure I understand what you mean. Would RangeOffsetToTilingSetIndex update the index? Is that instead of this function? Can you elaborate a little bit more? https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1720: bool wrapped_around = false; On 2014/07/29 21:41:32, reveman wrote: > I think this function would be a bit cleaner with a switch statement. switch (range_index_) { case HIGHER_THAN_HIGH_RES: range_index_ = LOWER_THAN_LOW_RES; break; case LOWER_THAN_LOW_RES: range_index_ = BETWEEN_HIGH_AND_LOW_RES; break; ... } Is that what you mean? I changed it to that since I agree that we shouldn't rely on the enum ordering. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1732: bool PictureLayerImpl::LayerEvictionTileIterator::AdvanceStage() { On 2014/07/29 21:41:33, reveman wrote: > Think this function would also be cleaner with a switch statement. It's hard to > understand the different stages we go through as it is now. switch over bins? That would end up looking like this: switch (bin) { case EVENTUALLY: if (!required_for_activation) { required_for_activation = true; } else { bin = SOON; required_for_activation = false; } break; case SOON: if (!required_for_activation) { required_for_activation = true; } else { bin = NOW; required_for_activation = false; } break; if (!required_for_activation) { required_for_activation = true; } else { return false; } break; } return true; To me, that's less clear. It's basically repeating exactly the same thing I have except iteration_stage_ = ... line is more explicit (and it's not immediately obvious that it's the only difference between each case). Would it be better to put a comment over the line saying that it goes through bins in EVENTUALLY, SOON, NOW order? Alternatively, I can put a comment right at the start of the function enumerating the order of all the stages. Wdyt? https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1749: return iterator_; On 2014/07/29 21:41:32, reveman wrote: > !!iterator_? Done. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:101: int tiling_index_; On 2014/07/29 21:41:33, reveman wrote: > nit: current_tiling_ > > btw, might be cleaner to maintain this as an offset in the tiling range rather > than offset from first tiling in the tiling set I changed it to current_tiling_index_. I would prefer to keep the word "index" in there, so it's clear that this is not an actual tiling object/pointer. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:102: TilePriority::PriorityBin iteration_stage_; On 2014/07/29 21:41:33, reveman wrote: > Maybe add something like this to make it clear that required_for_activation is > part of the stage. > > struct IterationStage { > bool required_for_activation; > TilePriority::PriorityBin tile_type; > }; > > IterationStage current_stage_; Done. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:104: int range_index_; On 2014/07/29 21:41:33, reveman wrote: > nit: current_range_ > > and "TilingSet::TilingRangeType current_range_" when rebased on my patch. Likewise I changed it to current_range_type_
https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1683: } On 2014/07/30 00:18:51, vmpstr wrote: > On 2014/07/29 21:41:32, reveman wrote: > > I think I'd like to see this function removed and the check always be as > simple > > as "current_tiling_ < current_tiling_range_.count". > > I'm still a little bit confused about how we would represent reverse ranges. As > well, I think accessing the tiling is far more clear if we use "tiling_index_" > as the index instead of "tiling_index_ + current_range_.start_index". See my comment about RangeOffsetToTilingSetIndex below. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1712: : -1; On 2014/07/30 00:18:52, vmpstr wrote: > On 2014/07/29 21:41:32, reveman wrote: > > I think it would be cleaner to adjust the index based on direction using an > > utility function with a switch statement right before calling > > layer_->tilings_->tiling_at(). ie. > > layer_->tilings_->tiling_at(RangeOffsetToTilingSetIndex(current_tiling_, > > current_range_)) > > I'm not sure I understand what you mean. Would RangeOffsetToTilingSetIndex > update the index? Is that instead of this function? Can you elaborate a little > bit more? RangeOffsetToTilingSetIndex would simply take an offset in the current range and a range type and return a tiling set index. All the logic needed to handle reverse direction would be easy to implement in that function. All other code can ignore the detail that iterating through a range can be done in different directions. We'd always start at 0 and increment the offset until we reach range.count. This would eliminate the need for CurrentTilingIndexInRange() as that would always be as simple as "current_offset_in_range < range_.count". And RangeOffsetToTilingSetIndex would look something like this: { switch (type) { case HIGHER_THAN_HIGH_RES: case LOW_RES: case HIGH_RES: return range_.offset + offset_in_range; case BETWEEN_HIGH_AND_LOW_RES: case LOWER_THAN_LOW_RES: // Note: walk these tilings in reverse order. // From low res to hi res. return range_.offset + (range_.count - offset_in_range - 1); } NOTREACHED(); return 0; } https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1732: bool PictureLayerImpl::LayerEvictionTileIterator::AdvanceStage() { On 2014/07/30 00:18:52, vmpstr wrote: > On 2014/07/29 21:41:33, reveman wrote: > > Think this function would also be cleaner with a switch statement. It's hard > to > > understand the different stages we go through as it is now. > > switch over bins? That would end up looking like this: > > switch (bin) { > case EVENTUALLY: > if (!required_for_activation) { > required_for_activation = true; > } else { > bin = SOON; > required_for_activation = false; > } > break; > case SOON: > if (!required_for_activation) { > required_for_activation = true; > } else { > bin = NOW; > required_for_activation = false; > } > break; > if (!required_for_activation) { > required_for_activation = true; > } else { > return false; > } > break; > } > return true; > > To me, that's less clear. It's basically repeating exactly the same thing I have > except iteration_stage_ = ... line is more explicit (and it's not immediately > obvious that it's the only difference between each case). Would it be better to > put a comment over the line saying that it goes through bins in EVENTUALLY, > SOON, NOW order? Alternatively, I can put a comment right at the start of the > function enumerating the order of all the stages. Wdyt? Why don't we add an enum for all stages? ie. EVENTUALLY, EVENTUALLY_AND_REQUIRED_FOR_ACTIVATION, SOON... https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:101: int tiling_index_; On 2014/07/30 00:18:52, vmpstr wrote: > On 2014/07/29 21:41:33, reveman wrote: > > nit: current_tiling_ > > > > btw, might be cleaner to maintain this as an offset in the tiling range rather > > than offset from first tiling in the tiling set > > I changed it to current_tiling_index_. I would prefer to keep the word "index" > in there, so it's clear that this is not an actual tiling object/pointer. Acknowledged. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:104: int range_index_; On 2014/07/30 00:18:52, vmpstr wrote: > On 2014/07/29 21:41:33, reveman wrote: > > nit: current_range_ > > > > and "TilingSet::TilingRangeType current_range_" when rebased on my patch. > > Likewise I changed it to current_range_type_ Acknowledged. https://codereview.chromium.org/428533008/diff/80001/cc/resources/picture_lay... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/428533008/diff/80001/cc/resources/picture_lay... cc/resources/picture_layer_tiling.h:118: std::vector<Tile*>* eviction_tiles_; what is this for?
PTAL. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1683: } On 2014/07/30 16:06:47, reveman wrote: > On 2014/07/30 00:18:51, vmpstr wrote: > > On 2014/07/29 21:41:32, reveman wrote: > > > I think I'd like to see this function removed and the check always be as > > simple > > > as "current_tiling_ < current_tiling_range_.count". > > > > I'm still a little bit confused about how we would represent reverse ranges. > As > > well, I think accessing the tiling is far more clear if we use "tiling_index_" > > as the index instead of "tiling_index_ + current_range_.start_index". > > See my comment about RangeOffsetToTilingSetIndex below. Acknowledged. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1712: : -1; On 2014/07/30 16:06:48, reveman wrote: > On 2014/07/30 00:18:52, vmpstr wrote: > > On 2014/07/29 21:41:32, reveman wrote: > > > I think it would be cleaner to adjust the index based on direction using an > > > utility function with a switch statement right before calling > > > layer_->tilings_->tiling_at(). ie. > > > layer_->tilings_->tiling_at(RangeOffsetToTilingSetIndex(current_tiling_, > > > current_range_)) > > > > I'm not sure I understand what you mean. Would RangeOffsetToTilingSetIndex > > update the index? Is that instead of this function? Can you elaborate a little > > bit more? > > RangeOffsetToTilingSetIndex would simply take an offset in the current range and > a range type and return a tiling set index. All the logic needed to handle > reverse direction would be easy to implement in that function. All other code > can ignore the detail that iterating through a range can be done in different > directions. We'd always start at 0 and increment the offset until we reach > range.count. This would eliminate the need for CurrentTilingIndexInRange() as > that would always be as simple as "current_offset_in_range < range_.count". And > RangeOffsetToTilingSetIndex would look something like this: > > { > switch (type) { > case HIGHER_THAN_HIGH_RES: > case LOW_RES: > case HIGH_RES: > return range_.offset + offset_in_range; > case BETWEEN_HIGH_AND_LOW_RES: > case LOWER_THAN_LOW_RES: > // Note: walk these tilings in reverse order. > // From low res to hi res. > return range_.offset + (range_.count - offset_in_range - 1); > } > NOTREACHED(); > return 0; > } I've done this. To be honest, I find this more confusing the previous patch set. Maybe your patch to move this into tiling set will make this better. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1732: bool PictureLayerImpl::LayerEvictionTileIterator::AdvanceStage() { On 2014/07/30 16:06:47, reveman wrote: > On 2014/07/30 00:18:52, vmpstr wrote: > > On 2014/07/29 21:41:33, reveman wrote: > > > Think this function would also be cleaner with a switch statement. It's hard > > to > > > understand the different stages we go through as it is now. > > > > switch over bins? That would end up looking like this: > > > > switch (bin) { > > case EVENTUALLY: > > if (!required_for_activation) { > > required_for_activation = true; > > } else { > > bin = SOON; > > required_for_activation = false; > > } > > break; > > case SOON: > > if (!required_for_activation) { > > required_for_activation = true; > > } else { > > bin = NOW; > > required_for_activation = false; > > } > > break; > > if (!required_for_activation) { > > required_for_activation = true; > > } else { > > return false; > > } > > break; > > } > > return true; > > > > To me, that's less clear. It's basically repeating exactly the same thing I > have > > except iteration_stage_ = ... line is more explicit (and it's not immediately > > obvious that it's the only difference between each case). Would it be better > to > > put a comment over the line saying that it goes through bins in EVENTUALLY, > > SOON, NOW order? Alternatively, I can put a comment right at the start of the > > function enumerating the order of all the stages. Wdyt? > > Why don't we add an enum for all stages? ie. EVENTUALLY, > EVENTUALLY_AND_REQUIRED_FOR_ACTIVATION, SOON... I don't like that. Either this enum would have to live on the tiling iterator, or we would need a couple of functions to extract activation vs bin back. I added a comment here to describe the stages, and I added a switch for the bin advancement. Let me know if this is acceptable. https://codereview.chromium.org/428533008/diff/80001/cc/resources/picture_lay... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/428533008/diff/80001/cc/resources/picture_lay... cc/resources/picture_layer_tiling.h:118: std::vector<Tile*>* eviction_tiles_; On 2014/07/30 16:06:48, reveman wrote: > what is this for? In the iterator, we iterate one of the vectors on the tiling. This points to the vector that we're iterating (set in the constructor of the iterator)
I'll have another look after you rebase this onto my TilingRange patch. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1694: break; On 2014/07/30 00:18:51, vmpstr wrote: > On 2014/07/29 21:41:32, reveman wrote: > > hm, doesn't this need to be a nested loop? ie. we need to keep advancing the > > range until we find one with tilings and same for advancing the stage.. > > AdvanceTiling will advance to the next tiling if one is available and return > false otherwise > AdvanceRange will advance to the next non empty range (always) and return false > if it looped around to the beginning. False here kind of means that it "failed" > to find a non-empty range and had to look through the ranges again. > AdvanceStage will advance the stage if one is available and return false > otherwise. > > So if we get below this line, we can get a new iterator for a new spot that we > haven't been before. If that iterator doesn't produce any tiles we start again. AdvanceTiling doesn't advance until it finds non-empty iterator. To be consistent, AdvanceRange should not advance until it finds a non-empty range. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1706: if (!CurrentTilingIndexInRange()) On 2014/07/30 00:18:52, vmpstr wrote: > On 2014/07/29 21:41:33, reveman wrote: > > Seems odd that we check this both before and after we increment the index. > Would > > setting the current_range_ to { 0, 0 } and current_tiling_ to -1 in the ctor > > help? > > I don't think this was required. I replaced it with a DCHECK. Acknowledged. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1712: : -1; On 2014/07/30 20:06:38, vmpstr wrote: > On 2014/07/30 16:06:48, reveman wrote: > > On 2014/07/30 00:18:52, vmpstr wrote: > > > On 2014/07/29 21:41:32, reveman wrote: > > > > I think it would be cleaner to adjust the index based on direction using > an > > > > utility function with a switch statement right before calling > > > > layer_->tilings_->tiling_at(). ie. > > > > layer_->tilings_->tiling_at(RangeOffsetToTilingSetIndex(current_tiling_, > > > > current_range_)) > > > > > > I'm not sure I understand what you mean. Would RangeOffsetToTilingSetIndex > > > update the index? Is that instead of this function? Can you elaborate a > little > > > bit more? > > > > RangeOffsetToTilingSetIndex would simply take an offset in the current range > and > > a range type and return a tiling set index. All the logic needed to handle > > reverse direction would be easy to implement in that function. All other code > > can ignore the detail that iterating through a range can be done in different > > directions. We'd always start at 0 and increment the offset until we reach > > range.count. This would eliminate the need for CurrentTilingIndexInRange() as > > that would always be as simple as "current_offset_in_range < range_.count". > And > > RangeOffsetToTilingSetIndex would look something like this: > > > > { > > switch (type) { > > case HIGHER_THAN_HIGH_RES: > > case LOW_RES: > > case HIGH_RES: > > return range_.offset + offset_in_range; > > case BETWEEN_HIGH_AND_LOW_RES: > > case LOWER_THAN_LOW_RES: > > // Note: walk these tilings in reverse order. > > // From low res to hi res. > > return range_.offset + (range_.count - offset_in_range - 1); > > } > > NOTREACHED(); > > return 0; > > } > > I've done this. To be honest, I find this more confusing the previous patch set. > Maybe your patch to move this into tiling set will make this better. Let's see what it looks like after rebase onto my patch. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1720: bool wrapped_around = false; On 2014/07/30 00:18:52, vmpstr wrote: > On 2014/07/29 21:41:32, reveman wrote: > > I think this function would be a bit cleaner with a switch statement. > > switch (range_index_) { > case HIGHER_THAN_HIGH_RES: > range_index_ = LOWER_THAN_LOW_RES; > break; > case LOWER_THAN_LOW_RES: > range_index_ = BETWEEN_HIGH_AND_LOW_RES; > break; > ... > } > > Is that what you mean? I changed it to that since I agree that we shouldn't rely > on the enum ordering. Yup, thanks! https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1732: bool PictureLayerImpl::LayerEvictionTileIterator::AdvanceStage() { On 2014/07/30 20:06:38, vmpstr wrote: > On 2014/07/30 16:06:47, reveman wrote: > > On 2014/07/30 00:18:52, vmpstr wrote: > > > On 2014/07/29 21:41:33, reveman wrote: > > > > Think this function would also be cleaner with a switch statement. It's > hard > > > to > > > > understand the different stages we go through as it is now. > > > > > > switch over bins? That would end up looking like this: > > > > > > switch (bin) { > > > case EVENTUALLY: > > > if (!required_for_activation) { > > > required_for_activation = true; > > > } else { > > > bin = SOON; > > > required_for_activation = false; > > > } > > > break; > > > case SOON: > > > if (!required_for_activation) { > > > required_for_activation = true; > > > } else { > > > bin = NOW; > > > required_for_activation = false; > > > } > > > break; > > > if (!required_for_activation) { > > > required_for_activation = true; > > > } else { > > > return false; > > > } > > > break; > > > } > > > return true; > > > > > > To me, that's less clear. It's basically repeating exactly the same thing I > > have > > > except iteration_stage_ = ... line is more explicit (and it's not > immediately > > > obvious that it's the only difference between each case). Would it be better > > to > > > put a comment over the line saying that it goes through bins in EVENTUALLY, > > > SOON, NOW order? Alternatively, I can put a comment right at the start of > the > > > function enumerating the order of all the stages. Wdyt? > > > > Why don't we add an enum for all stages? ie. EVENTUALLY, > > EVENTUALLY_AND_REQUIRED_FOR_ACTIVATION, SOON... > > I don't like that. Either this enum would have to live on the tiling iterator, > or we would need a couple of functions to extract activation vs bin back. I > added a comment here to describe the stages, and I added a switch for the bin > advancement. Let me know if this is acceptable. Enum living here and two utility functions that return tile-type and required-for-activation when passed this enum sgtm. It would remove all the need for comments in this function. https://codereview.chromium.org/428533008/diff/100001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1728: int PictureLayerImpl::LayerEvictionTileIterator::GetCurrentTilingIndex() { can you move this function to anonymous namespace? https://codereview.chromium.org/428533008/diff/100001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:93: IterationStage(TilePriority::PriorityBin type, bool is_required) style nit: use the same name for ctor arguments and members
PTAL https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1694: break; On 2014/07/31 15:32:47, reveman wrote: > On 2014/07/30 00:18:51, vmpstr wrote: > > On 2014/07/29 21:41:32, reveman wrote: > > > hm, doesn't this need to be a nested loop? ie. we need to keep advancing the > > > range until we find one with tilings and same for advancing the stage.. > > > > AdvanceTiling will advance to the next tiling if one is available and return > > false otherwise > > AdvanceRange will advance to the next non empty range (always) and return > false > > if it looped around to the beginning. False here kind of means that it > "failed" > > to find a non-empty range and had to look through the ranges again. > > AdvanceStage will advance the stage if one is available and return false > > otherwise. > > > > So if we get below this line, we can get a new iterator for a new spot that we > > haven't been before. If that iterator doesn't produce any tiles we start > again. > > AdvanceTiling doesn't advance until it finds non-empty iterator. To be > consistent, AdvanceRange should not advance until it finds a non-empty range. Well, this is kind of a weird one. Here's my reasoning: Advance tiling advances to the next valid tiling (ie tiling exists, no mention of iterators). When it can no longer do that, it returns false. Stage is similar, it advances to the next valid stage (no mention of iterators). When it can no longer do that, it returns false. Advance range advances to the next valid range (ie, range exists.. in this case ranges that are empty don't exist). When it can no longer do that.. oh wait, it can always find a range because it loops back, so we return false if it looped back. Yes, the semantics of advance range are subtly different from stage and tiling, but I think making it only advance once as you suggest and treat empty ranges etc makes this function more awkward. For instance, we can return false to indicate that we went to an empty range or that we looped around. Suppose we return false when we enter an empty range, then we don't really know when to advance the stage (ie we don't know when we loop). The other case is less awkward and ends up with the following: bool success; ... // handle tilings // handle ranges if (!success) { do { success = AdvanceRange(); } while (success && CurrentRange().IsEmpty()); } // handle stages if (!success) ... or something similar to that. That's OK, I guess. If you insist I can make it that, but please consider the current solution as I prefer it. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1712: : -1; On 2014/07/31 15:32:47, reveman wrote: > On 2014/07/30 20:06:38, vmpstr wrote: > > On 2014/07/30 16:06:48, reveman wrote: > > > On 2014/07/30 00:18:52, vmpstr wrote: > > > > On 2014/07/29 21:41:32, reveman wrote: > > > > > I think it would be cleaner to adjust the index based on direction using > > an > > > > > utility function with a switch statement right before calling > > > > > layer_->tilings_->tiling_at(). ie. > > > > > layer_->tilings_->tiling_at(RangeOffsetToTilingSetIndex(current_tiling_, > > > > > current_range_)) > > > > > > > > I'm not sure I understand what you mean. Would RangeOffsetToTilingSetIndex > > > > update the index? Is that instead of this function? Can you elaborate a > > little > > > > bit more? > > > > > > RangeOffsetToTilingSetIndex would simply take an offset in the current range > > and > > > a range type and return a tiling set index. All the logic needed to handle > > > reverse direction would be easy to implement in that function. All other > code > > > can ignore the detail that iterating through a range can be done in > different > > > directions. We'd always start at 0 and increment the offset until we reach > > > range.count. This would eliminate the need for CurrentTilingIndexInRange() > as > > > that would always be as simple as "current_offset_in_range < range_.count". > > And > > > RangeOffsetToTilingSetIndex would look something like this: > > > > > > { > > > switch (type) { > > > case HIGHER_THAN_HIGH_RES: > > > case LOW_RES: > > > case HIGH_RES: > > > return range_.offset + offset_in_range; > > > case BETWEEN_HIGH_AND_LOW_RES: > > > case LOWER_THAN_LOW_RES: > > > // Note: walk these tilings in reverse order. > > > // From low res to hi res. > > > return range_.offset + (range_.count - offset_in_range - 1); > > > } > > > NOTREACHED(); > > > return 0; > > > } > > > > I've done this. To be honest, I find this more confusing the previous patch > set. > > Maybe your patch to move this into tiling set will make this better. > > Let's see what it looks like after rebase onto my patch. Done. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1732: bool PictureLayerImpl::LayerEvictionTileIterator::AdvanceStage() { On 2014/07/31 15:32:47, reveman wrote: > On 2014/07/30 20:06:38, vmpstr wrote: > > On 2014/07/30 16:06:47, reveman wrote: > > > On 2014/07/30 00:18:52, vmpstr wrote: > > > > On 2014/07/29 21:41:33, reveman wrote: > > > > > Think this function would also be cleaner with a switch statement. It's > > hard > > > > to > > > > > understand the different stages we go through as it is now. > > > > > > > > switch over bins? That would end up looking like this: > > > > > > > > switch (bin) { > > > > case EVENTUALLY: > > > > if (!required_for_activation) { > > > > required_for_activation = true; > > > > } else { > > > > bin = SOON; > > > > required_for_activation = false; > > > > } > > > > break; > > > > case SOON: > > > > if (!required_for_activation) { > > > > required_for_activation = true; > > > > } else { > > > > bin = NOW; > > > > required_for_activation = false; > > > > } > > > > break; > > > > if (!required_for_activation) { > > > > required_for_activation = true; > > > > } else { > > > > return false; > > > > } > > > > break; > > > > } > > > > return true; > > > > > > > > To me, that's less clear. It's basically repeating exactly the same thing > I > > > have > > > > except iteration_stage_ = ... line is more explicit (and it's not > > immediately > > > > obvious that it's the only difference between each case). Would it be > better > > > to > > > > put a comment over the line saying that it goes through bins in > EVENTUALLY, > > > > SOON, NOW order? Alternatively, I can put a comment right at the start of > > the > > > > function enumerating the order of all the stages. Wdyt? > > > > > > Why don't we add an enum for all stages? ie. EVENTUALLY, > > > EVENTUALLY_AND_REQUIRED_FOR_ACTIVATION, SOON... > > > > I don't like that. Either this enum would have to live on the tiling iterator, > > or we would need a couple of functions to extract activation vs bin back. I > > added a comment here to describe the stages, and I added a switch for the bin > > advancement. Let me know if this is acceptable. > > Enum living here and two utility functions that return tile-type and > required-for-activation when passed this enum sgtm. It would remove all the need > for comments in this function. Done. https://codereview.chromium.org/428533008/diff/100001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1728: int PictureLayerImpl::LayerEvictionTileIterator::GetCurrentTilingIndex() { On 2014/07/31 15:32:48, reveman wrote: > can you move this function to anonymous namespace? It's using member variables. I like the fact that I don't have to think about what index/offset to pass to it, to be honest. If we want to change this, I'd recommend we move the whole iterator to a different file, since I don't really want to have anonymous functions in layer that are only used in one of the iterators. https://codereview.chromium.org/428533008/diff/100001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:93: IterationStage(TilePriority::PriorityBin type, bool is_required) On 2014/07/31 15:32:48, reveman wrote: > style nit: use the same name for ctor arguments and members This is gone now.
https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1694: break; On 2014/08/01 06:04:48, vmpstr wrote: > On 2014/07/31 15:32:47, reveman wrote: > > On 2014/07/30 00:18:51, vmpstr wrote: > > > On 2014/07/29 21:41:32, reveman wrote: > > > > hm, doesn't this need to be a nested loop? ie. we need to keep advancing > the > > > > range until we find one with tilings and same for advancing the stage.. > > > > > > AdvanceTiling will advance to the next tiling if one is available and return > > > false otherwise > > > AdvanceRange will advance to the next non empty range (always) and return > > false > > > if it looped around to the beginning. False here kind of means that it > > "failed" > > > to find a non-empty range and had to look through the ranges again. > > > AdvanceStage will advance the stage if one is available and return false > > > otherwise. > > > > > > So if we get below this line, we can get a new iterator for a new spot that > we > > > haven't been before. If that iterator doesn't produce any tiles we start > > again. > > > > AdvanceTiling doesn't advance until it finds non-empty iterator. To be > > consistent, AdvanceRange should not advance until it finds a non-empty range. > > Well, this is kind of a weird one. Here's my reasoning: > > Advance tiling advances to the next valid tiling (ie tiling exists, no mention > of iterators). When it can no longer do that, it returns false. > > Stage is similar, it advances to the next valid stage (no mention of iterators). > When it can no longer do that, it returns false. > > Advance range advances to the next valid range (ie, range exists.. in this case > ranges that are empty don't exist). When it can no longer do that.. oh wait, it > can always find a range because it loops back, so we return false if it looped > back. > > Yes, the semantics of advance range are subtly different from stage and tiling, > but I think making it only advance once as you suggest and treat empty ranges > etc makes this function more awkward. For instance, we can return false to > indicate that we went to an empty range or that we looped around. Suppose we > return false when we enter an empty range, then we don't really know when to > advance the stage (ie we don't know when we loop). > > The other case is less awkward and ends up with the following: > > bool success; > ... > // handle tilings > > // handle ranges > if (!success) { > do { > success = AdvanceRange(); > } while (success && CurrentRange().IsEmpty()); > } > > // handle stages > if (!success) > ... > > or something similar to that. > > That's OK, I guess. If you insist I can make it that, but please consider the > current solution as I prefer it. I feel like there's a lack of uniformity in current patch. To make it easier to understand what these AdvanceToNext*() functions do, I'd like to be able to define the behavior of AdvanceToNext*() in a way that is independent of what's being advanced. ie. "bool AdvanceToNextX()"; Advance X and return false when X cannot be advanced any further. AdvanceToNextX() might have to call AdvanceToNextY() but that's a detail that the caller of AdvanceToNextX() should not have to be aware of. This is basically how it would work if iterators were used. I was thinking we could do something similar to this: opertor++() { ++current_iterator; while (!current_iterator) { if (!AdvanceToNextTiling()) break; current_iterator = Iterator(...); } } bool AdvanceToNextTiling() { ++current_tiling_; while (current_tiling_ == current_tiling_range().end) { if (!AdvanceToNextTilingRangeType()) return false; current_tiling_ = current_tiling_range().start; } return true; } bool AdvanceToNextTilingRangeType() { switch (current_tiling_range_type_) { case HIGHER_THAN_HIGH_RES: case HIGH_RES: case BETWEEN_HIGH_AND_LOW_RES: case LOW_RES: current_tiling_range_type_ = static_cast<TilingRangeType>( current_tiling_range_type_ + 1); return true; case LOWER_THAN_LOW_RES: if (!AdvanceToNextStage()) return false; current_tiling_range_type_ = HIGHER_THAN_HIGH_RES; return true; } NOTREACHED(); return false; } bool AdvanceToNextStage() { switch (current_stage_) { case EVENTUALLY: case EVENTUALLY_AND_REQUIRED_FOR_ACTIVATION: case SOON: case SOON_AND_REQUIRED_FOR_ACTIVATION: case NOW: current_stage_ = static_cast<Stage>(current_stage_ + 1); return true; case NOW_AND_REQUIRED_FOR_ACTIVATION: return false; } NOTREACHED(); return false; } and ctor could then be reduced to something like this: current_stage_ = EVENTUALLY; current_tiling_range_type_ = HIGHER_THAN_HIGH_RES; current_tiling_ = current_tiling_range().start - 1; do { if (!AdvanceToNextTiling()) break; current_iterator = Iterator(...); } while (!current_iterator); Wdyt? https://codereview.chromium.org/428533008/diff/100001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1728: int PictureLayerImpl::LayerEvictionTileIterator::GetCurrentTilingIndex() { On 2014/08/01 06:04:48, vmpstr wrote: > On 2014/07/31 15:32:48, reveman wrote: > > can you move this function to anonymous namespace? > > It's using member variables. I like the fact that I don't have to think about > what index/offset to pass to it, to be honest. If we want to change this, I'd > recommend we move the whole iterator to a different file, since I don't really > want to have anonymous functions in layer that are only used in one of the > iterators. Acknowledged. https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1560: if (!layer_->tilings_ || !layer_->tilings_->num_tilings()) I don't think you should need to check num_tilings(). the tiling set should return empty ranges and that should result in us doing the right thing in this iterator without any special handling. https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:99: bool AdvanceStage(); Please get these function names and variables names below to line up so it's easy to see what is used for what. ie. AdvanceToNextTiling would use the "current_tiling_" state and AdvanceToNextTilingRangeType would use current_tiling_range_type_ state.. https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:102: int current_range_offset_; size_t instead of int? https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:104: IterationStage current_stage_; maybe current_iteration_stage_ instead to be perfectly consistent. https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:1013: if (required_for_activation && type != TilePriority::NOW) could this be a DCHECK? https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:1043: return eviction_tiles_ && tile_iterator_ != eviction_tiles_->end(); remove eviction_tiles_ check if you can DCHECK above. https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_set.h (right): https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_set.h:29: } I don't think we need this. I can't see a reason why you would have to check that "index >= start".
For the record, this patch initially was to create a tiling iterator only when required. I think we already passed "and make a small design change" stage and are well into "rewrite the whole thing". In particular with respect to the last patch set, I don't see any clear advantage the latest patchset has over the previous one. The previous patchset had an advantage that operator++ was very clear about what order things were advanced. Now, the reader has to dig into each of the Advance* functions to see what gets advanced next. (operator++ was a concept level function dealing with higher order concepts and not caring about implementation details. Advance* functions were implementation level). However, with the latest patch set, each of the functions are consistent in that they will do the most amount of work they can to get a valid iterator, the previous patch only cares about advancing its own thing (however, all of the functions including operator++ lose the concept level information and the reader has to know what each of the function does before getting an idea of what the iterator is advancing). Is one better than the other? I'm sure we both can make arguments for either one. I understand that you believe it's more clear, but I'd like to reiterate that my main priority right now is enabling these iterators. Rewriting what I think was a clear patch into what you think is a clear patch is not really making any progress towards that goal (regardless of the fact which one is objectively more clear if such a thing even exists). Note that I truly don't believe that I'm trying to take any shortcuts. It's just that I think we can agree that there are many ways to accomplish the same thing. I mean if I make a small patch for the layer raster iterator, will it end up being a full rewrite as well? :) I know that you don't like the array in there, but I don't know if anything I write for that will initially address that. Sorry for a bit of a rant... PTAL. https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:1694: break; On 2014/08/01 17:54:46, reveman wrote: > On 2014/08/01 06:04:48, vmpstr wrote: > > On 2014/07/31 15:32:47, reveman wrote: > > > On 2014/07/30 00:18:51, vmpstr wrote: > > > > On 2014/07/29 21:41:32, reveman wrote: > > > > > hm, doesn't this need to be a nested loop? ie. we need to keep advancing > > the > > > > > range until we find one with tilings and same for advancing the stage.. > > > > > > > > AdvanceTiling will advance to the next tiling if one is available and > return > > > > false otherwise > > > > AdvanceRange will advance to the next non empty range (always) and return > > > false > > > > if it looped around to the beginning. False here kind of means that it > > > "failed" > > > > to find a non-empty range and had to look through the ranges again. > > > > AdvanceStage will advance the stage if one is available and return false > > > > otherwise. > > > > > > > > So if we get below this line, we can get a new iterator for a new spot > that > > we > > > > haven't been before. If that iterator doesn't produce any tiles we start > > > again. > > > > > > AdvanceTiling doesn't advance until it finds non-empty iterator. To be > > > consistent, AdvanceRange should not advance until it finds a non-empty > range. > > > > Well, this is kind of a weird one. Here's my reasoning: > > > > Advance tiling advances to the next valid tiling (ie tiling exists, no mention > > of iterators). When it can no longer do that, it returns false. > > > > Stage is similar, it advances to the next valid stage (no mention of > iterators). > > When it can no longer do that, it returns false. > > > > Advance range advances to the next valid range (ie, range exists.. in this > case > > ranges that are empty don't exist). When it can no longer do that.. oh wait, > it > > can always find a range because it loops back, so we return false if it looped > > back. > > > > Yes, the semantics of advance range are subtly different from stage and > tiling, > > but I think making it only advance once as you suggest and treat empty ranges > > etc makes this function more awkward. For instance, we can return false to > > indicate that we went to an empty range or that we looped around. Suppose we > > return false when we enter an empty range, then we don't really know when to > > advance the stage (ie we don't know when we loop). > > > > The other case is less awkward and ends up with the following: > > > > bool success; > > ... > > // handle tilings > > > > // handle ranges > > if (!success) { > > do { > > success = AdvanceRange(); > > } while (success && CurrentRange().IsEmpty()); > > } > > > > // handle stages > > if (!success) > > ... > > > > or something similar to that. > > > > That's OK, I guess. If you insist I can make it that, but please consider the > > current solution as I prefer it. > > I feel like there's a lack of uniformity in current patch. To make it easier to > understand what these AdvanceToNext*() functions do, I'd like to be able to > define the behavior of AdvanceToNext*() in a way that is independent of what's > being advanced. ie. "bool AdvanceToNextX()"; Advance X and return false when X > cannot be advanced any further. AdvanceToNextX() might have to call > AdvanceToNextY() but that's a detail that the caller of AdvanceToNextX() should > not have to be aware of. This is basically how it would work if iterators were > used. > > I was thinking we could do something similar to this: > > opertor++() { > ++current_iterator; > while (!current_iterator) { > if (!AdvanceToNextTiling()) > break; > > current_iterator = Iterator(...); > } > } > > bool AdvanceToNextTiling() { > ++current_tiling_; > while (current_tiling_ == current_tiling_range().end) { > if (!AdvanceToNextTilingRangeType()) > return false; > > current_tiling_ = current_tiling_range().start; > } > return true; > } > > bool AdvanceToNextTilingRangeType() { > switch (current_tiling_range_type_) { > case HIGHER_THAN_HIGH_RES: > case HIGH_RES: > case BETWEEN_HIGH_AND_LOW_RES: > case LOW_RES: > current_tiling_range_type_ = static_cast<TilingRangeType>( > current_tiling_range_type_ + 1); > return true; > case LOWER_THAN_LOW_RES: > if (!AdvanceToNextStage()) > return false; > > current_tiling_range_type_ = HIGHER_THAN_HIGH_RES; > return true; > } > NOTREACHED(); > return false; > } > > bool AdvanceToNextStage() { > switch (current_stage_) { > case EVENTUALLY: > case EVENTUALLY_AND_REQUIRED_FOR_ACTIVATION: > case SOON: > case SOON_AND_REQUIRED_FOR_ACTIVATION: > case NOW: > current_stage_ = static_cast<Stage>(current_stage_ + 1); > return true; > case NOW_AND_REQUIRED_FOR_ACTIVATION: > return false; > } > NOTREACHED(); > return false; > } > > and ctor could then be reduced to something like this: > > current_stage_ = EVENTUALLY; > current_tiling_range_type_ = HIGHER_THAN_HIGH_RES; > current_tiling_ = current_tiling_range().start - 1; > do { > if (!AdvanceToNextTiling()) > break; > > current_iterator = Iterator(...); > } while (!current_iterator); > > Wdyt? Done. https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1560: if (!layer_->tilings_ || !layer_->tilings_->num_tilings()) On 2014/08/01 17:54:47, reveman wrote: > I don't think you should need to check num_tilings(). the tiling set should > return empty ranges and that should result in us doing the right thing in this > iterator without any special handling. It's an easy constructor early out. I mean even if I make it so that we handle this correctly (and I think we don't right now since advance tiling assumes that it will find a tiling), what's the objection against this? If it's not there, we'll just end up doing a bunch of work with exactly the same outcome. Please provide a justification. https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:99: bool AdvanceStage(); On 2014/08/01 17:54:47, reveman wrote: > Please get these function names and variables names below to line up so it's > easy to see what is used for what. ie. AdvanceToNextTiling would use the > "current_tiling_" state and AdvanceToNextTilingRangeType would use > current_tiling_range_type_ state.. Done. https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:102: int current_range_offset_; On 2014/08/01 17:54:47, reveman wrote: > size_t instead of int? Done. https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:104: IterationStage current_stage_; On 2014/08/01 17:54:47, reveman wrote: > maybe current_iteration_stage_ instead to be perfectly consistent. Done. https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:1013: if (required_for_activation && type != TilePriority::NOW) On 2014/08/01 17:54:47, reveman wrote: > could this be a DCHECK? No, this can't be a dcheck since we pass all 6 stages here, and I would like to not rely on the fact that 2 of those should always be empty. https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_set.h (right): https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_set.h:29: } On 2014/08/01 17:54:47, reveman wrote: > I don't think we need this. I can't see a reason why you would have to check > that "index >= start". We iterate two of the ranges in reverse order, so we need to know both if we end up below start and if we end up at or above end
Please don't make any changes that you don't agree with. You can land this in whatever form you like and I'll instead create some follow up patches if there's something I still feel can be improved. The important part is that it's relatively easy to understand what it does. I think we've passed that stage. LGTM. https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1560: if (!layer_->tilings_ || !layer_->tilings_->num_tilings()) On 2014/08/01 19:39:50, vmpstr wrote: > On 2014/08/01 17:54:47, reveman wrote: > > I don't think you should need to check num_tilings(). the tiling set should > > return empty ranges and that should result in us doing the right thing in this > > iterator without any special handling. > > It's an easy constructor early out. I mean even if I make it so that we handle > this correctly (and I think we don't right now since advance tiling assumes that > it will find a tiling), what's the objection against this? If it's not there, > we'll just end up doing a bunch of work with exactly the same outcome. > > Please provide a justification. I don't recommend doing it unless it's a cleanup. Just seems awkward that we have to special case this. I'd guess that removing it would result in cleanup but don't bother if that's not the case. https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/428533008/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:1013: if (required_for_activation && type != TilePriority::NOW) On 2014/08/01 19:39:50, vmpstr wrote: > On 2014/08/01 17:54:47, reveman wrote: > > could this be a DCHECK? > > No, this can't be a dcheck since we pass all 6 stages here, and I would like to > not rely on the fact that 2 of those should always be empty. Acknowledged.
On 2014/08/04 14:56:18, reveman wrote: > Please don't make any changes that you don't agree with. You can land this in > whatever form you like and I'll instead create some follow up patches if there's > something I still feel can be improved. The important part is that it's > relatively easy to understand what it does. I think we've passed that stage. > LGTM. Don't misunderstand, I think the changes you are proposing are making things better. It's just sometimes it hits diminishing returns, as major updates to the way things are done yield fairly small improvements in both clarity and performance. Thanks for putting up with my objections... I think after the whole thing lands and I get some more time, we should have a little design discussion and agree on the design that we're both happy with, even if it means doing some major rewrites. > > https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/428533008/diff/140001/cc/layers/picture_layer... > cc/layers/picture_layer_impl.cc:1560: if (!layer_->tilings_ || > !layer_->tilings_->num_tilings()) > On 2014/08/01 19:39:50, vmpstr wrote: > > On 2014/08/01 17:54:47, reveman wrote: > > > I don't think you should need to check num_tilings(). the tiling set should > > > return empty ranges and that should result in us doing the right thing in > this > > > iterator without any special handling. > > > > It's an easy constructor early out. I mean even if I make it so that we handle > > this correctly (and I think we don't right now since advance tiling assumes > that > > it will find a tiling), what's the objection against this? If it's not there, > > we'll just end up doing a bunch of work with exactly the same outcome. > > > > Please provide a justification. > > I don't recommend doing it unless it's a cleanup. Just seems awkward that we > have to special case this. I'd guess that removing it would result in cleanup > but don't bother if that's not the case. Done.
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/428533008/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/37163) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42189) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/37174) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/428533008/180001
The CQ bit was unchecked by vmpstr@chromium.org
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/428533008/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/428533008/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
Message was sent while issue was closed.
Change committed as 287688 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
