Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2484743002:
UMA metric for LayerUpdateTimes (Closed)
DescriptionUMA metric for LayerUpdateTimes
We are interested to learn about correlation between number of layers
and the cost of LayerUpdateTimes in actual sites to guide
some optimization efforts.
To do this, we collect stats on LayersUpdateTime but bucket that into
several suffixed histograms to be able to relate it to number of
layers.
BUG=661741
Committed: https://crrev.com/61be4cf0015eb2c0fb50cd58de61939423eab618
Cr-Commit-Position: refs/heads/master@{#441076}
Patch Set 1 #Patch Set 2 : UMA metric for LayerUpdateTimes and understand the impact of number of layers on it #
Total comments: 5
Patch Set 3 : address feedback #Patch Set 4 : Add missing header #
Total comments: 2
Patch Set 5 : Add brower histograms & record in microseconds #
Total comments: 4
Patch Set 6 : address feedback #
Total comments: 2
Patch Set 7 : Address feedback #
Messages
Total messages: 57 (39 generated)
Description was changed from ========== UMA metric for LayerUpdateTimes and understand the impact of number of layers on it BUG=661741 ========== to ========== UMA metric for LayerUpdateTimes and understand the impact of number of layers on it BUG=661741 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
majidvp@chromium.org changed reviewers: + flackr@chromium.org
https://codereview.chromium.org/2484743002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_in_process.cc (right): https://codereview.chromium.org/2484743002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_in_process.cc:730: std::min(static_cast<size_t>(400), layer_tree_->NumLayers()) / 50); In the current patch I have a simple equal buckets of 80 wide. Number of active layers is being collected in |Compositing.Browser.NumActiveLayers| histogram and looking at that buckets of size 80 are probably too coarse. Perhaps we should switch this to follow bucketization pattern of other histograms.
Description was changed from ========== UMA metric for LayerUpdateTimes and understand the impact of number of layers on it BUG=661741 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== UMA metric for LayerUpdateTimes We are interested to learn about correlation between number of layers and the cost of LayerUpdateTimes in actual sites to guide some optimization efforts. To do this, we collect stats on LayersUpdateTime but bucket that into several suffixed histograms to be able to relate it to number of layers. BUG=661741 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
https://codereview.chromium.org/2484743002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_in_process.cc (right): https://codereview.chromium.org/2484743002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_in_process.cc:730: std::min(static_cast<size_t>(400), layer_tree_->NumLayers()) / 50); On 2016/11/15 at 13:10:55, majidvp wrote: > In the current patch I have a simple equal buckets of 80 wide. You mean 50 wide? > Number of active > layers is being collected in |Compositing.Browser.NumActiveLayers| histogram and > looking at that buckets of size 80 are probably too coarse. Perhaps we should > switch this to follow bucketization pattern of other histograms. I'm not sure, I would like to know the difference between < 10 layers (which seems to be a large number of frames), and 50, but beyond that I think targeting some of the long tail, even though less common, makes sense since that's representative of many of the bad cases we look at. Maybe we should just create a special bucket for < 10 layers for a sort of baseline on ideal layer update costs? https://codereview.chromium.org/2484743002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2484743002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:107204: + <suffix name="8" label="Layer count bucket 8"/> nit: I think we should list the ranges in here.
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
https://codereview.chromium.org/2484743002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_in_process.cc (right): https://codereview.chromium.org/2484743002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_in_process.cc:730: std::min(static_cast<size_t>(400), layer_tree_->NumLayers()) / 50); On 2016/11/16 16:56:50, flackr wrote: > On 2016/11/15 at 13:10:55, majidvp wrote: > > In the current patch I have a simple equal buckets of 80 wide. > You mean 50 wide? > > > Number of active > > layers is being collected in |Compositing.Browser.NumActiveLayers| histogram > and > > looking at that buckets of size 80 are probably too coarse. Perhaps we should > > switch this to follow bucketization pattern of other histograms. > > I'm not sure, I would like to know the difference between < 10 layers (which > seems to be a large number of frames), and 50, but beyond that I think targeting > some of the long tail, even though less common, makes sense since that's > representative of many of the bad cases we look at. Maybe we should just create > a special bucket for < 10 layers for a sort of baseline on ideal layer update > costs? Fair enough, I changed my bucketization to an expontial scheme with a total of 5 buckets. Looking at the out UMA metrics for number of layers I think that covers the most interesting cases. https://codereview.chromium.org/2484743002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2484743002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:107204: + <suffix name="8" label="Layer count bucket 8"/> On 2016/11/16 16:56:51, flackr wrote: > nit: I think we should list the ranges in here. Done.
ajuma@chromium.org changed reviewers: + ajuma@chromium.org
Drive-by nit: https://codereview.chromium.org/2484743002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_in_process.cc (right): https://codereview.chromium.org/2484743002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_in_process.cc:612: "Compositing.Renderer.LayersUpdateTime.%d", This is going to get called for both the browser and renderers. To separate them, you can use GetClientNameForMetrics (see LayerTreeHostImpl::PrepareToDraw for an example).
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
Addressed ajuma@ comment and also ensured we capture the times in microseconds resolution. flackr@ PTAL https://codereview.chromium.org/2484743002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_in_process.cc (right): https://codereview.chromium.org/2484743002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_in_process.cc:612: "Compositing.Renderer.LayersUpdateTime.%d", On 2016/12/03 01:28:23, ajuma wrote: > This is going to get called for both the browser and renderers. To separate > them, you can use GetClientNameForMetrics (see LayerTreeHostImpl::PrepareToDraw > for an example). Thanks for the comment. Fixed!
LGTM with a couple nits. https://codereview.chromium.org/2484743002/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_in_process.cc (right): https://codereview.chromium.org/2484743002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_in_process.cc:593: else if (numLayers >= 10 && numLayers < 30) nit: >= condition in each of these is unnecessary given that we haven't returned yet. https://codereview.chromium.org/2484743002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_in_process.cc:599: else per the style guide, no else (or else if) after return.
lgtm too
https://codereview.chromium.org/2484743002/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_in_process.cc (right): https://codereview.chromium.org/2484743002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_in_process.cc:593: else if (numLayers >= 10 && numLayers < 30) On 2016/12/08 22:35:08, flackr (OOO) wrote: > nit: >= condition in each of these is unnecessary given that we haven't returned > yet. Done. https://codereview.chromium.org/2484743002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_in_process.cc:599: else On 2016/12/08 22:35:08, flackr (OOO) wrote: > per the style guide, no else (or else if) after return. Done.
majidvp@chromium.org changed reviewers: + isherman@chromium.org
+isherman@ please review histogram.xml changes.
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
LGTM % nits: https://codereview.chromium.org/2484743002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2484743002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6585: +<histogram name="Compositing.Browser.LayersUpdateTime" units="microseconds"> nit: Please add a base="true" attribute. https://codereview.chromium.org/2484743002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6857: +<histogram name="Compositing.Renderer.LayersUpdateTime" units="microseconds"> Ditto
The CQ bit was checked by majidvp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, ajuma@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2484743002/#ps120001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
The CQ bit was checked by majidvp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
Description was changed from ========== UMA metric for LayerUpdateTimes We are interested to learn about correlation between number of layers and the cost of LayerUpdateTimes in actual sites to guide some optimization efforts. To do this, we collect stats on LayersUpdateTime but bucket that into several suffixed histograms to be able to relate it to number of layers. BUG=661741 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== UMA metric for LayerUpdateTimes We are interested to learn about correlation between number of layers and the cost of LayerUpdateTimes in actual sites to guide some optimization efforts. To do this, we collect stats on LayersUpdateTime but bucket that into several suffixed histograms to be able to relate it to number of layers. BUG=661741 ==========
The CQ bit was checked by majidvp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483382483427970,
"parent_rev": "b795e378b54ea32fa266df4f13beddca84e2469d", "commit_rev":
"6490311596fcf53f251af7318b1b3e3ff062f632"}
Message was sent while issue was closed.
Description was changed from ========== UMA metric for LayerUpdateTimes We are interested to learn about correlation between number of layers and the cost of LayerUpdateTimes in actual sites to guide some optimization efforts. To do this, we collect stats on LayersUpdateTime but bucket that into several suffixed histograms to be able to relate it to number of layers. BUG=661741 ========== to ========== UMA metric for LayerUpdateTimes We are interested to learn about correlation between number of layers and the cost of LayerUpdateTimes in actual sites to guide some optimization efforts. To do this, we collect stats on LayersUpdateTime but bucket that into several suffixed histograms to be able to relate it to number of layers. BUG=661741 Review-Url: https://codereview.chromium.org/2484743002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== UMA metric for LayerUpdateTimes We are interested to learn about correlation between number of layers and the cost of LayerUpdateTimes in actual sites to guide some optimization efforts. To do this, we collect stats on LayersUpdateTime but bucket that into several suffixed histograms to be able to relate it to number of layers. BUG=661741 Review-Url: https://codereview.chromium.org/2484743002 ========== to ========== UMA metric for LayerUpdateTimes We are interested to learn about correlation between number of layers and the cost of LayerUpdateTimes in actual sites to guide some optimization efforts. To do this, we collect stats on LayersUpdateTime but bucket that into several suffixed histograms to be able to relate it to number of layers. BUG=661741 Committed: https://crrev.com/61be4cf0015eb2c0fb50cd58de61939423eab618 Cr-Commit-Position: refs/heads/master@{#441076} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/61be4cf0015eb2c0fb50cd58de61939423eab618 Cr-Commit-Position: refs/heads/master@{#441076} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
