Add UMA for number of layer promotions [chromium/src : master]
Xida Chen (Gerrit)
Stephen: PTAL before sending to histogram owners.
To view, visit change 619172. To unsubscribe, or for help writing mail filters, visit settings.
Stephen Chenney (Gerrit)
The implementation looks good. My only significant request is to pass the aggregator around by reference rather than using a pointer for something that is never NULL.
2 comments:
File third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.h:
Patch Set #3, Line 59:
void Update(PaintLayer* root, CompositingReasonsAggregator*);In general, if something cannot be null we prefer to pass by reference. I would prefer you to change it here and at the other call sites.
File third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.cpp:
Patch Set #3, Line 588:
if ((reasons_to_composite & kCompositingReasonComboAllDirectReasons) == 0)if (!(reasons_to_composite & kCompositingReasonComboAllDirectReasons))
or does that cause problems?
To view, visit change 619172. To unsubscribe, or for help writing mail filters, visit settings.
Xida Chen (Gerrit)
2 comments:
File third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.h:
Patch Set #3, Line 59:
void Update(PaintLayer* root, CompositingReasonsAggregator&);In general, if something cannot be null we prefer to pass by reference. […]
Done
File third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.cpp:
Patch Set #3, Line 588:
if (!(reasons_to_composite & kCompositingReasonComboAllDirectReasons))if (!(reasons_to_composite & kCompositingReasonComboAllDirectReasons)) […]
Done, should be equivalent.
To view, visit change 619172. To unsubscribe, or for help writing mail filters, visit settings.
Stephen Chenney (Gerrit)
LGTM for the blink changes.
Someone on histograms will give the official +1.
Patch set 4:Code-Review +1
Stephen Chenney (Gerrit)
Just removing the Code Review +1 pending the histograms review.
Patch set 4:-Code-Review
Xida Chen (Gerrit)
Xida Chen would like Jesse Doherty to review this change.
Add UMA for number of layer promotions
In this CL, we add histogram to check number of layers that gets promoted
due to the following reasons:
1. Overlap
2. Transform / opacity animation
3. Assumed overlap
4. Indirectly composited
We aggregate the number of layers over all frames.
Bug: 754786
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ifd099c6b1f88aa110ae9068ce6238d8a91f9a342
---
M third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.cpp
M third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.h
M third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp
M third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.h
M third_party/WebKit/Source/platform/graphics/CompositingReasons.h
M tools/metrics/histograms/histograms.xml
6 files changed, 70 insertions(+), 15 deletions(-)
Xida Chen (Gerrit)
+jwd@ for histogram changes.
Jesse Doherty (Gerrit)
1 comment:
File tools/metrics/histograms/histograms.xml:
Can you add when it's logged?
To view, visit change 619172. To unsubscribe, or for help writing mail filters, visit settings.
Xida Chen (Gerrit)
1 comment:
Can you add when it's logged?
Done
To view, visit change 619172. To unsubscribe, or for help writing mail filters, visit settings.
Philip Rogers (Gerrit)
Naive question: for calculating relative changes, can we use Compositing.Renderer.NumActiveLayers for the total number of layers created by PaintLayerCompositor?
Xida Chen (Gerrit)
Patch Set 5:
Naive question: for calculating relative changes, can we use Compositing.Renderer.NumActiveLayers for the total number of layers created by PaintLayerCompositor?
+chrishtr@: I am assuming that we would like to see break-down numbers for various reasons, instead of a total number?
Chris Harrelson (Gerrit)
Patch Set 5:
Patch Set 5:
Naive question: for calculating relative changes, can we use Compositing.Renderer.NumActiveLayers for the total number of layers created by PaintLayerCompositor?
+chrishtr@: I am assuming that we would like to see break-down numbers for various reasons, instead of a total number?
Just asked pdr offline. His concern is that this CL computes the number of PaintLayers that end up with a composited backing with overlap/etc factors, but if we don't know the total number of PaintLayers that get a composited backing, we also can't compute a percentage due to overlap/etc. This is a good point...
Xida, could you add another UMA couning total number of promoted PaintLayers? This can be different than NumActiveLayers or NumActivePictureLayers, since a composited paint layer can have more than one of the above.
Xida Chen (Gerrit)
Patch Set 5:
Patch Set 5:
Patch Set 5:
Naive question: for calculating relative changes, can we use Compositing.Renderer.NumActiveLayers for the total number of layers created by PaintLayerCompositor?
+chrishtr@: I am assuming that we would like to see break-down numbers for various reasons, instead of a total number?
Just asked pdr offline. His concern is that this CL computes the number of PaintLayers that end up with a composited backing with overlap/etc factors, but if we don't know the total number of PaintLayers that get a composited backing, we also can't compute a percentage due to overlap/etc. This is a good point...
Xida, could you add another UMA couning total number of promoted PaintLayers? This can be different than NumActiveLayers or NumActivePictureLayers, since a composited paint layer can have more than one of the above.
Done. Please review the latest patch.
Chris Harrelson (Gerrit)
LGTM with one comment to change the name of the struct.
Patch set 7:Code-Review +1
1 comment:
File third_party/WebKit/Source/platform/graphics/CompositingReasons.h:
Patch Set #7, Line 191:
struct CompositingReasonsAggregator {CompositingReasonsStats is better I think. "Aggregator" makes it
sound like this struct does work, which it doesn't - it just stores
data.
To view, visit change 619172. To unsubscribe, or for help writing mail filters, visit settings.
Xida Chen (Gerrit)
+holte@: please review histogram changes. Thank you.
Xida Chen (Gerrit)
Xida Chen removed Jesse Doherty from this change.
To view, visit change 619172. To unsubscribe, or for help writing mail filters, visit settings.
Steven Holte (Gerrit)
Add base="true", otherwise LGTM
Patch set 8:Code-Review +1
1 comment:
File tools/metrics/histograms/histograms.xml:
Since you are not recording to this specific histogram, mark it with base="true".
To view, visit change 619172. To unsubscribe, or for help writing mail filters, visit settings.
Xida Chen (Gerrit)
Patch set 11:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"increase bucket to 5 and put basetrue back" https://chromium-review.googlesource.com/c/619172/11
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/619172/11
Bot data: {"action": "start", "triggered_at": "2017-08-23T02:28:32.0Z", "cq_cfg_revision": "81f1a3f0cb07446abeff7dd01ff1b8bae94ffe0d", "revision": "d074b136aba64cf9883df8a138a2dd8683945180"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/369107)
Xida Chen (Gerrit)
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"increase bucket to 5 and put basetrue back" https://chromium-review.googlesource.com/c/619172/11
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/619172/11
Bot data: {"action": "start", "triggered_at": "2017-08-23T10:54:42.0Z", "cq_cfg_revision": "81f1a3f0cb07446abeff7dd01ff1b8bae94ffe0d", "revision": "d074b136aba64cf9883df8a138a2dd8683945180"}
To view, visit change 619172. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
Commit Bot merged this change.
Add UMA for number of layer promotions
In this CL, we add histogram to check number of layers that gets promoted
due to the following reasons:
1. Overlap
2. Transform / opacity animation
3. Assumed overlap
4. Indirectly composited
We aggregate the number of layers over all frames.
Bug: 754786
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ifd099c6b1f88aa110ae9068ce6238d8a91f9a342
Reviewed-on: https://chromium-review.googlesource.com/619172
Commit-Queue: Xida Chen <xida...@chromium.org>
Reviewed-by: Steven Holte <ho...@chromium.org>
Reviewed-by: Chris Harrelson <chri...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496660}
---
M third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.cpp
M third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.h
M third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp
M third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.h
M third_party/WebKit/Source/platform/graphics/CompositingReasons.h
M tools/metrics/histograms/histograms.xml
6 files changed, 81 insertions(+), 18 deletions(-)