Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183663003:
cc: Add tiling raster tile iterators. (Closed)
|
Created:
6 years, 9 months ago by vmpstr Modified:
6 years, 9 months ago CC:
chromium-reviews, cc-bugs_chromium.org, epenner, aelias_OOO_until_Jul13 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Add tiling raster tile iterators.
This patch adds PictureLayerTiling::Tiling{Raster,Eviction}TileIterator
classes. This is required for a larger change to tile prioritization.
Currently, the classes are not used by anything except unit and perf
tests.
BUG=329686
R=enne@chromium.org, reveman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257041
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 15
Patch Set 8 : #
Total comments: 22
Patch Set 9 : update #Patch Set 10 : update #Patch Set 11 : #
Messages
Total messages: 32 (0 generated)
Hi, This is a patch that adds the iterators to the tiling. I tried to keep most of the cost in the iterators, but some has to appear in update tile priorities. Please take a look. https://codereview.chromium.org/183663003/diff/20001/cc/resources/picture_lay... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/183663003/diff/20001/cc/resources/picture_lay... cc/resources/picture_layer_tiling.cc:546: visible_tiles_.push_back(tile); Doing conditional pushes here (if it actually needs raster, or if it actually has resources to evict) results in a bigger regression in update tile priorities. With this approach the regression on my machine is about 10% (which is still pretty bad). But at least, most of the cost is moved to the iterators that are currently unused. I think making this part fast is more of a priority than I initially thought.
I have to admit I'm a little concerned about the perf regression. Is it possible to just make the iterators go find the tiles they need in the order they need them rather than pushing into vectors and sorting? I think I'm suggesting that you hoist the TilingData::Iterator/TilingData/DifferenceIterator iterators out of UpdateTilePriorities and into TilingRasterTileIterator. Then for now, UpdateTilePriorities could use the TilingRasterTileIterator and set priorities on all those tiles. TileRasterTileIterator would also have to be able to tell a caller you whether a tile was visible/skew/eventually. I think that's in line with where we're going in the future (aka don't prioritize and iterate through everything) and should help with short-term performance too as it'd be a similar amount of work.
On 2014/02/27 21:53:38, enne wrote: > I have to admit I'm a little concerned about the perf regression. > > Is it possible to just make the iterators go find the tiles they need in the > order they need them rather than pushing into vectors and sorting? Note that the 10% regression is just from the pushing, sorting doesn't come into play there. Sorting is done only by the use of the iterator. > > I think I'm suggesting that you hoist the > TilingData::Iterator/TilingData/DifferenceIterator iterators out of > UpdateTilePriorities and into TilingRasterTileIterator. Then for now, > UpdateTilePriorities could use the TilingRasterTileIterator and set priorities > on all those tiles. TileRasterTileIterator would also have to be able to tell a > caller you whether a tile was visible/skew/eventually. > > I think that's in line with where we're going in the future (aka don't > prioritize and iterate through everything) and should help with short-term > performance too as it'd be a similar amount of work. OK, I can work on doing this. The biggest problem here I think will be figuring out in which order to iterate things. That is, Simple Iterator and DifferenceIterator is not as appropriate here, since we need the tiles to be returned in an increasing distance from the viewport order. Maybe we need some sort of a new iterator with a center (for distance) and a rect to iterate, which would do a smart thing? I'm not sure yet.
On 2014/02/27 22:49:47, vmpstr wrote: > OK, I can work on doing this. The biggest problem here I think will be figuring > out in which order to iterate things. That is, Simple Iterator and > DifferenceIterator is not as appropriate here, since we need the tiles to be > returned in an increasing distance from the viewport order. Maybe we need some > sort of a new iterator with a center (for distance) and a rect to iterate, which > would do a smart thing? I'm not sure yet. Yeah, maybe you'd need a different iterator for eventually tiles. I guess also for skewport if it's skewed in multiple directions. But even then you just need a few pieces of state for the iterator rather than vectors of everything.
OK, I think this does what you proposed. This is still quite a bad perf regression, but let's hold off on landing this until I have more patches ready to go.
Cool stuff. The iterator concept is nice. I looked mostly at the unit-test to figure out what's going on. First round of comments is there.
On 2014/03/04 22:58:32, epennerAtGoogle wrote: > Cool stuff. The iterator concept is nice. > > I looked mostly at the unit-test to figure out what's going on. First round of > comments is there. Did you publish the comments? I don't see anything on the latest patch set.
Doh! https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:784: for (int i = 0; i < 3; ++i) { Might help to say why there is three passes. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:787: // There are 3 bins in TilePriority. Can we add a TilePriority::NUM_PRIORITIES, or is that annoying for 'no default' switch statements? Even if it was an constant (beside the enum) it would be nicer IMO. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:796: // On the second iteration, mark everything as ready to draw (solid color). It wasn't super clear to me what impact this should have on this test or how that is tested (since no asserts have i==1). https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:809: unique_tiles.insert(new_tile); Should this check that the tile is not already in unique_tiles? I think std::Set doesn't mind multiple insertions. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:840: eventually_bin_order_incorrect_count); I've looked at the above code but this is still confusing me. Part of it is just the naming (Two 'rights' don't fix two 'wrongs', so it could probably use a better name). More importantly though I don't see why these distance comparisons should balance one another in the EVENTUALLY bin. Maybe just a comment will clear this up. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:868: // There are 3 bins in TilePriority. Same here.
Couple more comments that I missed. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:764: operator++() { General question regarding final design: Is this eventually going to be the only place where we set the tile's priority? It seems like we only need it at this point. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling.h:48: class CC_EXPORT TilingRasterTileIterator { Cool. One thing I didn't see in here is whether we filter already-rastered tiles. Seems like we should only return tiles that haven't yet been rastered.
Just answers, no new patch set yet. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:764: operator++() { On 2014/03/05 00:16:03, epennerAtGoogle wrote: > General question regarding final design: Is this eventually going to be the only > place where we set the tile's priority? It seems like we only need it at this > point. Yeah, the priority will be set lazily. I think here is the best spot. Possibly in the layer/tile manager version of these iterators as well, but I'll have to see what makes it look cleanest. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling.h:48: class CC_EXPORT TilingRasterTileIterator { On 2014/03/05 00:16:03, epennerAtGoogle wrote: > Cool. One thing I didn't see in here is whether we filter already-rastered > tiles. Seems like we should only return tiles that haven't yet been rastered. Not really... See my comment in the unittests. I think I can rework this a bit. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:784: for (int i = 0; i < 3; ++i) { On 2014/03/05 00:05:50, epennerAtGoogle wrote: > Might help to say why there is three passes. I'll add a comment. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:787: // There are 3 bins in TilePriority. On 2014/03/05 00:05:50, epennerAtGoogle wrote: > Can we add a TilePriority::NUM_PRIORITIES, or is that annoying for 'no default' > switch statements? Even if it was an constant (beside the enum) it would be > nicer IMO. There was a bit of a discussion a while back on this (https://codereview.chromium.org/22875045/). Without re-reading all of the comments, I don't think we really agreed on any particular style or variable name. In that spirit, and because this is only used in unittests (now and in foreseeable future), I can define a constant right here for the size of the enums. Do you think that would be an acceptable compromise? https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:796: // On the second iteration, mark everything as ready to draw (solid color). On 2014/03/05 00:05:50, epennerAtGoogle wrote: > It wasn't super clear to me what impact this should have on this test or how > that is tested (since no asserts have i==1). I agree that this part is a bit confusing (and this goes along with your comment that we don't filter tiles that are already rasterized as well): The final stage of this iterator would be to only return tiles that need to be rasterized. Initially, the third pass through the tiles just said "EXPECT_FALSE(it)" right at the start, since everything is marked as ready to draw on the second pass. However, because of the usage here (to update tile priorities), it returns all tiles. There are a couple of options: 1. Wait until I have a chance to write up more follow-up patches that will eventually make the transition from all tiles to only raster-required tiles (which would also update this test and remove the use in update tile priorities in favour of more laziness) 2. Change this now so that update tile priorities uses the previous method of updating the priorities and raster tile iterator goes back to being used only in unittests. I think I actually like option #2, since that would make this patch landable without regression (well, setting three more rects will regress, but not noticeably). https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:809: unique_tiles.insert(new_tile); On 2014/03/05 00:05:50, epennerAtGoogle wrote: > Should this check that the tile is not already in unique_tiles? I think std::Set > doesn't mind multiple insertions. This goes with the check on line 848 that ensures that unique tiles size is the same as all tiles size. If there were duplicates, then unique tiles size would be less. https://codereview.chromium.org/183663003/diff/120001/cc/resources/picture_la... cc/resources/picture_layer_tiling_unittest.cc:840: eventually_bin_order_incorrect_count); On 2014/03/05 00:05:50, epennerAtGoogle wrote: > I've looked at the above code but this is still confusing me. Part of it is just > the naming (Two 'rights' don't fix two 'wrongs', so it could probably use a > better name). More importantly though I don't see why these distance comparisons > should balance one another in the EVENTUALLY bin. Maybe just a comment will > clear this up. This part is a consequence of the spiral iterator. In a square spiral, the corner tiles will be further away than the edge tiles relative to the rect that is being spiraled. This means that we technically would return wrong order tiles (ie, corners before some of the edges). This is partly because we use manhattan distance, and partly because having a spiral iteration that considers distance would be fairly complicated (that last bit is just my gut feeling). Personally, I feel that is an acceptable limitation. That is, we're still perfect in the visible rect and in a single direction skewport. In the eventually rect, we're "OK" as in we're mostly correct. Anyway, the end result is that this is hard to test. This line just says that for the eventually bin we're more right than wrong. Suggestions for something better are more than welcome.
PTAL. I suspect I might've leaked a few other changes in here like making CreateTile return the tile in preparation for being able to have "GetOrCreateTile(int i, int j)" type of functionality. Let me know if you would prefer I remove it.
On 2014/03/05 01:20:25, vmpstr wrote: > PTAL. I suspect I might've leaked a few other changes in here like making > CreateTile return the tile in preparation for being able to have > "GetOrCreateTile(int i, int j)" type of functionality. Let me know if you would > prefer I remove it. Btw, the latest patch does not regress and thus is landable. The iterator code is only used in the unit and perf tests (thanks Eric for kicking my brain)
On 2014/03/05 01:21:22, vmpstr wrote: > On 2014/03/05 01:20:25, vmpstr wrote: > > PTAL. I suspect I might've leaked a few other changes in here like making > > CreateTile return the tile in preparation for being able to have > > "GetOrCreateTile(int i, int j)" type of functionality. Let me know if you > would > > prefer I remove it. > > Btw, the latest patch does not regress and thus is landable. The iterator code > is only used in the unit and perf tests (thanks Eric for kicking my brain) preemptive ping.
Can you update the description to remove the perf warning? https://codereview.chromium.org/183663003/diff/140001/cc/base/tiling_data.cc File cc/base/tiling_data.cc (right): https://codereview.chromium.org/183663003/diff/140001/cc/base/tiling_data.cc#... cc/base/tiling_data.cc:77: RoundDown(x - 2 * border_texels_, inner_tile_width) / inner_tile_width; Why? https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:441: TileMap::const_iterator find = tiles_.find(iter.index()); I realize that technically the iterator is const because you're not modifying the map, you're modifying something the map points to. It's like using const on a function that modifies something your object points to. Sure, it's technically right, but I think it's a confusing communication to other developers. Does const buy you something here? If not, I preferred the previous code. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.h:64: bool HasVisibleTiles() const { return type_ == VISIBLE; } unused? https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.h:271: gfx::Rect current_visible_rect_in_content_space_; I think it'd be cleaner if you just passed them to the iterators explicitly and didn't store them on the tiling. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_perftest.cc:188: TEST_F(PictureLayerTilingPerfTest, TilingRasterTileIterator) { This is a little weird. Isn't this an UpdateTilePriorities test and not a RasterTileIterator test? This test won't really be here in the end state, right?
Just some suggestions. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:559: size_t PictureLayerTiling::RequiredGPUMemoryInBytes() const { Could this be replaced with math? If not, or this isn't a significant cost, maybe just comment why it isn't just math at the moment, for posterity. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:783: while (spiral_iterator_) { This is just for advancing past rastered tiles right? It feels sort of awkward to be done in several places. Feel free to ignore this suggestion if it doesn't end up helping, but would it work to have an Advance() function that just advances the iterator one step (like operator++ would do were it not for skipping some tiles), and then call that Advance() in a loop in operator++ until TileNeedsRaster(current_tile)? https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_perftest.cc:144: "runs/s", Again no need to change if you don't want, but I'm a huge fan of reporting absolute times. Firstly seconds isn't a relevant unit for CC (I hope! ;) ), and second improvements are relative this way. For example, "I improved the frame rate by 5fps" doesn't mean anything (maybe the framerate is already 1000fps), whereas "I improved the frame time by 5ms" will always mean the same thing. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_perftest.cc:188: TEST_F(PictureLayerTilingPerfTest, TilingRasterTileIterator) { Hmm, to represent final performance here, could we create different numbers of layers and then pull a total of 32 tiles out of that set of layers? That's the real performance right?
PTAL. https://codereview.chromium.org/183663003/diff/140001/cc/base/tiling_data.cc File cc/base/tiling_data.cc (right): https://codereview.chromium.org/183663003/diff/140001/cc/base/tiling_data.cc#... cc/base/tiling_data.cc:77: RoundDown(x - 2 * border_texels_, inner_tile_width) / inner_tile_width; On 2014/03/06 02:30:45, enne wrote: > Why? I moved this to a separate patch. It's a bug in the spiral iterator. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:441: TileMap::const_iterator find = tiles_.find(iter.index()); On 2014/03/06 02:30:45, enne wrote: > I realize that technically the iterator is const because you're not modifying > the map, you're modifying something the map points to. It's like using const on > a function that modifies something your object points to. Sure, it's > technically right, but I think it's a confusing communication to other > developers. > > Does const buy you something here? If not, I preferred the previous code. Not really, I just prefer const_iterator when it can be const_iterator to indicate that the map itself will not mutate. I reverted back to non-const. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:559: size_t PictureLayerTiling::RequiredGPUMemoryInBytes() const { On 2014/03/06 10:21:40, epenner wrote: > Could this be replaced with math? If not, or this isn't a significant cost, > maybe just comment why it isn't just math at the moment, for posterity. Do you mean something like visible_tiles_count * some_gpu_usage? If so, I don't think it can. tile->GPUMemoryUsageInBytes is not a constant expression and it depends on how many tile versions have resources. If we decide that RequiredGPUMemoryInBytes should just return visible_tiles_count * one_tile_size, then it can change. I've actually removed this function for now, since it's not being used currently. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.cc:783: while (spiral_iterator_) { On 2014/03/06 10:21:40, epenner wrote: > This is just for advancing past rastered tiles right? It feels sort of awkward > to be done in several places. Feel free to ignore this suggestion if it doesn't > end up helping, but would it work to have an Advance() function that just > advances the iterator one step (like operator++ would do were it not for > skipping some tiles), and then call that Advance() in a loop in operator++ until > TileNeedsRaster(current_tile)? So I was just going to call ++(*this); here in a similar way that Advance would work. The problem is that any form of advance or ++, should call AdvancePhase when it's at the end of the current iterator, and I don't really want this to be recursive. I mean it shouldn't hurt anything, but it's harder to reason about it. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.h:64: bool HasVisibleTiles() const { return type_ == VISIBLE; } On 2014/03/06 02:30:45, enne wrote: > unused? Removed. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.h:271: gfx::Rect current_visible_rect_in_content_space_; On 2014/03/06 02:30:45, enne wrote: > I think it'd be cleaner if you just passed them to the iterators explicitly and > didn't store them on the tiling. The problem is that these are generated during UpdateTilePriorities. The thing that will be creating iterators is the layer, which doesn't know about skewport/eventually rect. It also shouldn't know that, IMHO. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_perftest.cc:144: "runs/s", On 2014/03/06 10:21:40, epenner wrote: > Again no need to change if you don't want, but I'm a huge fan of reporting > absolute times. Firstly seconds isn't a relevant unit for CC (I hope! ;) ), and > second improvements are relative this way. For example, "I improved the frame > rate by 5fps" doesn't mean anything (maybe the framerate is already 1000fps), > whereas "I improved the frame time by 5ms" will always mean the same thing. Hmm, that's a good point. I mean looking at our perf tests, a lot of them report runs/s. Isn't ms/run just a 1000/this number? :P https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_perftest.cc:188: TEST_F(PictureLayerTilingPerfTest, TilingRasterTileIterator) { On 2014/03/06 10:21:40, epenner wrote: > Hmm, to represent final performance here, could we create different numbers of > layers and then pull a total of 32 tiles out of that set of layers? That's the > real performance right? Yeah, but I'd prefer to do that at the LayerRasterTileIterator level, not at the tiling level. This should just run the iterator through all the tiles as fast as it can, I think. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_perftest.cc:188: TEST_F(PictureLayerTilingPerfTest, TilingRasterTileIterator) { On 2014/03/06 02:30:45, enne wrote: > This is a little weird. Isn't this an UpdateTilePriorities test and not a > RasterTileIterator test? This test won't really be here in the end state, right? This tests calls update tile priorities (to create tiles and set the rects), followed by iterating all tiles using raster tile iterator. I think this test would still be here in the end. I can move UpdateTilePriorities outside of the loop and remove "moving" versions of the tests. Would you prefer that?
https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling.h:271: gfx::Rect current_visible_rect_in_content_space_; On 2014/03/07 18:22:23, vmpstr wrote: > On 2014/03/06 02:30:45, enne wrote: > > I think it'd be cleaner if you just passed them to the iterators explicitly > and > > didn't store them on the tiling. > > The problem is that these are generated during UpdateTilePriorities. The thing > that will be creating iterators is the layer, which doesn't know about > skewport/eventually rect. It also shouldn't know that, IMHO. Can you get them off the tiling and pass them to the iterator? There's this weird kind of timing issue, where when those are updated and when you create the iterator are intertwined because you're reaching back and getting data off the tiling that will then change the iterators. I think having to recreate the iterators when those rects change seems cleaner. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_perftest.cc:188: TEST_F(PictureLayerTilingPerfTest, TilingRasterTileIterator) { On 2014/03/07 18:22:23, vmpstr wrote: > On 2014/03/06 02:30:45, enne wrote: > > This is a little weird. Isn't this an UpdateTilePriorities test and not a > > RasterTileIterator test? This test won't really be here in the end state, > right? > > This tests calls update tile priorities (to create tiles and set the rects), > followed by iterating all tiles using raster tile iterator. I think this test > would still be here in the end. > > I can move UpdateTilePriorities outside of the loop and remove "moving" versions > of the tests. Would you prefer that? But in the end state we're not going to iterate through all tiles. We're going to find n tiles, and it's not going to be via some universal UpdateTilePriorities function that updates/finds everything.
On 2014/03/10 21:36:29, enne wrote: > https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... > File cc/resources/picture_layer_tiling.h (right): > > https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... > cc/resources/picture_layer_tiling.h:271: gfx::Rect > current_visible_rect_in_content_space_; > On 2014/03/07 18:22:23, vmpstr wrote: > > On 2014/03/06 02:30:45, enne wrote: > > > I think it'd be cleaner if you just passed them to the iterators explicitly > > and > > > didn't store them on the tiling. > > > > The problem is that these are generated during UpdateTilePriorities. The thing > > that will be creating iterators is the layer, which doesn't know about > > skewport/eventually rect. It also shouldn't know that, IMHO. > > Can you get them off the tiling and pass them to the iterator? Sure. > There's this > weird kind of timing issue, where when those are updated and when you create the > iterator are intertwined because you're reaching back and getting data off the > tiling that will then change the iterators. Yes, that would be bad. However, that's akin to sorting a vector in the midst of iterating it. Calling UpdateTilePriorities invalidates all TilingRasterTileIterators that point to it (at least in the current implementation), so the intertwined case is an invalid use of the iterator. > I think having to recreate the > iterators when those rects change seems cleaner. I'm not sure I agree with this part. Yes, it might be cleaner from the tests and implementation of the iterator, but what about layer side? I'm not sure where these rects would be coming from. One option is the layer, when creating an iterator to one tiling, would first compute the skewport for that tiling in the tiling space. Another option is for the layer to compute the skewport in layer space and just scale those into the tiling. This would mean that the "previous" visible rect is going to move to the layer instead of tiling to determine the extent of the skewport. I mean the skewport is really the only one that is kinda iffy outside of the tiling, so it might be OK. I'll rework this. > > https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... > File cc/resources/picture_layer_tiling_perftest.cc (right): > > https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... > cc/resources/picture_layer_tiling_perftest.cc:188: > TEST_F(PictureLayerTilingPerfTest, TilingRasterTileIterator) { > On 2014/03/07 18:22:23, vmpstr wrote: > > On 2014/03/06 02:30:45, enne wrote: > > > This is a little weird. Isn't this an UpdateTilePriorities test and not a > > > RasterTileIterator test? This test won't really be here in the end state, > > right? > > > > This tests calls update tile priorities (to create tiles and set the rects), > > followed by iterating all tiles using raster tile iterator. I think this test > > would still be here in the end. > > > > I can move UpdateTilePriorities outside of the loop and remove "moving" > versions > > of the tests. Would you prefer that? > > But in the end state we're not going to iterate through all tiles. We're going > to find n tiles, and it's not going to be via some universal > UpdateTilePriorities function that updates/finds everything. I'll rework this as well.
PTAL.
Okay, I take it all back. I think you were right that it would make the next patch more messy if the rects were explicit. Your explanation offline that UpdateTilePriorities would still be the place where these rects got computed and ManageTiles would at least initially invalidate iterators seems pretty sane, so any worries about invalidated iterators seem, uh, also invalid. I'm convinced by your argument, and the previous patch with respect to constructor params was better.
https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_perftest.cc:144: "runs/s", I'm sure you jest, but I'm not sure in which direction. It's indeed trivial to convert the number if that's what you mean. It's just bad UX if 1,000/number or 1,000,000/number is what you always want to know. Or perhaps you meant 1,000,000/number is not trivial to compute in one's head at just a glance, in which case I agree. Anyway, I'm used to runs/s so no worries if we keep it that way. https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... cc/resources/picture_layer_tiling_perftest.cc:188: TEST_F(PictureLayerTilingPerfTest, TilingRasterTileIterator) { I wanted to mention another thing I thought of here. Although we will only pull k=32 tiles, in the steady-state we will walk all rastered tiles. So the real perf should be O(n) based on memory limit (or perhaps maximum skewport/eventually-rect sizes assuming the memory limit is large).
PTAL On 2014/03/11 19:04:50, epennerAtGoogle wrote: > https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... > File cc/resources/picture_layer_tiling_perftest.cc (right): > > https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... > cc/resources/picture_layer_tiling_perftest.cc:144: "runs/s", > I'm sure you jest, but I'm not sure in which direction. > > It's indeed trivial to convert the number if that's what you mean. It's just bad > UX if 1,000/number or 1,000,000/number is what you always want to know. Or > perhaps you meant 1,000,000/number is not trivial to compute in one's head at > just a glance, in which case I agree. I agree that we should possibly switch most of our perf tests to be 1000/number_we_report_right_now ms, but I'd prefer to do that separately for all cc_perftests, so that it's consistent. > > Anyway, I'm used to runs/s so no worries if we keep it that way. > > https://codereview.chromium.org/183663003/diff/140001/cc/resources/picture_la... > cc/resources/picture_layer_tiling_perftest.cc:188: > TEST_F(PictureLayerTilingPerfTest, TilingRasterTileIterator) { > I wanted to mention another thing I thought of here. Although we will only pull > k=32 tiles, in the steady-state we will walk all rastered tiles. So the real > perf should be O(n) based on memory limit (or perhaps maximum > skewport/eventually-rect sizes assuming the memory limit is large). That's a good point. I mean in some respects that's going to be an implementation detail of the iterators. It doesn't _have_ to walk all the tiles to find that none need rasterization. However, I don't really see an easy way of avoiding that right now (short of keeping more state, which I want to avoid at this time). One good thing is that this is meant to allow us to reduce the interest rect significantly, which means that n in O(n) will be smaller. I added two tests, 32 tiles and 64, we can add more just to get a sense of how the time evolves with more tiles.
ping :)
lgtm
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/183663003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
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/183663003/240001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/183663003/240001
Message was sent while issue was closed.
Change committed as 257041 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
