Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(677)

Issue 375093002: Initial attempt at counting layers in the compositor thread. (Closed)

Created:
6 years, 5 months ago by dneto
Modified:
6 years, 5 months ago
CC:
chromium-reviews, cc-bugs_chromium.org, asvitkine+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Add 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… #

Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -0 lines) Patch
M cc/trees/layer_tree_host_impl.cc View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_impl.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_impl.cc View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_impl_unittest.cc View 1 2 3 4 1 chunk +15 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
dneto
I'm unsure of a couple of things and would appreciate comments on the "coder's choice" ...
6 years, 5 months ago (2014-07-10 01:29:03 UTC) #1
Ian Vollick
https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode1112 cc/trees/layer_tree_host_impl.cc:1112: active_tree_->root_layer(), base::Bind(LayerCounter, &num_layers_)); I'm a bit worried about doing ...
6 years, 5 months ago (2014-07-10 03:16:23 UTC) #2
danakj
https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode117 cc/trees/layer_tree_host_impl.cc:117: void LayerCounter(size_t* counter, cc::LayerImpl* /* layer */) { we ...
6 years, 5 months ago (2014-07-10 18:03:41 UTC) #3
Ian Vollick
https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode1110 cc/trees/layer_tree_host_impl.cc:1110: num_layers_ = 0; > The active_tree_->layer_id_map_.size() has the number ...
6 years, 5 months ago (2014-07-10 18:09:51 UTC) #4
dneto
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_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode1110 ...
6 years, 5 months ago (2014-07-10 18:44:17 UTC) #5
dneto
Uploaded new patch that uses LayerTreeImpl::layer_id_map_.size() directly, as suggested. Much less code! Also I fixed ...
6 years, 5 months ago (2014-07-10 20:15:57 UTC) #6
dneto
https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host_impl.cc#newcode1107 cc/trees/layer_tree_host_impl.cc:1107: "Compositing.NumActiveLayers", active_tree_->NumLayers(), 1, 1000, 100); This is the corrected ...
6 years, 5 months ago (2014-07-10 21:20:11 UTC) #7
danakj
LG a few small things https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host_impl.cc#newcode1104 cc/trees/layer_tree_host_impl.cc:1104: // Use a bucket ...
6 years, 5 months ago (2014-07-10 21:27:44 UTC) #8
dneto
I will tweak the patch as requested. The histogram configuration is still up for debate. ...
6 years, 5 months ago (2014-07-11 15:23:31 UTC) #9
danakj
https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host_impl.cc#newcode1104 cc/trees/layer_tree_host_impl.cc:1104: // Use a bucket width of 10 (i.e. 1000/100) ...
6 years, 5 months ago (2014-07-11 15:27:21 UTC) #10
dneto
On 2014/07/11 15:27:21, danakj wrote: > https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host_impl.cc > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/375093002/diff/60001/cc/trees/layer_tree_host_impl.cc#newcode1104 > ...
6 years, 5 months ago (2014-07-11 17:07:41 UTC) #11
danakj
FYI Please try to not upload a rebase + your own changes in the same ...
6 years, 5 months ago (2014-07-11 17:08:57 UTC) #12
danakj
LGTM https://codereview.chromium.org/375093002/diff/80001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/375093002/diff/80001/tools/metrics/histograms/histograms.xml#newcode2749 tools/metrics/histograms/histograms.xml:2749: + The number of layers in a the ...
6 years, 5 months ago (2014-07-11 17:09:54 UTC) #13
dneto
On 2014/07/11 17:09:54, danakj wrote: > LGTM > > https://codereview.chromium.org/375093002/diff/80001/tools/metrics/histograms/histograms.xml > File tools/metrics/histograms/histograms.xml (right): > ...
6 years, 5 months ago (2014-07-11 17:39:20 UTC) #14
danakj
+asvitkine for histogram OWNERS
6 years, 5 months ago (2014-07-11 17:40:28 UTC) #15
Alexei Svitkine (slow)
lgtm % comment https://codereview.chromium.org/375093002/diff/100001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/375093002/diff/100001/tools/metrics/histograms/histograms.xml#newcode2749 tools/metrics/histograms/histograms.xml:2749: + The number of layers in ...
6 years, 5 months ago (2014-07-14 13:52:24 UTC) #16
dneto
On 2014/07/14 13:52:24, Alexei Svitkine wrote: > lgtm % comment > > https://codereview.chromium.org/375093002/diff/100001/tools/metrics/histograms/histograms.xml > File ...
6 years, 5 months ago (2014-07-14 14:48:42 UTC) #17
danakj
The CQ bit was checked by danakj@chromium.org
6 years, 5 months ago (2014-07-14 14:59:00 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dneto@chromium.org/375093002/120001
6 years, 5 months ago (2014-07-14 15:00:07 UTC) #19
commit-bot: I haz the power
6 years, 5 months ago (2014-07-14 16:58:08 UTC) #20
Message was sent while issue was closed.
Change committed as 282964

Powered by Google App Engine
This is Rietveld 408576698