Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2119613002:
Adds a prerenderer watchdog timer. (Closed)
|
Created:
4 years, 5 months ago by Pete Williamson Modified:
4 years, 5 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds a prerenderer watchdog timer.
The prerenderer can take too long on a page, when it does, we should cancel
the pre-render in progress. This change adds a timer and a test.
The actual timeout value will need tuning with real data on a 2G
impairment network and a list of real EM web pages.
BUG=610521
Committed: https://crrev.com/63cc9eb76507384c24c0217ca16595e706998955
Cr-Commit-Position: refs/heads/master@{#403543}
Patch Set 1 #
Total comments: 20
Patch Set 2 : CR feedback per DougArnett and DewittJ #
Total comments: 2
Patch Set 3 : Constant renaming per DewittJ #
Messages
Total messages: 13 (4 generated)
petewil@chromium.org changed reviewers: + dewittj@chromium.org, dougarnett@chromium.org
lgtm with few comments https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:30: // maintenance window. TODO(petewil): Find the optimal timeout based maybe line break before TODO https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:31: // on data for 2G connections and common EM websites. another TODO might be to move this value into Policy object https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:93: // Return control to the scheduler when there is no more to do. Think this comment needs some rewording. Perhaps // Let scheduler know we are now done with processing. https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:133: // How long to wait for an offliner before giving up. ... an offliner request ... ? https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:36: const bool POWER_NOT_REQUIRED = false; nit - this declaration is bit confusing to me - vs. having true constant matching sense of the parm and then !constant in the call (eg, !POWER_REQUIRED).
https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:30: // maintenance window. TODO(petewil): Find the optimal timeout based Please make a bug specifically to find this number out, and reference it here. https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:32: long OFFLINER_TIMEOUT_SECONDS = 180; If this is exactly the marshmallow timeout, then the timer might not actually fire when you want it to. Perhaps use something slightly shorter like 180 - 30? https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:88: void RequestCoordinator::StopProcessing() { as far as I can tell, this is only called in tests. Is that true? If not, how does it interact with RequestQueueEpty()? Should that method call StopProcessing? https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:134: long offliner_timeout_; Please use base::TimeDelta here so the units are unambiguous.
CR feedback per DougArnett and DewittJ https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:30: // maintenance window. TODO(petewil): Find the optimal timeout based On 2016/06/30 20:38:33, dewittj wrote: > Please make a bug specifically to find this number out, and reference it here. Done. https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:30: // maintenance window. TODO(petewil): Find the optimal timeout based On 2016/06/30 19:16:48, dougarnett wrote: > maybe line break before TODO Done. https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:31: // on data for 2G connections and common EM websites. On 2016/06/30 19:16:48, dougarnett wrote: > another TODO might be to move this value into Policy object Done. https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:32: long OFFLINER_TIMEOUT_SECONDS = 180; On 2016/06/30 20:38:33, dewittj wrote: > If this is exactly the marshmallow timeout, then the timer might not actually > fire when you want it to. Perhaps use something slightly shorter like 180 - 30? Done. https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:88: void RequestCoordinator::StopProcessing() { On 2016/06/30 20:38:33, dewittj wrote: > as far as I can tell, this is only called in tests. Is that true? If not, how > does it interact with RequestQueueEpty()? Should that method call > StopProcessing? The primary purpose of this function is to cancel a render in progress. This is called by the watchdog timer (line 161 in this file in this version of the patch). It is also a top level API to the request coordinator so that we can stop processing if we realize later that the user has picked up their phone and started interacting with it, and would like their CPU back. (DougArnett has a design doc for that he just sent out this week). RequestQueueEmpty and this method both call scheduler_callback_.Run() to return control to the scheduler, so we don't really need to have one call the other. If you think it makes the code more readable, I could make a new function to return control to the scheduler that both call. https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:93: // Return control to the scheduler when there is no more to do. On 2016/06/30 19:16:48, dougarnett wrote: > Think this comment needs some rewording. Perhaps > // Let scheduler know we are now done with processing. Done (changed here and in RequestQueueEmpty) https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:133: // How long to wait for an offliner before giving up. On 2016/06/30 19:16:48, dougarnett wrote: > ... an offliner request ... ? Done. https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:134: long offliner_timeout_; On 2016/06/30 20:38:33, dewittj wrote: > Please use base::TimeDelta here so the units are unambiguous. Done. https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:36: const bool POWER_NOT_REQUIRED = false; On 2016/06/30 19:16:48, dougarnett wrote: > nit - this declaration is bit confusing to me - vs. having true constant > matching sense of the parm and then !constant in the call (eg, !POWER_REQUIRED). Done.
lgtm, sorry for confusion https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:88: void RequestCoordinator::StopProcessing() { On 2016/07/01 17:16:54, Pete Williamson wrote: > On 2016/06/30 20:38:33, dewittj wrote: > > as far as I can tell, this is only called in tests. Is that true? If not, how > > does it interact with RequestQueueEpty()? Should that method call > > StopProcessing? > > The primary purpose of this function is to cancel a render in progress. > > This is called by the watchdog timer (line 161 in this file in this version of > the patch). Ah right, turns out code search ignores new additions in codereview! :) > It is also a top level API to the request coordinator so that we > can stop processing if we realize later that the user has picked up their phone > and started interacting with it, and would like their CPU back. (DougArnett has > a design doc for that he just sent out this week). > > RequestQueueEmpty and this method both call scheduler_callback_.Run() to return > control to the scheduler, so we don't really need to have one call the other. > If you think it makes the code more readable, I could make a new function to > return control to the scheduler that both call. The reason I ask is that RequestQueueEmpty and this function do slightly different things but the end result should be the same. For example, scheduler_->Unschedule() is not called here. Should it be? https://codereview.chromium.org/2119613002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2119613002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:37: const bool POWER_REQUIRED = true; nit: looks like there is inconsistent naming: kFooScheme and FOO_SCHEME. Could we make it consistent?
https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2119613002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:88: void RequestCoordinator::StopProcessing() { On 2016/07/01 17:27:48, dewittj wrote: > On 2016/07/01 17:16:54, Pete Williamson wrote: > > On 2016/06/30 20:38:33, dewittj wrote: > > > as far as I can tell, this is only called in tests. Is that true? If not, > how > > > does it interact with RequestQueueEpty()? Should that method call > > > StopProcessing? > > > > The primary purpose of this function is to cancel a render in progress. > > > > This is called by the watchdog timer (line 161 in this file in this version of > > the patch). > Ah right, turns out code search ignores new additions in codereview! :) > > > It is also a top level API to the request coordinator so that we > > can stop processing if we realize later that the user has picked up their > phone > > and started interacting with it, and would like their CPU back. (DougArnett > has > > a design doc for that he just sent out this week). > > > > RequestQueueEmpty and this method both call scheduler_callback_.Run() to > return > > control to the scheduler, so we don't really need to have one call the other. > > If you think it makes the code more readable, I could make a new function to > > return control to the scheduler that both call. > The reason I ask is that RequestQueueEmpty and this function do slightly > different things but the end result should be the same. For example, > scheduler_->Unschedule() is not called here. Should it be? I presume that StopProcessing is being called because the user wants to use the device. In that case, we still want to do pre-rendering, we just want to do it another time, so we don't call unschedule() to remove the pending schedule appointment On the other hand, if the request queue is empty, there is just no work to be done, so we remove the safety schedule appointment so we don't get scheduled again. This is explained in the design docs, but may not be well explained in the code, let me know there is anything specific you think I could to do explain it better in the comments. https://codereview.chromium.org/2119613002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2119613002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:37: const bool POWER_REQUIRED = true; On 2016/07/01 17:27:48, dewittj wrote: > nit: > > looks like there is inconsistent naming: kFooScheme and FOO_SCHEME. Could we > make it consistent? Done. Right you are, old habits creeping back in. Renamed to kFooScheme after a quick check of the chromium code style guide.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2119613002/#ps40001 (title: "Constant renaming per DewittJ")
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 ========== Adds a prerenderer watchdog timer. The prerenderer can take too long on a page, when it does, we should cancel the pre-render in progress. This change adds a timer and a test. The actual timeout value will need tuning with real data on a 2G impairment network and a list of real EM web pages. BUG=610521 ========== to ========== Adds a prerenderer watchdog timer. The prerenderer can take too long on a page, when it does, we should cancel the pre-render in progress. This change adds a timer and a test. The actual timeout value will need tuning with real data on a 2G impairment network and a list of real EM web pages. BUG=610521 Committed: https://crrev.com/63cc9eb76507384c24c0217ca16595e706998955 Cr-Commit-Position: refs/heads/master@{#403543} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/63cc9eb76507384c24c0217ca16595e706998955 Cr-Commit-Position: refs/heads/master@{#403543} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
