Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
375093002:
Initial attempt at counting layers in the compositor thread. (Closed)
DescriptionAdd a UMA for the number of layers in a frame.
Reported from the compositor thread.
BUG=253919
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282964
Patch Set 1 #Patch Set 2 : Add UMA for number of layers in a compositor frame #
Total comments: 17
Patch Set 3 : Use LayerTreeImpl::layer_id_map_.size() directly #
Total comments: 1
Patch Set 4 : Use LayerTreeImpl::layer_id_map_.size() and fix histogram name #
Total comments: 14
Patch Set 5 : Tweak histogram config; tweak placement of code #
Total comments: 1
Patch Set 6 : I fixed the English in the histogram summary. #
Total comments: 1
Patch Set 7 : Update comment in histograms.xml to say the stat is collected once per frame, before the frame is d… #
Messages
Total messages: 20 (0 generated)
I'm unsure of a couple of things and would appreciate comments on the "coder's choice" issues. https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1114: "Compositing.NumLayers", num_layers_, 1, 1000, 50); Also not sure if range of 1000 and width-20 buckets is the most useful view. Alternate would be standard histogram of up to 100. https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.h:705: size_t num_layers_; Not sure if size_t is appropriate here, or overkill. I went with size_t because the Chromium style guide says to use size_t when counting things. To be honest unsigned would feel more natural.
https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1112: active_tree_->root_layer(), base::Bind(LayerCounter, &num_layers_)); I'm a bit worried about doing an extra tree walk every frame to gather this number, especially since this number can only change at commit time when we update the trees. If mucking with the tree synchronizer is ugly, perhaps we could update num_layers_ in ::CommitComplete and just report it here (so that we get the right weighting)? https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1114: "Compositing.NumLayers", num_layers_, 1, 1000, 50); On 2014/07/10 01:29:03, dneto wrote: > Also not sure if range of 1000 and width-20 buckets is the most useful view. > Alternate would be standard histogram of up to 100. This is probably ok. width-10 buckets might be nice to give some more resolution for small trees (where we'll likely have the most samples), but I'm not sure it would make much difference. https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.h:705: size_t num_layers_; On 2014/07/10 01:29:03, dneto wrote: > Not sure if size_t is appropriate here, or overkill. I went with size_t because > the Chromium style guide says to use size_t when counting things. To be honest > unsigned would feel more natural. I agree that this is overkill, but there's no harm in using a size_t for style's sake. I think this is fine. https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:6726: TEST_F(LayerTreeHostImplTest, TestLayerCountingZeroInit) { Nice tests!
https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:117: void LayerCounter(size_t* counter, cc::LayerImpl* /* layer */) { we don't comment out parameter names in chrome https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1110: num_layers_ = 0; While I love tests, I don't think we need to add a member variable to this class that is only for unit tests. Generally we haven't tested UMA counts with unit tests, which maybe is bad, idk. If we want to test this, we should find a way to test the computation without requiring a new member var on this class. The active_tree_->layer_id_map_.size() has the number of layers in the active tree, doesn't it? https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.h:467: // Public only for for unit testing comments are full sentences with periods https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.h:705: size_t num_layers_; On 2014/07/10 03:16:23, Ian Vollick wrote: > On 2014/07/10 01:29:03, dneto wrote: > > Not sure if size_t is appropriate here, or overkill. I went with size_t > because > > the Chromium style guide says to use size_t when counting things. To be honest > > unsigned would feel more natural. > > I agree that this is overkill, but there's no harm in using a size_t for style's > sake. I think this is fine. "unsigned" is generally banned. You'd use an int otherwise (we use unsigned for GL data types actually, or uint32 but that's one of few exceptions).
https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1110: num_layers_ = 0; > The active_tree_->layer_id_map_.size() has the number of layers in the active > tree, doesn't it? Ah, cool! That's a great idea. If we can use that size rather than computing the # by hand here, then we def don't need tests.
I'll rework to just use layer_id_map_.size(), as per danakj's suggestion https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1110: num_layers_ = 0; Yes, that's smart! I don't know the data structures well enough yet. :-) https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1112: active_tree_->root_layer(), base::Bind(LayerCounter, &num_layers_)); On 2014/07/10 03:16:23, Ian Vollick wrote: > I'm a bit worried about doing an extra tree walk every frame to gather this > number, especially since this number can only change at commit time when we > update the trees. > > If mucking with the tree synchronizer is ugly, perhaps we could update > num_layers_ in ::CommitComplete and just report it here (so that we get the > right weighting)? I'll rework it one way or another. https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1114: "Compositing.NumLayers", num_layers_, 1, 1000, 50); On 2014/07/10 03:16:22, Ian Vollick wrote: > On 2014/07/10 01:29:03, dneto wrote: > > Also not sure if range of 1000 and width-20 buckets is the most useful view. > > Alternate would be standard histogram of up to 100. > > This is probably ok. width-10 buckets might be nice to give some more resolution > for small trees (where we'll likely have the most samples), but I'm not sure it > would make much difference. Ok. I'll make width-10 buckets with a range up to 1000. I was only concerned about space but it looks like the histogram implementation effectively uses a std::vector<int> sized to the number of buckets. So 100 ints shouldn't be a problem. https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.h:705: size_t num_layers_; On 2014/07/10 03:16:23, Ian Vollick wrote: > On 2014/07/10 01:29:03, dneto wrote: > > Not sure if size_t is appropriate here, or overkill. I went with size_t > because > > the Chromium style guide says to use size_t when counting things. To be honest > > unsigned would feel more natural. > > I agree that this is overkill, but there's no harm in using a size_t for style's > sake. I think this is fine. Acknowledged. https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.h:705: size_t num_layers_; On 2014/07/10 18:03:41, danakj wrote: > On 2014/07/10 03:16:23, Ian Vollick wrote: > > On 2014/07/10 01:29:03, dneto wrote: > > > Not sure if size_t is appropriate here, or overkill. I went with size_t > > because > > > the Chromium style guide says to use size_t when counting things. To be > honest > > > unsigned would feel more natural. > > > > I agree that this is overkill, but there's no harm in using a size_t for > style's > > sake. I think this is fine. > > "unsigned" is generally banned. You'd use an int otherwise (we use unsigned for > GL data types actually, or uint32 but that's one of few exceptions). Acknowledged. https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:6726: TEST_F(LayerTreeHostImplTest, TestLayerCountingZeroInit) { On 2014/07/10 03:16:23, Ian Vollick wrote: > Nice tests! Thanks, but obviated by danakj's suggestion. :-)
Uploaded new patch that uses LayerTreeImpl::layer_id_map_.size() directly, as suggested. Much less code! Also I fixed the naming of the histogram to match the .xml file. It's now "Compositing.NumActiveLayers", which should be clearer. https://codereview.chromium.org/375093002/diff/40001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1107: "Compositing.NumLayers", active_tree_->NumLayers(), 1, 1000, 100); Gah. Disregard. I changed the name of the histogram in the .xml but not here... will upload modified patch soon.
https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1107: "Compositing.NumActiveLayers", active_tree_->NumLayers(), 1, 1000, 100); This is the corrected name for the UMA, matching the one in the histogram.xml file.
LG a few small things https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1104: // Use a bucket width of 10 (i.e. 1000/100) to better I'm not sure what this comment is saying? That you could change the 1 to a 10? This histogram will be out in the wild and show up on dashboards etc, so we won't really be able to change it once we ship it. https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:1380: size_t LayerTreeImpl::NumLayers() { can you move up below Register/UnregisterLayer() https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.h:279: size_t NumLayers(); can you move up below RegisterLayer/UnregisterLayer()? https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... File cc/trees/layer_tree_impl_unittest.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:2328: host_impl().active_tree()->SetRootLayer(root.Pass()); how about just EXPECT 0 before setting the root layer, then you don't need the test case above and you verify also that it's not already 1 so the check below is for real https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:2329: CHECK_EQ(1u, host_impl().active_tree()->NumLayers()); EXPECT_EQ not CHECK_EQ in a test https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:2337: host_impl().active_tree()->SetRootLayer(root.Pass()); can you EXPECT 0 before doing the set root so we can demonstrate it changing, so we know it wasn't already 4.
I will tweak the patch as requested. The histogram configuration is still up for debate. https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1104: // Use a bucket width of 10 (i.e. 1000/100) to better On 2014/07/10 21:27:44, danakj wrote: > I'm not sure what this comment is saying? That you could change the 1 to a 10? > This histogram will be out in the wild and show up on dashboards etc, so we > won't really be able to change it once we ship it. The 1 is min, the 1000 is max, and 10 is the number of buckets. Vollick suggested that chrome wouldn't perform well at all beyond 200, and expected the vast majority of frames to have very few layers. So we wanted more granularity at the lower end. The canned counting histograms are: (1,1000000,50): probably too large a range (1,100,50): good granularity, but maybe not enough range at the high end? (1,10000,50): poor granularity, and probably overkill. I don't have good intuition on the distribution, so I'm looking for guidance on the numbers, or on expressing intent in the comment. https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:1380: size_t LayerTreeImpl::NumLayers() { On 2014/07/10 21:27:44, danakj wrote: > can you move up below Register/UnregisterLayer() Acknowledged. https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.h:279: size_t NumLayers(); On 2014/07/10 21:27:44, danakj wrote: > can you move up below RegisterLayer/UnregisterLayer()? Acknowledged. https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... File cc/trees/layer_tree_impl_unittest.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:2328: host_impl().active_tree()->SetRootLayer(root.Pass()); On 2014/07/10 21:27:44, danakj wrote: > how about just EXPECT 0 before setting the root layer, then you don't need the > test case above and you verify also that it's not already 1 so the check below > is for real Acknowledged. https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:2329: CHECK_EQ(1u, host_impl().active_tree()->NumLayers()); On 2014/07/10 21:27:44, danakj wrote: > EXPECT_EQ not CHECK_EQ in a test Ouch! Thanks. https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:2337: host_impl().active_tree()->SetRootLayer(root.Pass()); On 2014/07/10 21:27:44, danakj wrote: > can you EXPECT 0 before doing the set root so we can demonstrate it changing, so > we know it wasn't already 4. Acknowledged.
https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1104: // Use a bucket width of 10 (i.e. 1000/100) to better On 2014/07/11 15:23:30, dneto wrote: > On 2014/07/10 21:27:44, danakj wrote: > > I'm not sure what this comment is saying? That you could change the 1 to a 10? > > This histogram will be out in the wild and show up on dashboards etc, so we > > won't really be able to change it once we ship it. > > The 1 is min, the 1000 is max, and 10 is the number of buckets. Vollick > suggested that chrome wouldn't perform well at all beyond 200, and expected the > vast majority of frames to have very few layers. So we wanted more granularity > at the lower end. > The canned counting histograms are: > (1,1000000,50): probably too large a range > (1,100,50): good granularity, but maybe not enough range at the high > end? > (1,10000,50): poor granularity, and probably overkill. > I don't have good intuition on the distribution, so I'm looking for guidance on > the numbers, or on expressing intent in the comment. Ok how about 1, 400, 20? And we can drop the comment?
On 2014/07/11 15:27:21, danakj wrote: > https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host... > cc/trees/layer_tree_host_impl.cc:1104: // Use a bucket width of 10 (i.e. > 1000/100) to better > On 2014/07/11 15:23:30, dneto wrote: > > On 2014/07/10 21:27:44, danakj wrote: > > > I'm not sure what this comment is saying? That you could change the 1 to a > 10? > > > This histogram will be out in the wild and show up on dashboards etc, so we > > > won't really be able to change it once we ship it. > > > > The 1 is min, the 1000 is max, and 10 is the number of buckets. Vollick > > suggested that chrome wouldn't perform well at all beyond 200, and expected > the > > vast majority of frames to have very few layers. So we wanted more > granularity > > at the lower end. > > The canned counting histograms are: > > (1,1000000,50): probably too large a range > > (1,100,50): good granularity, but maybe not enough range at the high > > end? > > (1,10000,50): poor granularity, and probably overkill. > > I don't have good intuition on the distribution, so I'm looking for guidance > on > > the numbers, or on expressing intent in the comment. > > Ok how about 1, 400, 20? And we can drop the comment? Fixed in latest patch
FYI Please try to not upload a rebase + your own changes in the same patch set. When you rebase, upload that as a patchset on its own.
LGTM https://codereview.chromium.org/375093002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/375093002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:2749: + The number of layers in a the active tree for the frame, whether or not they "in the active tree for each compositor frame"
On 2014/07/11 17:09:54, danakj wrote: > LGTM > > https://codereview.chromium.org/375093002/diff/80001/tools/metrics/histograms... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/375093002/diff/80001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:2749: + The number of layers in a the > active tree for the frame, whether or not they > "in the active tree for each compositor frame" I fixed the English as suggested.
+asvitkine for histogram OWNERS
lgtm % comment https://codereview.chromium.org/375093002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/375093002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2749: + The number of layers in the active tree for each compositor frame. Please expand description to clarify when it's logged (i.e. once per frame before it's drawn?).
On 2014/07/14 13:52:24, Alexei Svitkine wrote: > lgtm % comment > > https://codereview.chromium.org/375093002/diff/100001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/375093002/diff/100001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:2749: + The number of layers in the > active tree for each compositor frame. > Please expand description to clarify when it's logged (i.e. once per frame > before it's drawn?). Fixed in patch 7. Yes, the stat is collected once per frame, before it's drawn.
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dneto@chromium.org/375093002/120001
Message was sent while issue was closed.
Change committed as 282964 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
