Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
392413002:
cc: Add tiling eviction iterator perftests. (Closed)
Descriptioncc: Add tiling eviction iterator perftests.
This adds both a perftest for constructing the iterator (which is
very cheap) and accessing it, which is fairly heavy.
Initial runs:
[ RUN ] PictureLayerTilingPerfTest.TilingEvictionTileIteratorConstruction
*RESULT tiling_eviction_tile_iterator_construction: 0_0_100x100= 17806804 runs/s
*RESULT tiling_eviction_tile_iterator_construction: 50_0_100x100= 17840996 runs/s
*RESULT tiling_eviction_tile_iterator_construction: 100_0_100x100= 17842676 runs/s
*RESULT tiling_eviction_tile_iterator_construction: 150_0_100x100= 17710480 runs/s
[ OK ] PictureLayerTilingPerfTest.TilingEvictionTileIteratorConstruction (25206 ms)
[ RUN ] PictureLayerTilingPerfTest.TilingEvictionTileIterator
*RESULT tiling_eviction_tile_iterator: 32_100x100= 66903.9296875 runs/s
*RESULT tiling_eviction_tile_iterator: 32_500x500= 64616.3828125 runs/s
*RESULT tiling_eviction_tile_iterator: 64_100x100= 66526.5078125 runs/s
*RESULT tiling_eviction_tile_iterator: 64_500x500= 65154.87109375 runs/s
[ OK ] PictureLayerTilingPerfTest.TilingEvictionTileIterator (8086 ms)
R=reveman
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283975
Patch Set 1 #
Total comments: 10
Patch Set 2 : update #Patch Set 3 : update #
Total comments: 4
Patch Set 4 : update #Patch Set 5 : fix comment typo #Patch Set 6 : fix #Patch Set 7 : rebase #Messages
Total messages: 22 (0 generated)
PTAL. Note that technically there's nothing to evict in these tests, but the cost is still captured. It's a bit more work to get actual resources in here, since the tile manager is the one that creates them. Do you know of a different way of getting resources on the tiles? For the layer test, I will make sure that resources are created.
Why do we want real resources here? If anything, it would seem like we don't want the cost of using real resources included in these test results. Or would the code behave differently with resources? https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling_perftest.cc:154: const int num_tree_priorities = 3; nit: can you remove this and just do "priorities[] = {...}" and use arraysize? https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling_perftest.cc:184: const int num_tree_priorities = 3; nit: remove and use arraysize? https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling_perftest.cc:193: it && count; can you DHCECK(it) instead as the result would not be what we expect if something changes that cause count to not drop to 0? https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling_perftest.cc:207: } It's a bit hard to evaluate this test when the result also includes ctor cost. ie. a change that improves ctor perf but regress iteration perf will look like it only improved ctor perf and iteration perf was unaffected. Unless there's an easy way to make this only test iteration cost I think we should rename it to something like tiling_eviction_tile_iterator_construct_and_iterate to avoid confusion.
PTAL https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling_perftest.cc:154: const int num_tree_priorities = 3; On 2014/07/16 20:17:05, reveman wrote: > nit: can you remove this and just do "priorities[] = {...}" and use arraysize? Done. https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling_perftest.cc:184: const int num_tree_priorities = 3; On 2014/07/16 20:17:05, reveman wrote: > nit: remove and use arraysize? Done. https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling_perftest.cc:193: it && count; On 2014/07/16 20:17:05, reveman wrote: > can you DHCECK(it) instead as the result would not be what we expect if > something changes that cause count to not drop to 0? That's the reason it needs resources. The iterator doesn't return tiles with no resources. So in effect here count is unchanged. The work done by the iterator is equivalent to finding a tile with resources, which it doesn't so it returns false. it uses tile->HasResources(), which under the covers returns true if one of the tile versions' resource_ is not null. Short of using resources, one approach would be to say something like tile->SetHasResourcesForTesting(); but then that would result in a somewhat ugly HasResources implementation: bool HasResources() { if (has_resources_for_testing_) return true; // check tile versions } which would run in non test code as well... Alternatively we can just change the test to do "ASSERT_FALSE(it);" so that it still does the work, but doesn't mislead people into thinking this returns something. Wdyt? https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling_perftest.cc:207: } On 2014/07/16 20:17:05, reveman wrote: > It's a bit hard to evaluate this test when the result also includes ctor cost. > ie. a change that improves ctor perf but regress iteration perf will look like > it only improved ctor perf and iteration perf was unaffected. > > Unless there's an easy way to make this only test iteration cost I think we > should rename it to something like > tiling_eviction_tile_iterator_construct_and_iterate to avoid confusion. I've changed the name, since I think it's kind of hard to separate construction, and then do repeated iteration
https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling_perftest.cc:193: it && count; On 2014/07/16 20:35:22, vmpstr wrote: > On 2014/07/16 20:17:05, reveman wrote: > > can you DHCECK(it) instead as the result would not be what we expect if > > something changes that cause count to not drop to 0? > > That's the reason it needs resources. The iterator doesn't return tiles with no > resources. So in effect here count is unchanged. The work done by the iterator > is equivalent to finding a tile with resources, which it doesn't so it returns > false. > > it uses tile->HasResources(), which under the covers returns true if one of the > tile versions' resource_ is not null. > > Short of using resources, one approach would be to say something like > tile->SetHasResourcesForTesting(); but then that would result in a somewhat ugly > HasResources implementation: > > bool HasResources() { > if (has_resources_for_testing_) > return true; > // check tile versions > } > > which would run in non test code as well... > > Alternatively we can just change the test to do "ASSERT_FALSE(it);" so that it > still does the work, but doesn't mislead people into thinking this returns > something. > > Wdyt? Can you set it to a fake resource? It seems better to mimic the real usage as close as possible and not assume that the implementation does basically the same when no resources are set.
PTAL https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/392413002/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling_perftest.cc:193: it && count; On 2014/07/16 22:03:34, reveman wrote: > On 2014/07/16 20:35:22, vmpstr wrote: > > On 2014/07/16 20:17:05, reveman wrote: > > > can you DHCECK(it) instead as the result would not be what we expect if > > > something changes that cause count to not drop to 0? > > > > That's the reason it needs resources. The iterator doesn't return tiles with > no > > resources. So in effect here count is unchanged. The work done by the iterator > > is equivalent to finding a tile with resources, which it doesn't so it returns > > false. > > > > it uses tile->HasResources(), which under the covers returns true if one of > the > > tile versions' resource_ is not null. > > > > Short of using resources, one approach would be to say something like > > tile->SetHasResourcesForTesting(); but then that would result in a somewhat > ugly > > HasResources implementation: > > > > bool HasResources() { > > if (has_resources_for_testing_) > > return true; > > // check tile versions > > } > > > > which would run in non test code as well... > > > > Alternatively we can just change the test to do "ASSERT_FALSE(it);" so that it > > still does the work, but doesn't mislead people into thinking this returns > > something. > > > > Wdyt? > > Can you set it to a fake resource? It seems better to mimic the real usage as > close as possible and not assume that the implementation does basically the same > when no resources are set. Done.
lgtm with nits https://codereview.chromium.org/392413002/diff/40001/cc/resources/picture_lay... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/392413002/diff/40001/cc/resources/picture_lay... cc/resources/picture_layer_tiling_perftest.cc:204: for (PictureLayerTiling::TilingRasterTileIterator it( nit: I think it's a bit awkward to use an iterator to prepare testing of an iterator. even more so when making the raster iterator above do the same thing. please use PictureLayerTiling::AllTilesForTesting here instead. https://codereview.chromium.org/392413002/diff/40001/cc/resources/picture_lay... cc/resources/picture_layer_tiling_perftest.cc:225: ASSERT_EQ(0, count); nit: same suggestion as the other patch, maybe replace the 5 lines above with: while (count--) { ASSERT_TRUE(it); ASSERT_TRUE(*it != NULL); ++it; }
https://codereview.chromium.org/392413002/diff/40001/cc/resources/picture_lay... File cc/resources/picture_layer_tiling_perftest.cc (right): https://codereview.chromium.org/392413002/diff/40001/cc/resources/picture_lay... cc/resources/picture_layer_tiling_perftest.cc:204: for (PictureLayerTiling::TilingRasterTileIterator it( On 2014/07/17 16:26:23, reveman wrote: > nit: I think it's a bit awkward to use an iterator to prepare testing of an > iterator. even more so when making the raster iterator above do the same thing. > please use PictureLayerTiling::AllTilesForTesting here instead. Done. https://codereview.chromium.org/392413002/diff/40001/cc/resources/picture_lay... cc/resources/picture_layer_tiling_perftest.cc:225: ASSERT_EQ(0, count); On 2014/07/17 16:26:23, reveman wrote: > nit: same suggestion as the other patch, maybe replace the 5 lines above with: > > while (count--) { > ASSERT_TRUE(it); > ASSERT_TRUE(*it != NULL); > ++it; > } 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/392413002/80001
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/392413002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
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/392413002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
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/392413002/120001
Message was sent while issue was closed.
Change committed as 283975 |
