New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 728179 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2017
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 726387



Sign in to add a comment

Re-land change to buffer and merge paint timing updates in the browser process

Project Member Reported by bmcquade@chromium.org, May 31 2017

Issue description

https://codereview.chromium.org/2901383002 was landed to buffer and merge paint timing updates, however it caused the SubresourceFcpOrder test to flake.

Once that test is de-flaked, we should re-land this change.
 
Blocking: 726387
Project Member

Comment 2 by bugdroid1@chromium.org, May 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f576d151955389c1b797409da6ed19c2a5530ff5

commit f576d151955389c1b797409da6ed19c2a5530ff5
Author: bmcquade <bmcquade@chromium.org>
Date: Wed May 31 17:21:23 2017

Reland: Buffer cross frame paint timing updates.

This is a re-land of https://codereview.chromium.org/2901383002, which was
reverted due to a timing dependency in ResourcePrefetchPredictorBrowserTest.
SubresourceFcpOrder that has since been fixed ( crbug.com/728176 ).

Original change description:

With the recent change to merge cross-frame paint timings, we sometimes encounter
cases where these timings arrive out of order.

For example, we may receive a first paint notification for frame A first,
then a first paint notification for frame B, but frame B's first paint
time may have happened before frame A's first paint time.

In the current implementation, we dispatch a first paint notification to observers
as soon as we receive a first paint notification from any frame in the page.

This means we don't end up recording the true page-level first paint time on
all pages.

This change adds a 1s buffering period before we dispatch first paint notifications
to observers. During this buffering period, we accumulate all first paint updates
across all frames, and retain the minimum first paint time. We then dispatch the min
observed first paint time across this window.

This change should significantly reduce the frequency with which we log out of order
paint timings. We have added new UMA to keep track of how often this continues to
happen, even with the 1s buffering.

BUG= 728179 
TBR=isherman,mattcary

Review-Url: https://codereview.chromium.org/2917693002
Cr-Commit-Position: refs/heads/master@{#475955}

[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/page_load_metrics_embedder_interface.h
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/page_load_metrics_util.cc
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/page_load_metrics_util.h
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/page_load_metrics/page_load_tracker.cc
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/browser/prerender/prerender_browsertest.cc
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/common/BUILD.gn
[add] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/common/page_load_metrics/page_load_metrics_util.cc
[add] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/common/page_load_metrics/page_load_metrics_util.h
[add] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/common/page_load_metrics/test/page_load_metrics_test_util.cc
[add] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/common/page_load_metrics/test/page_load_metrics_test_util.h
[add] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/common/page_load_metrics/test/weak_mock_timer.cc
[add] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/common/page_load_metrics/test/weak_mock_timer.h
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/renderer/page_load_metrics/metrics_render_frame_observer.h
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/chrome/test/BUILD.gn
[modify] https://crrev.com/f576d151955389c1b797409da6ed19c2a5530ff5/tools/metrics/histograms/histograms.xml

Labels: Merge-Request-60
Project Member

Comment 4 by sheriffbot@chromium.org, Jun 2 2017

Labels: -Merge-Request-60 Hotlist-Merge-Approved Merge-Approved-60
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 3 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99cf612851ca1f1e870f508d91b1ebc729c7ebca

commit 99cf612851ca1f1e870f508d91b1ebc729c7ebca
Author: bmcquade <bmcquade@chromium.org>
Date: Sat Jun 03 01:11:52 2017

Reland: Buffer cross frame paint timing updates.

This is a re-land of https://codereview.chromium.org/2901383002, which was
reverted due to a timing dependency in ResourcePrefetchPredictorBrowserTest.
SubresourceFcpOrder that has since been fixed ( crbug.com/728176 ).

Original change description:

With the recent change to merge cross-frame paint timings, we sometimes encounter
cases where these timings arrive out of order.

For example, we may receive a first paint notification for frame A first,
then a first paint notification for frame B, but frame B's first paint
time may have happened before frame A's first paint time.

In the current implementation, we dispatch a first paint notification to observers
as soon as we receive a first paint notification from any frame in the page.

This means we don't end up recording the true page-level first paint time on
all pages.

This change adds a 1s buffering period before we dispatch first paint notifications
to observers. During this buffering period, we accumulate all first paint updates
across all frames, and retain the minimum first paint time. We then dispatch the min
observed first paint time across this window.

This change should significantly reduce the frequency with which we log out of order
paint timings. We have added new UMA to keep track of how often this continues to
happen, even with the 1s buffering.

BUG= 728179 
NOTRY=true
NOPRESUBMIT=true
TBR=csharrison

Review-Url: https://codereview.chromium.org/2917693002
Cr-Original-Commit-Position: refs/heads/master@{#475955}
Review-Url: https://codereview.chromium.org/2917153002
Cr-Commit-Position: refs/branch-heads/3112@{#130}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/page_load_metrics_embedder_interface.h
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/page_load_metrics_util.cc
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/page_load_metrics_util.h
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/page_load_metrics/page_load_tracker.cc
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/browser/prerender/prerender_browsertest.cc
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/common/BUILD.gn
[add] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/common/page_load_metrics/page_load_metrics_util.cc
[add] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/common/page_load_metrics/page_load_metrics_util.h
[add] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/common/page_load_metrics/test/page_load_metrics_test_util.cc
[add] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/common/page_load_metrics/test/page_load_metrics_test_util.h
[add] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/common/page_load_metrics/test/weak_mock_timer.cc
[add] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/common/page_load_metrics/test/weak_mock_timer.h
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/renderer/page_load_metrics/metrics_render_frame_observer.h
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/chrome/test/BUILD.gn
[modify] https://crrev.com/99cf612851ca1f1e870f508d91b1ebc729c7ebca/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Started)

Sign in to add a comment