Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2450293002:
Tune web page background fetching (Closed)
|
Created:
4 years, 1 month ago by Pete Williamson Modified:
4 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, dewittj+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, gavinp+prer_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTune web page background fetching
1. Treat timeout attempts as a completed attempt, not a started attempt.
2. Disable NQE check before doing immediate loading
3. Increase timing on OnDomContentLoaded to allow for poor 2G networks.
BUG=659723
Committed: https://crrev.com/11aeb394f106f8ee8dccd0b01d03d107ae535c44
Cr-Commit-Position: refs/heads/master@{#428506}
Patch Set 1 #
Total comments: 4
Patch Set 2 : CR fixes per DougArnett #
Total comments: 4
Patch Set 3 : CR feedback per DougArnett #
Messages
Total messages: 22 (8 generated)
petewil@chromium.org changed reviewers: + dougarnett@chromium.org
petewil@chromium.org changed reviewers: + dimich@chromium.org
https://codereview.chromium.org/2450293002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2450293002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:19: long kOfflinePageDclDelayMilliseconds = 25000; Milliseconds => Ms ? [recent_tab_helper and snapshot_controller use the "Ms" naming] https://codereview.chromium.org/2450293002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2450293002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:216: if (stop_status == Offliner::RequestStatus::REQUEST_COORDINATOR_TIMED_OUT) { It would be good to break this change out into a separate CL and check with evaluation pass. We might decide to only apply to IMMEDIATE_WINDOW initially or add some retry support in order to keep success rate acceptable.
https://codereview.chromium.org/2450293002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2450293002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:19: long kOfflinePageDclDelayMilliseconds = 25000; On 2016/10/26 23:23:26, dougarnett wrote: > Milliseconds => Ms ? > > [recent_tab_helper and snapshot_controller use the "Ms" naming] Done. https://codereview.chromium.org/2450293002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2450293002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:216: if (stop_status == Offliner::RequestStatus::REQUEST_COORDINATOR_TIMED_OUT) { On 2016/10/26 23:23:26, dougarnett wrote: > It would be good to break this change out into a separate CL and check with > evaluation pass. We might decide to only apply to IMMEDIATE_WINDOW initially or > add some retry support in order to keep success rate acceptable. Done. I think we should check it in anyway, since we now have an 8 minute timeout, but I will put it in a separate changelist as you requested.
The CQ bit was checked by petewil@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...)
lgtm with comments https://codereview.chromium.org/2450293002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (left): https://codereview.chromium.org/2450293002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:496: if (network_quality_estimator_) { Let's keep the network_quality_estimator integration in this class (since it got approval from NQE guys already). I would at least like to record UMA for connection quality even if we don't start using it again soon otherwise. So if some bot complains of unused variable, perhaps add DVLOG with current effective connection type next to the TODO to add UMA. https://codereview.chromium.org/2450293002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:497: // TODO(dougarnett): Add UMA for quality type experienced. Please carry forward this TODO
https://codereview.chromium.org/2450293002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (left): https://codereview.chromium.org/2450293002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:496: if (network_quality_estimator_) { On 2016/10/28 15:51:25, dougarnett wrote: > Let's keep the network_quality_estimator integration in this class (since it got > approval from NQE guys already). I would at least like to record UMA for > connection quality even if we don't start using it again soon otherwise. So if > some bot complains of unused variable, perhaps add DVLOG with current effective > connection type next to the TODO to add UMA. Done. https://codereview.chromium.org/2450293002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:497: // TODO(dougarnett): Add UMA for quality type experienced. On 2016/10/28 15:51:25, dougarnett wrote: > Please carry forward this TODO Done.
lgtm with a big caveat: How do we know this improves things? On slow networks, assuming onLoad doesn't happen at all in reasonable timeout window, this has an effect of just delaying snapshot for 18 seconds more than today. Do we know if background loading survives long enough for that to produce better snapshots? Is the perf impact (RAM/Battery etc) worth extended time? While this can land as an initial approximation based on manual observations, I'd strongly suggest building specific metrics that can measure in the wild the success rate as the immediate next step.
On 2016/10/28 18:21:12, Dmitry Titov wrote: > lgtm with a big caveat: > > How do we know this improves things? On slow networks, assuming onLoad doesn't > happen at all in reasonable timeout window, this has an effect of just delaying > snapshot for 18 seconds more than today. Do we know if background loading > survives long enough for that to produce better snapshots? Is the perf impact > (RAM/Battery etc) worth extended time? > > While this can land as an initial approximation based on manual observations, > I'd strongly suggest building specific metrics that can measure in the wild the > success rate as the immediate next step. Actually, current RMD only waits for OnLoad so this will use less battery if OnLoad doesn't arrive. This is our initial tuning for background based on the data we do have.
still lgtm
On 2016/10/28 18:34:38, dougarnett wrote: > On 2016/10/28 18:21:12, Dmitry Titov wrote: > > lgtm with a big caveat: > > > > How do we know this improves things? On slow networks, assuming onLoad doesn't > > happen at all in reasonable timeout window, this has an effect of just > delaying > > snapshot for 18 seconds more than today. Do we know if background loading > > survives long enough for that to produce better snapshots? Is the perf impact > > (RAM/Battery etc) worth extended time? > > > > While this can land as an initial approximation based on manual observations, > > I'd strongly suggest building specific metrics that can measure in the wild > the > > success rate as the immediate next step. > > Actually, current RMD only waits for OnLoad so this will use less battery if > OnLoad doesn't arrive. This is our initial tuning for background based on the > data we do have. Acknowledged.
The CQ bit was checked by petewil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Tune web page background fetching 1. Treat timeout attempts as a completed attempt, not a started attempt. 2. Disable NQE check before doing immediate loading 3. Increase timing on OnDomContentLoaded to allow for poor 2G networks. BUG=659723 ========== to ========== Tune web page background fetching 1. Treat timeout attempts as a completed attempt, not a started attempt. 2. Disable NQE check before doing immediate loading 3. Increase timing on OnDomContentLoaded to allow for poor 2G networks. BUG=659723 Committed: https://crrev.com/11aeb394f106f8ee8dccd0b01d03d107ae535c44 Cr-Commit-Position: refs/heads/master@{#428506} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/11aeb394f106f8ee8dccd0b01d03d107ae535c44 Cr-Commit-Position: refs/heads/master@{#428506}
Message was sent while issue was closed.
On 2016/10/28 18:21:12, Dmitry Titov wrote: > lgtm with a big caveat: > > How do we know this improves things? On slow networks, assuming onLoad doesn't > happen at all in reasonable timeout window, this has an effect of just delaying > snapshot for 18 seconds more than today. Do we know if background loading > survives long enough for that to produce better snapshots? Is the perf impact > (RAM/Battery etc) worth extended time? > > While this can land as an initial approximation based on manual observations, > I'd strongly suggest building specific metrics that can measure in the wild the > success rate as the immediate next step. Fyi dimich@: It turns out this CL did not include putting back the call to snapshot controller for OnDomContentLoaded so the 25 second delay here means nothing. Instead this effectively adds 2 seconds to the OnLoad event before snapshotting (so potentially much longer than the 18 secs that you raised). This is subtle and was not clear to me when we reviewed this. I created patch https://codereview.chromium.org/2473443004 to put back this call because I thought is was an omission but Pete actually intends not to trigger on DCL+delay at the moment so we will discuss further. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
