Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2835763002:
Use swap-promise to improve first* paint times (Closed)
|
Created:
3 years, 8 months ago by panicker Modified:
3 years, 7 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-paint_chromium.org, cc-bugs_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, dshwang, jam, mlamouri+watch-content_chromium.org, piman+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse swap-promise to track first* paint up until swap time.
BUG=657826, 657825
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2835763002
Cr-Commit-Position: refs/heads/master@{#469594}
Committed: https://chromium.googlesource.com/chromium/src/+/34797fd73ea2d3b09f27c84d50eee3c0dfdbc4da
Patch Set 1 #Patch Set 2 : lint #Patch Set 3 : remove printfs #
Total comments: 6
Patch Set 4 : address review comments #
Total comments: 8
Patch Set 5 : address nits #Patch Set 6 : rebase to head #Patch Set 7 : restore dep on base/callback_forward.h #Patch Set 8 : GetFrame() can be null in tests #Patch Set 9 : WrapWeakPersistent -> WrapCrossThreadWeakPersistent: as the callback lives on impl thread and destr… #
Total comments: 2
Patch Set 10 : address nit #
Messages
Total messages: 68 (45 generated)
Description was changed from ========== Use swap-promise to improve first* paint times BUG= ========== to ========== Use swap-promise to improve first* paint times BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Use swap-promise to improve first* paint times BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Use swap-promise to improve first* paint times BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
panicker@chromium.org changed reviewers: + sunnyps@chromium.org
Sunny, could you provide initial feedback on this approach? Thanks!
On 2017/04/21 22:39:16, panicker wrote: > Sunny, could you provide initial feedback on this approach? > Thanks! Looks reasonable to me. I'd probably keep the swap promise class internal to render_widget_compositor.cc.
panicker@chromium.org changed reviewers: + enne@chromium.org
Enne - could you please review this CL? Do you think the swap-promise should be made internal to render_widget_compositor or kept more general (as is) ? Thanks.
Overall approach seems fine to me! The patch description says "improve" but the patch looks like it's really more about "tracking" first paint times, unless I'm misunderstanding? https://codereview.chromium.org/2835763002/diff/40001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/2835763002/diff/40001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:1161: QueueSwapPromise(base::MakeUnique<cc::ReportTimeSwapPromise>( Agreed with sunnyps that ReportSwapTimePromise could just live in render_widget_compositor.cc. https://codereview.chromium.org/2835763002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintTiming.cpp (right): https://codereview.chromium.org/2835763002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintTiming.cpp:169: if (GetFrame()->GetPage() && style nit: could rewrite this as "if (!GetPage) return;" and then if (WLTV* layerTreeView = super long indirection) and not have to repeat the Get->Get->Get->Get->Get part. https://codereview.chromium.org/2835763002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintTiming.cpp:183: if (did_swap) { style nit: could early out here instead of indenting everything
Description was changed from ========== Use swap-promise to improve first* paint times BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Use swap-promise to track first* paint up until swap time. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
PTAL https://codereview.chromium.org/2835763002/diff/40001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/2835763002/diff/40001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:1161: QueueSwapPromise(base::MakeUnique<cc::ReportTimeSwapPromise>( On 2017/05/01 21:58:11, enne wrote: > Agreed with sunnyps that ReportSwapTimePromise could just live in > render_widget_compositor.cc. Done. https://codereview.chromium.org/2835763002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintTiming.cpp (right): https://codereview.chromium.org/2835763002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintTiming.cpp:169: if (GetFrame()->GetPage() && On 2017/05/01 21:58:11, enne wrote: > style nit: could rewrite this as "if (!GetPage) return;" and then if (WLTV* > layerTreeView = super long indirection) and not have to repeat the > Get->Get->Get->Get->Get part. Done. https://codereview.chromium.org/2835763002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintTiming.cpp:183: if (did_swap) { On 2017/05/01 21:58:11, enne wrote: > style nit: could early out here instead of indenting everything Done.
panicker@chromium.org changed reviewers: + kinuko@chromium.org
Kinuko - could you please review for public/platform ?
lgtm
Description was changed from ========== Use swap-promise to track first* paint up until swap time. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Use swap-promise to track first* paint up until swap time. BUG=657826,657825 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
lgtm % nits https://codereview.chromium.org/2835763002/diff/60001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/2835763002/diff/60001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:191: static_cast<double>(base::Time::kMicrosecondsPerSecond); nit: can you put the static_case<double> on the first part of the expression (...ToInternalValue)? https://codereview.chromium.org/2835763002/diff/60001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/2835763002/diff/60001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.h:241: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; nit: DISALLOW_COPY_AND_ASSIGN(ReportTimeSwapPromise) and also for RenderWidgetCompositor since that's missing too https://codereview.chromium.org/2835763002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintTiming.h (right): https://codereview.chromium.org/2835763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintTiming.h:91: void ReportSwapTime(PaintEvent, bool did_swap, double timestamp); nit: newline after ReportSwapTime https://codereview.chromium.org/2835763002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebLayerTreeView.h (right): https://codereview.chromium.org/2835763002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebLayerTreeView.h:185: nit: can you typedef or using the callback here? and document what the parameters mean?
lgtm
https://codereview.chromium.org/2835763002/diff/60001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/2835763002/diff/60001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:191: static_cast<double>(base::Time::kMicrosecondsPerSecond); On 2017/05/02 01:20:46, sunnyps wrote: > nit: can you put the static_case<double> on the first part of the expression > (...ToInternalValue)? Done. https://codereview.chromium.org/2835763002/diff/60001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/2835763002/diff/60001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.h:241: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2017/05/02 01:20:46, sunnyps wrote: > nit: DISALLOW_COPY_AND_ASSIGN(ReportTimeSwapPromise) and also for > RenderWidgetCompositor since that's missing too Done. https://codereview.chromium.org/2835763002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintTiming.h (right): https://codereview.chromium.org/2835763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintTiming.h:91: void ReportSwapTime(PaintEvent, bool did_swap, double timestamp); On 2017/05/02 01:20:46, sunnyps wrote: > nit: newline after ReportSwapTime Done. https://codereview.chromium.org/2835763002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebLayerTreeView.h (right): https://codereview.chromium.org/2835763002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebLayerTreeView.h:185: On 2017/05/02 01:20:46, sunnyps wrote: > nit: can you typedef or using the callback here? and document what the > parameters mean? Done.
The CQ bit was checked by panicker@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_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, sunnyps@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2835763002/#ps80001 (title: "address nits")
The CQ bit was unchecked by panicker@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 checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, sunnyps@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2835763002/#ps100001 (title: "rebase to head")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by panicker@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_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
The CQ bit was checked by panicker@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 panicker@chromium.org
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, sunnyps@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2835763002/#ps120001 (title: "restore dep on base/callback_forward.h")
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_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
The CQ bit was checked by panicker@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, sunnyps@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2835763002/#ps140001 (title: "GetFrame() can be null in tests")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by panicker@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, sunnyps@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2835763002/#ps160001 (title: "WrapWeakPersistent -> WrapCrossThreadWeakPersistent: as the callback lives on impl thread and destr…")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % small nit https://codereview.chromium.org/2835763002/diff/160001/content/renderer/gpu/r... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/2835763002/diff/160001/content/renderer/gpu/r... content/renderer/gpu/render_widget_compositor.h:225: class ReportTimeSwapPromise : public cc::SwapPromise { nit: Can you move this to the .cc file? It doesn't have to be an inner class and can go in an anonymous namespace instead.
The CQ bit was unchecked by panicker@chromium.org
https://codereview.chromium.org/2835763002/diff/160001/content/renderer/gpu/r... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/2835763002/diff/160001/content/renderer/gpu/r... content/renderer/gpu/render_widget_compositor.h:225: class ReportTimeSwapPromise : public cc::SwapPromise { On 2017/05/05 00:12:14, sunnyps wrote: > nit: Can you move this to the .cc file? It doesn't have to be an inner class and > can go in an anonymous namespace instead. Done.
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, sunnyps@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2835763002/#ps180001 (title: "address nit")
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": 180001, "attempt_start_ts": 1493943681853160,
"parent_rev": "77365188782303a17f15571a0f253dcb5cc0e0b9", "commit_rev":
"d1ee56390e79c9ea7779ad7247d964ba61b572c1"}
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493943681853160,
"parent_rev": "c7a279c0eebd469a2aac1177710cce723f8c060d", "commit_rev":
"34797fd73ea2d3b09f27c84d50eee3c0dfdbc4da"}
Message was sent while issue was closed.
Description was changed from ========== Use swap-promise to track first* paint up until swap time. BUG=657826,657825 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Use swap-promise to track first* paint up until swap time. BUG=657826,657825 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2835763002 Cr-Commit-Position: refs/heads/master@{#469594} Committed: https://chromium.googlesource.com/chromium/src/+/34797fd73ea2d3b09f27c84d50ee... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/34797fd73ea2d3b09f27c84d50ee... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
