Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
364063005:
Histogram to track missing and incomplete tiles (Closed)
DescriptionHistogram to track missing and incomplete tiles
Keeps track of missing and incomplete tiles to measure ugliness during
scrolling.
BUG=381695
TEST=cc_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283009
Patch Set 1 #
Total comments: 9
Patch Set 2 : address review comments #
Total comments: 11
Patch Set 3 : rebase #Patch Set 4 : address review comments #
Total comments: 2
Patch Set 5 : review comment addressed and unit test fixed #Patch Set 6 : rebase #Patch Set 7 : rebase #Patch Set 8 : rm confusing missing_tiles++ #
Messages
Total messages: 28 (0 generated)
cc: danakj histogram: asvitkine
https://codereview.chromium.org/364063005/diff/1/cc/layers/append_quads_data.h File cc/layers/append_quads_data.h (right): https://codereview.chromium.org/364063005/diff/1/cc/layers/append_quads_data.... cc/layers/append_quads_data.h:15: : had_incomplete_tile(false), do we need this one anymore? Just check if num is > 0? https://codereview.chromium.org/364063005/diff/1/cc/trees/layer_tree_host_imp... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/364063005/diff/1/cc/trees/layer_tree_host_imp... cc/trees/layer_tree_host_impl.cc:2241: is_scrolling_ = true; can you use the CurrentlyScrollingLayer() instead of adding a bool?
LGTM % comments https://codereview.chromium.org/364063005/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/364063005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:24597: +<histogram name="RenderPass.AppendQuadData.NumIncompleteTiles"> There's already a "Compositing.*" namespace. Would it make sense to put these under there instead of introducing "RenderPass.*"? https://codereview.chromium.org/364063005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:24601: + scrolling. This is a rough measurement of ugliness during user interaction. Please say explicitly when this gets sampled. i.e. something like "A sample is recorded every time a frame is drawn while a scroll is in progress." or whatever the correct description is. Same for the one below.
https://codereview.chromium.org/364063005/diff/1/cc/layers/append_quads_data.h File cc/layers/append_quads_data.h (right): https://codereview.chromium.org/364063005/diff/1/cc/layers/append_quads_data.... cc/layers/append_quads_data.h:15: : had_incomplete_tile(false), On 2014/07/03 16:01:45, danakj wrote: > do we need this one anymore? Just check if num is > 0? Done. https://codereview.chromium.org/364063005/diff/1/cc/trees/layer_tree_host_imp... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/364063005/diff/1/cc/trees/layer_tree_host_imp... cc/trees/layer_tree_host_impl.cc:2241: is_scrolling_ = true; On 2014/07/03 16:01:45, danakj wrote: > can you use the CurrentlyScrollingLayer() instead of adding a bool? Using IsCurrentlyScrolling() https://codereview.chromium.org/364063005/diff/1/cc/trees/layer_tree_host_imp... cc/trees/layer_tree_host_impl.cc:2241: is_scrolling_ = true; On 2014/07/03 16:01:45, danakj wrote: > can you use the CurrentlyScrollingLayer() instead of adding a bool? Done. https://codereview.chromium.org/364063005/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/364063005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:24597: +<histogram name="RenderPass.AppendQuadData.NumIncompleteTiles"> On 2014/07/04 15:40:27, asvitkine (OOO back July 14th) wrote: > There's already a "Compositing.*" namespace. Would it make sense to put these > under there instead of introducing "RenderPass.*"? Done. https://codereview.chromium.org/364063005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:24601: + scrolling. This is a rough measurement of ugliness during user interaction. On 2014/07/04 15:40:27, asvitkine (OOO back July 14th) wrote: > Please say explicitly when this gets sampled. i.e. something like "A sample is > recorded every time a frame is drawn while a scroll is in progress." or whatever > the correct description is. Same for the one below. Done.
https://codereview.chromium.org/364063005/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/364063005/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:283: if (iter->contents_scale() != ideal_contents_scale_) { nit: don't need to add {} https://codereview.chromium.org/364063005/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:361: append_quads_data->num_incomplete_tiles++; this means we're double counting missing tiles as incomplete tiles tho, now, how about we don't add to incomplete here, and we change checks for incomplete to check for incomplete||missing? https://codereview.chromium.org/364063005/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/364063005/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:870: if (append_quads_data.num_incomplete_tiles > 0) { you can still just do if (append_quads_data.num_incomplete_tiles) .. tho, i think we should do if (append_quads_data.num_incomplete_tiles || append_quads_data.num_missing_tiles) as per earlier comment https://codereview.chromium.org/364063005/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/364063005/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:2737: + Keeps track of number of incomplete tiles in RenderPass during DrawQuad s/in RenderPass during DrawQuad stage/in a drawn compositor frame/
https://codereview.chromium.org/364063005/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/364063005/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:870: if (append_quads_data.num_incomplete_tiles > 0) { On 2014/07/07 20:06:28, danakj wrote: > you can still just do if (append_quads_data.num_incomplete_tiles) > > .. tho, i think we should do > > if (append_quads_data.num_incomplete_tiles || > append_quads_data.num_missing_tiles) > > as per earlier comment Do we still want to sum missing_tiles too? As in adding line: num_incomplete_tiles += aqd.num_missing_tiles; ?
https://codereview.chromium.org/364063005/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/364063005/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:870: if (append_quads_data.num_incomplete_tiles > 0) { On 2014/07/07 21:50:09, weiliangc wrote: > On 2014/07/07 20:06:28, danakj wrote: > > you can still just do if (append_quads_data.num_incomplete_tiles) > > > > .. tho, i think we should do > > > > if (append_quads_data.num_incomplete_tiles || > > append_quads_data.num_missing_tiles) > > > > as per earlier comment > > Do we still want to sum missing_tiles too? As in adding line: > num_incomplete_tiles += aqd.num_missing_tiles; > ? > I don't think so, then you'd double count them again cuz you wanna know just how many tiles were the wrong res for this number right?
https://codereview.chromium.org/364063005/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/364063005/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:870: if (append_quads_data.num_incomplete_tiles > 0) { On 2014/07/07 21:52:01, danakj wrote: > On 2014/07/07 21:50:09, weiliangc wrote: > > On 2014/07/07 20:06:28, danakj wrote: > > > you can still just do if (append_quads_data.num_incomplete_tiles) > > > > > > .. tho, i think we should do > > > > > > if (append_quads_data.num_incomplete_tiles || > > > append_quads_data.num_missing_tiles) > > > > > > as per earlier comment > > > > Do we still want to sum missing_tiles too? As in adding line: > > num_incomplete_tiles += aqd.num_missing_tiles; > > ? > > > > I don't think so, then you'd double count them again cuz you wanna know just how > many tiles were the wrong res for this number right? Ah, I forgot the parts after += num_incomplete_tiles need to execute when missing tiles. Got it. Thanks!
https://codereview.chromium.org/364063005/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/364063005/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:283: if (iter->contents_scale() != ideal_contents_scale_) { On 2014/07/07 20:06:27, danakj wrote: > nit: don't need to add {} Done. https://codereview.chromium.org/364063005/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:361: append_quads_data->num_incomplete_tiles++; On 2014/07/07 20:06:28, danakj wrote: > this means we're double counting missing tiles as incomplete tiles tho, now, how > about we don't add to incomplete here, and we change checks for incomplete to > check for incomplete||missing? Done. https://codereview.chromium.org/364063005/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/364063005/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:870: if (append_quads_data.num_incomplete_tiles > 0) { On 2014/07/07 20:06:28, danakj wrote: > you can still just do if (append_quads_data.num_incomplete_tiles) > > .. tho, i think we should do > > if (append_quads_data.num_incomplete_tiles || > append_quads_data.num_missing_tiles) > > as per earlier comment Done. https://codereview.chromium.org/364063005/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/364063005/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:2737: + Keeps track of number of incomplete tiles in RenderPass during DrawQuad On 2014/07/07 20:06:28, danakj wrote: > s/in RenderPass during DrawQuad stage/in a drawn compositor frame/ Done.
LGTM https://codereview.chromium.org/364063005/diff/60001/cc/layers/picture_image_... File cc/layers/picture_image_layer_impl_unittest.cc (right): https://codereview.chromium.org/364063005/diff/60001/cc/layers/picture_image_... cc/layers/picture_image_layer_impl_unittest.cc:160: EXPECT_EQ(data.num_incomplete_tiles, 0); order should be: expected, actual https://codereview.chromium.org/364063005/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/364063005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:872: num_incomplete_tiles += append_quads_data.num_incomplete_tiles; maybe these += don't really fit inside these if blocks.. how about we just move them both out below the if blocks. += 0 is a fine thing to do.
The CQ bit was checked by weiliangc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weiliangc@chromium.org/364063005/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by weiliangc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weiliangc@chromium.org/364063005/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by weiliangc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weiliangc@chromium.org/364063005/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by weiliangc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weiliangc@chromium.org/364063005/140001
Message was sent while issue was closed.
Change committed as 283009 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
