Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1531013004:
cc: Do solid color analysis before scheduling tiles. (Closed)
Descriptioncc: Do solid color analysis before scheduling tiles.
Instead of creating separate analysis task in raster thread for
solid color detection, we do it while scheduling and rasterization
in cc thread itself. This would save us the thread overhead etc.
BUG=553612
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/5b2f22a73e08684db17a2b86a4c2db2ae6ba81ee
Cr-Commit-Position: refs/heads/master@{#369263}
Patch Set 1 #
Total comments: 8
Patch Set 2 : move tile check while assigning mem. #
Total comments: 2
Patch Set 3 : cleanup. #
Total comments: 10
Patch Set 4 : review comments updated. #
Total comments: 2
Patch Set 5 : add test. #
Total comments: 8
Patch Set 6 : review comments updated. #
Total comments: 1
Patch Set 7 : use non-solid color for ActivateAndDrawWhenOOM test. #
Total comments: 10
Patch Set 8 : notifytilestate change before skipping raster. #
Total comments: 3
Patch Set 9 : test updated. #
Messages
Total messages: 44 (14 generated)
Description was changed from ========== cc: Do solid color analysis during scheduling tiles. BUG=553612 ========== to ========== cc: Do solid color analysis during scheduling tiles. BUG=553612 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
sohan.jyoti@samsung.com changed reviewers: + vmpstr@chromium.org
Vlad@, sorry got caught up elsewhere. Can you please have a look, if this is what we want ? I'll run the tests if you confirm. Anything in telemetry? thanks.
https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... File cc/playback/display_list_raster_source.cc (right): https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... cc/playback/display_list_raster_source.cc:220: bool DisplayListRasterSource::IsSolidColorTile(gfx::Rect content_rect, Can you use PerformSolidColorAnalysis instead? That is, this function is way too similar to it, so I don't think we should add it. https://codereview.chromium.org/1531013004/diff/1/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:720: bool is_solid_color = prioritized_tile.raster_source()->IsSolidColorTile( This is an OK spot to do this. When I was thinking about it, I wanted to put it in AssignGpuMemory, since if it's solid color we can schedule an extra tile for raster. At this point, we can only reduce the list of tiles, we can't fill the available spots with new tiles. https://codereview.chromium.org/1531013004/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:863: if (draw_info.mode() == TileDrawInfo::SOLID_COLOR_MODE) { This shouldn't be possible anymore, right? UpdateTileDrawInfo shouldn't take analysis anymore.
Please take a look, thanks. https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... File cc/playback/display_list_raster_source.cc (right): https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... cc/playback/display_list_raster_source.cc:220: bool DisplayListRasterSource::IsSolidColorTile(gfx::Rect content_rect, On 2015/12/17 18:55:47, vmpstr wrote: > Can you use PerformSolidColorAnalysis instead? That is, this function is way too > similar to it, so I don't think we should add it. Yes, i thought we would remove PerformSolidColorAnalysis at a later patchset, once we modify the tests. Or you want to use PerformSolidColorAnalysis as function name here ? https://codereview.chromium.org/1531013004/diff/1/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:720: bool is_solid_color = prioritized_tile.raster_source()->IsSolidColorTile( On 2015/12/17 18:55:47, vmpstr wrote: > This is an OK spot to do this. When I was thinking about it, I wanted to put it > in AssignGpuMemory, since if it's solid color we can schedule an extra tile for > raster. At this point, we can only reduce the list of tiles, we can't fill the > available spots with new tiles. Done. https://codereview.chromium.org/1531013004/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:863: if (draw_info.mode() == TileDrawInfo::SOLID_COLOR_MODE) { On 2015/12/17 18:55:47, vmpstr wrote: > This shouldn't be possible anymore, right? UpdateTileDrawInfo shouldn't take > analysis anymore. Acknowledged.
Description was changed from ========== cc: Do solid color analysis during scheduling tiles. BUG=553612 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Do solid color analysis before scheduling tiles. BUG=553612 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
There are a bunch of things that could be cleaned up in this patch as well, but I think this is enough to do tests with. I'm not sure what the best test here is, but I would start with a set of telemetry tests like smoothness and thread_times. If you have other ideas of how to measure impact, that would be great as well. https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... File cc/playback/display_list_raster_source.cc (right): https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... cc/playback/display_list_raster_source.cc:220: bool DisplayListRasterSource::IsSolidColorTile(gfx::Rect content_rect, On 2015/12/18 12:50:00, sohanjg wrote: > On 2015/12/17 18:55:47, vmpstr wrote: > > Can you use PerformSolidColorAnalysis instead? That is, this function is way > too > > similar to it, so I don't think we should add it. > > Yes, i thought we would remove PerformSolidColorAnalysis at a later patchset, > once we modify the tests. > > Or you want to use PerformSolidColorAnalysis as function name here ? > I don't a rename, it's just this is the patch that should clean up other instances. My thought is that we should actually use PerformSolidColorAnalysis instead of this function. Note that the method for analyzing is slightly different: this one does a scale on the matrix, the other one scales the rect to analyze instead. I would prefer we keep the current approach to minimize changes.
On 2015/12/18 19:03:06, vmpstr wrote: > There are a bunch of things that could be cleaned up in this patch as well, but > I think this is enough to do tests with. > > I'm not sure what the best test here is, but I would start with a set of > telemetry tests like smoothness and thread_times. If you have other ideas of how > to measure impact, that would be great as well. > > https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... > File cc/playback/display_list_raster_source.cc (right): > > https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... > cc/playback/display_list_raster_source.cc:220: bool > DisplayListRasterSource::IsSolidColorTile(gfx::Rect content_rect, > On 2015/12/18 12:50:00, sohanjg wrote: > > On 2015/12/17 18:55:47, vmpstr wrote: > > > Can you use PerformSolidColorAnalysis instead? That is, this function is way > > too > > > similar to it, so I don't think we should add it. > > > > Yes, i thought we would remove PerformSolidColorAnalysis at a later patchset, > > once we modify the tests. > > > > Or you want to use PerformSolidColorAnalysis as function name here ? > > > > I don't a rename, it's just this is the patch that should clean up other > instances. My thought is that we should actually use PerformSolidColorAnalysis > instead of this function. Note that the method for analyzing is slightly > different: this one does a scale on the matrix, the other one scales the rect to > analyze instead. I would prefer we keep the current approach to minimize > changes. I took benchmark data for thread times in key mobile site smooth and smoothness in top 25. There are deterioration with patch. I am attaching in the bug (https://code.google.com/p/chromium/issues/detail?id=553612). Please take a look.
I think it's worth cleaning this patch up and landing it. I would still prefer that this patch leaves only one of PerformSolidColorAnalysis and IsSolidColor functions. https://codereview.chromium.org/1531013004/diff/20001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:583: SkColor color = SK_ColorTRANSPARENT; Leave a comment here please
Patchset #3 (id:40001) has been deleted
Please take a look. We can cleanup SolidColorAnalysis usage completely, should we do it here or on a seperate CL ? Also, reg the trace you mentioned in bug with and w/o patch, you mean for sites like weather.com,news yahoo etc.? where it shows some degradation ? https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... File cc/playback/display_list_raster_source.cc (right): https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... cc/playback/display_list_raster_source.cc:220: bool DisplayListRasterSource::IsSolidColorTile(gfx::Rect content_rect, On 2015/12/18 19:03:06, vmpstr wrote: > On 2015/12/18 12:50:00, sohanjg wrote: > > On 2015/12/17 18:55:47, vmpstr wrote: > > > Can you use PerformSolidColorAnalysis instead? That is, this function is way > > too > > > similar to it, so I don't think we should add it. > > > > Yes, i thought we would remove PerformSolidColorAnalysis at a later patchset, > > once we modify the tests. > > > > Or you want to use PerformSolidColorAnalysis as function name here ? > > > > I don't a rename, it's just this is the patch that should clean up other > instances. My thought is that we should actually use PerformSolidColorAnalysis > instead of this function. Note that the method for analyzing is slightly > different: this one does a scale on the matrix, the other one scales the rect to > analyze instead. I would prefer we keep the current approach to minimize > changes. Acknowledged. https://codereview.chromium.org/1531013004/diff/20001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:583: SkColor color = SK_ColorTRANSPARENT; On 2015/12/28 21:22:32, vmpstr wrote: > Leave a comment here please Done.
On 2015/12/29 14:29:47, sohanjg wrote: > Please take a look. > We can cleanup SolidColorAnalysis usage completely, should we do it here or on a > seperate CL ? Let's do it here. > > Also, reg the trace you mentioned in bug with and w/o patch, you mean for sites > like weather.com,news yahoo etc.? where it shows some degradation ? Just a regular trace where there are some solid tiles. It doesn't have to show improvement or degradation just what it looks like now. Although maybe a trace from a site that has mostly solid colors would be nice as well. > > https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... > File cc/playback/display_list_raster_source.cc (right): > > https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_ra... > cc/playback/display_list_raster_source.cc:220: bool > DisplayListRasterSource::IsSolidColorTile(gfx::Rect content_rect, > On 2015/12/18 19:03:06, vmpstr wrote: > > On 2015/12/18 12:50:00, sohanjg wrote: > > > On 2015/12/17 18:55:47, vmpstr wrote: > > > > Can you use PerformSolidColorAnalysis instead? That is, this function is > way > > > too > > > > similar to it, so I don't think we should add it. > > > > > > Yes, i thought we would remove PerformSolidColorAnalysis at a later > patchset, > > > once we modify the tests. > > > > > > Or you want to use PerformSolidColorAnalysis as function name here ? > > > > > > > I don't a rename, it's just this is the patch that should clean up other > > instances. My thought is that we should actually use PerformSolidColorAnalysis > > instead of this function. Note that the method for analyzing is slightly > > different: this one does a scale on the matrix, the other one scales the rect > to > > analyze instead. I would prefer we keep the current approach to minimize > > changes. > > Acknowledged. > > https://codereview.chromium.org/1531013004/diff/20001/cc/tiles/tile_manager.cc > File cc/tiles/tile_manager.cc (right): > > https://codereview.chromium.org/1531013004/diff/20001/cc/tiles/tile_manager.c... > cc/tiles/tile_manager.cc:583: SkColor color = SK_ColorTRANSPARENT; > On 2015/12/28 21:22:32, vmpstr wrote: > > Leave a comment here please > > Done. https://codereview.chromium.org/1531013004/diff/60001/cc/debug/rasterize_and_... File cc/debug/rasterize_and_record_benchmark_impl.cc (right): https://codereview.chromium.org/1531013004/diff/60001/cc/debug/rasterize_and_... cc/debug/rasterize_and_record_benchmark_impl.cc:52: *is_solid_color = raster_source->PerformSolidColorAnalysis( I'm not sure what to do in this case. This is measuring the raster speed. So, if solid color analysis is no longer a part of raster, then maybe it should move outside. That is, raster will now not consider solid color tiles. This will affect benchmarks, but reflect the updated behavior. I think things like thread times benchmarks still capture all of the behavior changes. https://codereview.chromium.org/1531013004/diff/60001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (left): https://codereview.chromium.org/1531013004/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:658: flags = Tile::USE_PICTURE_ANALYSIS; We should keep respecting this flag, since it would break solid color masks https://codereview.chromium.org/1531013004/diff/60001/cc/playback/display_lis... File cc/playback/display_list_raster_source.h (left): https://codereview.chromium.org/1531013004/diff/60001/cc/playback/display_lis... cc/playback/display_list_raster_source.h:25: struct CC_EXPORT SolidColorAnalysis { This can go away too right? https://codereview.chromium.org/1531013004/diff/60001/cc/tiles/tile.h File cc/tiles/tile.h (left): https://codereview.chromium.org/1531013004/diff/60001/cc/tiles/tile.h#oldcode64 cc/tiles/tile.h:64: bool use_picture_analysis() const { Keep this https://codereview.chromium.org/1531013004/diff/60001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:570: SkColor color = SK_ColorTRANSPARENT; You'll need to skip this block if use_picture_analysis is false.
Please take a look, thanks. https://codereview.chromium.org/1531013004/diff/60001/cc/debug/rasterize_and_... File cc/debug/rasterize_and_record_benchmark_impl.cc (right): https://codereview.chromium.org/1531013004/diff/60001/cc/debug/rasterize_and_... cc/debug/rasterize_and_record_benchmark_impl.cc:52: *is_solid_color = raster_source->PerformSolidColorAnalysis( On 2015/12/29 21:24:23, vmpstr wrote: > I'm not sure what to do in this case. This is measuring the raster speed. So, if > solid color analysis is no longer a part of raster, then maybe it should move > outside. > > That is, raster will now not consider solid color tiles. This will affect > benchmarks, but reflect the updated behavior. I think things like thread times > benchmarks still capture all of the behavior changes. We are always issuing PlaybackToCanvas which wouldn't check for solid or normal content. Only while assigning memory to tiles that we change the logic, so we will still raster them as before, isnt it ? is_solid_color will only be used for calculating the number of non -solid tiles rendered. i think it will be same as inside or out of the do-while loop, and with or without our changes. But, yes we can save some extra analysis call if we move out. https://codereview.chromium.org/1531013004/diff/60001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (left): https://codereview.chromium.org/1531013004/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:658: flags = Tile::USE_PICTURE_ANALYSIS; On 2015/12/29 21:24:23, vmpstr wrote: > We should keep respecting this flag, since it would break solid color masks Acknowledged. https://codereview.chromium.org/1531013004/diff/60001/cc/playback/display_lis... File cc/playback/display_list_raster_source.h (left): https://codereview.chromium.org/1531013004/diff/60001/cc/playback/display_lis... cc/playback/display_list_raster_source.h:25: struct CC_EXPORT SolidColorAnalysis { On 2015/12/29 21:24:23, vmpstr wrote: > This can go away too right? Done. https://codereview.chromium.org/1531013004/diff/60001/cc/tiles/tile.h File cc/tiles/tile.h (left): https://codereview.chromium.org/1531013004/diff/60001/cc/tiles/tile.h#oldcode64 cc/tiles/tile.h:64: bool use_picture_analysis() const { On 2015/12/29 21:24:23, vmpstr wrote: > Keep this Acknowledged. https://codereview.chromium.org/1531013004/diff/60001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:570: SkColor color = SK_ColorTRANSPARENT; On 2015/12/29 21:24:23, vmpstr wrote: > You'll need to skip this block if use_picture_analysis is false. Done.
This looks good. Can you see if you add a test somewhere that ensures that if we have solid tiles then we don't produce raster tasks? https://codereview.chromium.org/1531013004/diff/80001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/80001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:564: if (tile->use_picture_analysis()) { && kUseColorEstimator
Sorry for the delay on this, was OOO. Please take a look, thanks. https://codereview.chromium.org/1531013004/diff/80001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/80001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:564: if (tile->use_picture_analysis()) { On 2015/12/30 20:42:38, vmpstr wrote: > && kUseColorEstimator Done.
Can you describe the CL a bit more in the description field please? https://codereview.chromium.org/1531013004/diff/100001/cc/playback/display_li... File cc/playback/display_list_raster_source.h (right): https://codereview.chromium.org/1531013004/diff/100001/cc/playback/display_li... cc/playback/display_list_raster_source.h:50: bool PerformSolidColorAnalysis(const gfx::Rect& content_rect, nit: Can you make a comment about the return value? https://codereview.chromium.org/1531013004/diff/100001/cc/tiles/tile_manager_... File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/100001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1466: scoped_refptr<DisplayListRasterSource> raster = nit: raster_source https://codereview.chromium.org/1531013004/diff/100001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1473: scoped_ptr<PictureLayerImpl> layer = nit: layer_impl https://codereview.chromium.org/1531013004/diff/100001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1490: EXPECT_FALSE(tile->HasRasterTask()); can you also EXPECT_EQ(TileDrawInfo::SOLID_COLOR_MODE, tile->draw_info().mode()); and EXPECT_EQ(solid_color, tile->draw_info().solid_color());
Description was changed from ========== cc: Do solid color analysis before scheduling tiles. BUG=553612 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Do solid color analysis before scheduling tiles. Instead of creating separate analysis task in raster thread for solid color detection, we do it while scheduling and rasterization in cc thread itself. This would save us the thread overhead etc. BUG=553612 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Please take a look, thanks. https://codereview.chromium.org/1531013004/diff/100001/cc/playback/display_li... File cc/playback/display_list_raster_source.h (right): https://codereview.chromium.org/1531013004/diff/100001/cc/playback/display_li... cc/playback/display_list_raster_source.h:50: bool PerformSolidColorAnalysis(const gfx::Rect& content_rect, On 2016/01/06 19:35:03, vmpstr wrote: > nit: Can you make a comment about the return value? Done. https://codereview.chromium.org/1531013004/diff/100001/cc/tiles/tile_manager_... File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/100001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1466: scoped_refptr<DisplayListRasterSource> raster = On 2016/01/06 19:35:03, vmpstr wrote: > nit: raster_source Done. https://codereview.chromium.org/1531013004/diff/100001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1473: scoped_ptr<PictureLayerImpl> layer = On 2016/01/06 19:35:03, vmpstr wrote: > nit: layer_impl Done. https://codereview.chromium.org/1531013004/diff/100001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1490: EXPECT_FALSE(tile->HasRasterTask()); On 2016/01/06 19:35:03, vmpstr wrote: > can you also > > EXPECT_EQ(TileDrawInfo::SOLID_COLOR_MODE, tile->draw_info().mode()); > > and > > EXPECT_EQ(solid_color, tile->draw_info().solid_color()); Done.
The CQ bit was checked by sohan.jyoti@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531013004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531013004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Can you give your opinion for this ? https://codereview.chromium.org/1531013004/diff/120001/cc/tiles/tile_manager_... File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/120001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1480: PictureLayerTiling* tiling = tiling_set->AddTiling(1.0f, raster_source); this causes a dcheck fail for solid color raster source, in PLT creation :/ https://code.google.com/p/chromium/codesearch#chromium/src/cc/tiles/picture_l...
I have updated the ActivateAndDrawWhenOOM test, it was getting solid color src and notifytilestate was failing. Please take a look, and provide your opinion for NoRasterTasksforSolidColorTiles test failing in solid color check in PLT create. thanks.
ericrk@chromium.org changed reviewers: + ericrk@chromium.org
https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:576: prioritized_tile.raster_source()->PerformSolidColorAnalysis( There's a comment in the bug that we might be spending significant time in canvas creation, and one benefit of moving solid color checks to a tight loop is that we could use a shared canvas which is reset between checks (rather than re-created each time). Did you look into this at all? We don't need to address this now, but might be good to add a TODO to investigate.
https://codereview.chromium.org/1531013004/diff/140001/cc/playback/display_li... File cc/playback/display_list_raster_source_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/140001/cc/playback/display_li... cc/playback/display_list_raster_source_unittest.cc:67: is_solid_color = false; nit: you don't need to reset this to false, since you assign it unconditionally below (here and throughout these tests) https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:579: tile->draw_info().set_solid_color(color); nit: call tile->draw_info().set_was_ever_ready_to_draw(); please. I think we also have to call client_->NotifyTileStateChanged(tile); These are all the functions that would have happened in UpdateTileDrawInfo https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:843: UpdateTileDrawInfo(tile, resource); nit: since a lot of the logic of UpdateTileDrawInfo is gone, maybe we can just move the code here and delete the function. Up to you if you think it's worth it. https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager_... File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1622: TEST_F(TileManagerTest, ActivateAndDrawWhenOOM) { It's actually kind of nice that this fails now, since solid color tiles don't cause OOM anymore (they don't require memory). Before, we would have to create resources before even figuring out whether tiles were solid.
Please take a look, thanks. https://codereview.chromium.org/1531013004/diff/140001/cc/playback/display_li... File cc/playback/display_list_raster_source_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/140001/cc/playback/display_li... cc/playback/display_list_raster_source_unittest.cc:67: is_solid_color = false; On 2016/01/07 22:36:02, vmpstr wrote: > nit: you don't need to reset this to false, since you assign it unconditionally > below (here and throughout these tests) Done. https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:576: prioritized_tile.raster_source()->PerformSolidColorAnalysis( On 2016/01/07 18:18:10, ericrk wrote: > There's a comment in the bug that we might be spending significant time in > canvas creation, and one benefit of moving solid color checks to a tight loop is > that we could use a shared canvas which is reset between checks (rather than > re-created each time). Did you look into this at all? We don't need to address > this now, but might be good to add a TODO to investigate. Acknowledged. https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:579: tile->draw_info().set_solid_color(color); On 2016/01/07 22:36:02, vmpstr wrote: > nit: call tile->draw_info().set_was_ever_ready_to_draw(); please. I think we > also have to call client_->NotifyTileStateChanged(tile); > > These are all the functions that would have happened in UpdateTileDrawInfo Done. https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:843: UpdateTileDrawInfo(tile, resource); On 2016/01/07 22:36:02, vmpstr wrote: > nit: since a lot of the logic of UpdateTileDrawInfo is gone, maybe we can just > move the code here and delete the function. Up to you if you think it's worth > it. Done. https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager_... File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1622: TEST_F(TileManagerTest, ActivateAndDrawWhenOOM) { On 2016/01/07 22:36:02, vmpstr wrote: > It's actually kind of nice that this fails now, since solid color tiles don't > cause OOM anymore (they don't require memory). Before, we would have to create > resources before even figuring out whether tiles were solid. OK. i will remove these changes to draw a non-solid src. As we are now notifying tile state changed while skipping solid color, this will not fail anymore. https://codereview.chromium.org/1531013004/diff/160001/cc/tiles/tile_manager_... File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/160001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1480: PictureLayerTiling* tiling = tiling_set->AddTiling(1.0f, raster_source); This would fail the dchecks, where we test for solid color src, we shouldnt have tilings. Now that we don't separately analyse and continue to raster. Should we remove the dchecks for avoiding solid color tilings ?
lgtm % ericrk https://codereview.chromium.org/1531013004/diff/160001/cc/tiles/tile_manager_... File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/160001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1480: PictureLayerTiling* tiling = tiling_set->AddTiling(1.0f, raster_source); On 2016/01/08 11:47:56, sohanjg wrote: > This would fail the dchecks, where we test for solid color src, we shouldnt have > tilings. > Now that we don't separately analyse and continue to raster. Should we remove > the dchecks for avoiding solid color tilings ? No, these DCHECKs should stay. The solid color raster source has to do with solid color layer, not solid color tiles (that is if the whole layer is solid then we shouldn't be creatig tilings)
Updated the test, can you please take a look, thanks. https://codereview.chromium.org/1531013004/diff/160001/cc/tiles/tile_manager_... File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/160001/cc/tiles/tile_manager_... cc/tiles/tile_manager_unittest.cc:1480: PictureLayerTiling* tiling = tiling_set->AddTiling(1.0f, raster_source); On 2016/01/08 18:51:41, vmpstr wrote: > On 2016/01/08 11:47:56, sohanjg wrote: > > This would fail the dchecks, where we test for solid color src, we shouldnt > have > > tilings. > > Now that we don't separately analyse and continue to raster. Should we remove > > the dchecks for avoiding solid color tilings ? > > No, these DCHECKs should stay. The solid color raster source has to do with > solid color layer, not solid color tiles (that is if the whole layer is solid > then we shouldn't be creatig tilings) Acknowledged.
The CQ bit was checked by sohan.jyoti@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531013004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531013004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/01/11 12:49:00, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Eric, can you pls take a look. thanks.
On 2016/01/12 05:07:17, sohanjg wrote: > On 2016/01/11 12:49:00, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > Eric, can you pls take a look. thanks. Thanks for the comment - LGTM
On 2016/01/12 22:12:58, ericrk wrote: > On 2016/01/12 05:07:17, sohanjg wrote: > > On 2016/01/11 12:49:00, commit-bot: I haz the power wrote: > > > Dry run: This issue passed the CQ dry run. > > > > Eric, can you pls take a look. thanks. > > Thanks for the comment - LGTM Vlad, can you pls take a look before we cq it. Thanks. I have updated the tests a bit, frm d last time u checked.
The CQ bit was checked by sohan.jyoti@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1531013004/#ps180001 (title: "test updated.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531013004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531013004/180001
Message was sent while issue was closed.
Description was changed from ========== cc: Do solid color analysis before scheduling tiles. Instead of creating separate analysis task in raster thread for solid color detection, we do it while scheduling and rasterization in cc thread itself. This would save us the thread overhead etc. BUG=553612 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Do solid color analysis before scheduling tiles. Instead of creating separate analysis task in raster thread for solid color detection, we do it while scheduling and rasterization in cc thread itself. This would save us the thread overhead etc. BUG=553612 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== cc: Do solid color analysis before scheduling tiles. Instead of creating separate analysis task in raster thread for solid color detection, we do it while scheduling and rasterization in cc thread itself. This would save us the thread overhead etc. BUG=553612 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Do solid color analysis before scheduling tiles. Instead of creating separate analysis task in raster thread for solid color detection, we do it while scheduling and rasterization in cc thread itself. This would save us the thread overhead etc. BUG=553612 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/5b2f22a73e08684db17a2b86a4c2db2ae6ba81ee Cr-Commit-Position: refs/heads/master@{#369263} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5b2f22a73e08684db17a2b86a4c2db2ae6ba81ee Cr-Commit-Position: refs/heads/master@{#369263} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
