Check time budget before starting new requests, and unittest.

Check time budget before starting new requests, and unittest.

BUG=610521
Committed: https://crrev.com/901652b78a269dab96fca010bdf1434587a72cfc
Cr-Commit-Position: refs/heads/master@{#407936}

[Pete Williamson, 2016-07-25 21:36:13]

petewil@chromium.org changed reviewers:
+ dougarnett@chromium.org, jianli@chromium.org

[jianli, 2016-07-25 21:50:38]

https://codereview.chromium.org/2178143002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2178143002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:47:
operation_start_time_(base::Time::Now()),
It seems to me that this initialization was not needed.

https://codereview.chromium.org/2178143002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:141: // the "safety"
task, so we get called again next time.
I found it hard to understand what the "safety" task was meant for. Could you
please rephrase it?

https://codereview.chromium.org/2178143002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:146:
scheduler_callback_.Run(true);
Do we need to return here?

[Pete Williamson, 2016-07-25 22:01:33]

https://codereview.chromium.org/2178143002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2178143002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:47:
operation_start_time_(base::Time::Now()),
On 2016/07/25 21:50:38, jianli wrote:
> It seems to me that this initialization was not needed.

Done.

https://codereview.chromium.org/2178143002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:141: // the "safety"
task, so we get called again next time.
On 2016/07/25 21:50:38, jianli wrote:
> I found it hard to understand what the "safety" task was meant for. Could you
> please rephrase it?

Done.  I was referring to something in the design docs, but it is better if the
code is standalone.

https://codereview.chromium.org/2178143002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:146:
scheduler_callback_.Run(true);
On 2016/07/25 21:50:38, jianli wrote:
> Do we need to return here?

Oh my goodness, you're right!  Added.

[Pete Williamson, 2016-07-25 23:59:43]

re-pub

[Pete Williamson, 2016-07-26 00:00:37]

this time with the actual patch ^_^

[jianli, 2016-07-26 00:01:11]

lgtm

[dougarnett, 2016-07-26 16:11:34]

https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:139: // If there is
no time left in the budget, return to the scheduler.
So are we willing to overrun the time budget by just less than page time budget?
Perhaps more specifically, do you see the page budget as a hard limit (timeout)
and the processing limit as a soft limit (just up front check) or will we also
set a timeout for the processing time?

[Pete Williamson, 2016-07-26 18:42:19]

https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:139: // If there is
no time left in the budget, return to the scheduler.
On 2016/07/26 16:11:34, dougarnett wrote:
> So are we willing to overrun the time budget by just less than page time
budget?
> Perhaps more specifically, do you see the page budget as a hard limit
(timeout)
> and the processing limit as a soft limit (just up front check) or will we also
> set a timeout for the processing time?
> 

We are totally willing to overrun the time budget here.  The problem is that we
don't really have a good idea how long pages will take, 2 minutes might be par
for the course for a poor 2G network.  I expect to tune all of these numbers as
time goes by.  When we tune them, we might decide not to start a page unless a
we know that we are likely to finish it, and we might change this logic.  For
the moment, I have fairly lenient settings for while we are testing out the
product.

We can also tune this by tuning the budget number.  For instance, if we think it
will take two minutes, but we have a 3 minute window, we can make the budget
limit one minute, so we refuse to start a request unless we expect to finish it
in the worst case.

There's also the thought of worst case vs average case.  If worst case is 2
minutes, we will only ever get one page done in a 3 minute maintenance window if
we refuse to start with less than two minutes left.  However, the average case
might be better, so we might get 2-3 pages in the same time if we don't have
that limit.

[Dmitry Titov, 2016-07-26 19:55:39]

On 2016/07/26 18:42:19, Pete Williamson wrote:
>
https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
> File components/offline_pages/background/request_coordinator.cc (right):
> 
>
https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
> components/offline_pages/background/request_coordinator.cc:139: // If there is
> no time left in the budget, return to the scheduler.
> On 2016/07/26 16:11:34, dougarnett wrote:
> > So are we willing to overrun the time budget by just less than page time
> budget?
> > Perhaps more specifically, do you see the page budget as a hard limit
> (timeout)
> > and the processing limit as a soft limit (just up front check) or will we
also
> > set a timeout for the processing time?
> > 
> 
> We are totally willing to overrun the time budget here.  The problem is that
we
> don't really have a good idea how long pages will take, 2 minutes might be par
> for the course for a poor 2G network.  I expect to tune all of these numbers
as
> time goes by.  When we tune them, we might decide not to start a page unless a
> we know that we are likely to finish it, and we might change this logic.  For
> the moment, I have fairly lenient settings for while we are testing out the
> product.
> 
> We can also tune this by tuning the budget number.  For instance, if we think
it
> will take two minutes, but we have a 3 minute window, we can make the budget
> limit one minute, so we refuse to start a request unless we expect to finish
it
> in the worst case.
> 
> There's also the thought of worst case vs average case.  If worst case is 2
> minutes, we will only ever get one page done in a 3 minute maintenance window
if
> we refuse to start with less than two minutes left.  However, the average case
> might be better, so we might get 2-3 pages in the same time if we don't have
> that limit.

Is there a design doc that would a) collect all the rules b) would be easy to
comment on?
There is always a danger to optimize things before we know we should and have a
lot more complicated code w/o a good trace of why exactly all the decisions were
made.

Or if we already have a doc, I'd be glad to have a link here.

[Pete Williamson, 2016-07-26 20:01:00]

On 2016/07/26 19:55:39, Dmitry Titov wrote:
> On 2016/07/26 18:42:19, Pete Williamson wrote:
> >
>
https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
> > File components/offline_pages/background/request_coordinator.cc (right):
> > 
> >
>
https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
> > components/offline_pages/background/request_coordinator.cc:139: // If there
is
> > no time left in the budget, return to the scheduler.
> > On 2016/07/26 16:11:34, dougarnett wrote:
> > > So are we willing to overrun the time budget by just less than page time
> > budget?
> > > Perhaps more specifically, do you see the page budget as a hard limit
> > (timeout)
> > > and the processing limit as a soft limit (just up front check) or will we
> also
> > > set a timeout for the processing time?
> > > 
> > 
> > We are totally willing to overrun the time budget here.  The problem is that
> we
> > don't really have a good idea how long pages will take, 2 minutes might be
par
> > for the course for a poor 2G network.  I expect to tune all of these numbers
> as
> > time goes by.  When we tune them, we might decide not to start a page unless
a
> > we know that we are likely to finish it, and we might change this logic. 
For
> > the moment, I have fairly lenient settings for while we are testing out the
> > product.
> > 
> > We can also tune this by tuning the budget number.  For instance, if we
think
> it
> > will take two minutes, but we have a 3 minute window, we can make the budget
> > limit one minute, so we refuse to start a request unless we expect to finish
> it
> > in the worst case.
> > 
> > There's also the thought of worst case vs average case.  If worst case is 2
> > minutes, we will only ever get one page done in a 3 minute maintenance
window
> if
> > we refuse to start with less than two minutes left.  However, the average
case
> > might be better, so we might get 2-3 pages in the same time if we don't have
> > that limit.
> 
> Is there a design doc that would a) collect all the rules b) would be easy to
> comment on?
> There is always a danger to optimize things before we know we should and have
a
> lot more complicated code w/o a good trace of why exactly all the decisions
were
> made.
> 
> Or if we already have a doc, I'd be glad to have a link here.

Relevant docs are here:
https://docs.google.com/document/d/1v02mL0v7MZHNiYaxFWUseru_SCB_3hxftZooGqWLSqI
-and-
https://docs.google.com/document/d/1v02mL0v7MZHNiYaxFWUseru_SCB_3hxftZooGqWLSqI

[dougarnett, 2016-07-26 20:26:44]

https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:139: // If there is
no time left in the budget, return to the scheduler.
On 2016/07/26 18:42:19, Pete Williamson wrote:
> On 2016/07/26 16:11:34, dougarnett wrote:
> > So are we willing to overrun the time budget by just less than page time
> budget?
> > Perhaps more specifically, do you see the page budget as a hard limit
> (timeout)
> > and the processing limit as a soft limit (just up front check) or will we
also
> > set a timeout for the processing time?
> > 
> 
> We are totally willing to overrun the time budget here.  The problem is that
we
> don't really have a good idea how long pages will take, 2 minutes might be par
> for the course for a poor 2G network.  I expect to tune all of these numbers
as
> time goes by.  When we tune them, we might decide not to start a page unless a
> we know that we are likely to finish it, and we might change this logic.  For
> the moment, I have fairly lenient settings for while we are testing out the
> product.
> 
> We can also tune this by tuning the budget number.  For instance, if we think
it
> will take two minutes, but we have a 3 minute window, we can make the budget
> limit one minute, so we refuse to start a request unless we expect to finish
it
> in the worst case.
> 
> There's also the thought of worst case vs average case.  If worst case is 2
> minutes, we will only ever get one page done in a 3 minute maintenance window
if
> we refuse to start with less than two minutes left.  However, the average case
> might be better, so we might get 2-3 pages in the same time if we don't have
> that limit.

I guess my meta point was that we have two things named "budget" that are used
differently which I didn't expect so want to check if behavior is what you
intended. Sounds like it is - so then I recommend making that a bit more clear
even if it is just terminology tweak such as *ProcessingTimeBudget* vs.
*PageTimeLimit* or some comment treatment.

[Pete Williamson, 2016-07-26 20:37:12]

On 2016/07/26 20:01:00, Pete Williamson wrote:
> On 2016/07/26 19:55:39, Dmitry Titov wrote:
> > On 2016/07/26 18:42:19, Pete Williamson wrote:
> > >
> >
>
https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
> > > File components/offline_pages/background/request_coordinator.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
> > > components/offline_pages/background/request_coordinator.cc:139: // If
there
> is
> > > no time left in the budget, return to the scheduler.
> > > On 2016/07/26 16:11:34, dougarnett wrote:
> > > > So are we willing to overrun the time budget by just less than page time
> > > budget?
> > > > Perhaps more specifically, do you see the page budget as a hard limit
> > > (timeout)
> > > > and the processing limit as a soft limit (just up front check) or will
we
> > also
> > > > set a timeout for the processing time?
> > > > 
> > > 
> > > We are totally willing to overrun the time budget here.  The problem is
that
> > we
> > > don't really have a good idea how long pages will take, 2 minutes might be
> par
> > > for the course for a poor 2G network.  I expect to tune all of these
numbers
> > as
> > > time goes by.  When we tune them, we might decide not to start a page
unless
> a
> > > we know that we are likely to finish it, and we might change this logic. 
> For
> > > the moment, I have fairly lenient settings for while we are testing out
the
> > > product.
> > > 
> > > We can also tune this by tuning the budget number.  For instance, if we
> think
> > it
> > > will take two minutes, but we have a 3 minute window, we can make the
budget
> > > limit one minute, so we refuse to start a request unless we expect to
finish
> > it
> > > in the worst case.
> > > 
> > > There's also the thought of worst case vs average case.  If worst case is
2
> > > minutes, we will only ever get one page done in a 3 minute maintenance
> window
> > if
> > > we refuse to start with less than two minutes left.  However, the average
> case
> > > might be better, so we might get 2-3 pages in the same time if we don't
have
> > > that limit.
> > 
> > Is there a design doc that would a) collect all the rules b) would be easy
to
> > comment on?
> > There is always a danger to optimize things before we know we should and
have
> a
> > lot more complicated code w/o a good trace of why exactly all the decisions
> were
> > made.
> > 
> > Or if we already have a doc, I'd be glad to have a link here.
> 
> Relevant docs are here:
>
https://docs.google.com/document/d/1v02mL0v7MZHNiYaxFWUseru_SCB_3hxftZooGqWLSqI
> -and-
>
https://docs.google.com/document/d/1v02mL0v7MZHNiYaxFWUseru_SCB_3hxftZooGqWLSqI

Correction, the second doc is
here:https://docs.google.com/document/d/1NLGAxS0i5ePtgYvKr_v7YLqps7JPqyNwNckC7Q-moRA

[Pete Williamson, 2016-07-26 20:59:40]

https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2178143002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:139: // If there is
no time left in the budget, return to the scheduler.
On 2016/07/26 20:26:44, dougarnett wrote:
> On 2016/07/26 18:42:19, Pete Williamson wrote:
> > On 2016/07/26 16:11:34, dougarnett wrote:
> > > So are we willing to overrun the time budget by just less than page time
> > budget?
> > > Perhaps more specifically, do you see the page budget as a hard limit
> > (timeout)
> > > and the processing limit as a soft limit (just up front check) or will we
> also
> > > set a timeout for the processing time?
> > > 
> > 
> > We are totally willing to overrun the time budget here.  The problem is that
> we
> > don't really have a good idea how long pages will take, 2 minutes might be
par
> > for the course for a poor 2G network.  I expect to tune all of these numbers
> as
> > time goes by.  When we tune them, we might decide not to start a page unless
a
> > we know that we are likely to finish it, and we might change this logic. 
For
> > the moment, I have fairly lenient settings for while we are testing out the
> > product.
> > 
> > We can also tune this by tuning the budget number.  For instance, if we
think
> it
> > will take two minutes, but we have a 3 minute window, we can make the budget
> > limit one minute, so we refuse to start a request unless we expect to finish
> it
> > in the worst case.
> > 
> > There's also the thought of worst case vs average case.  If worst case is 2
> > minutes, we will only ever get one page done in a 3 minute maintenance
window
> if
> > we refuse to start with less than two minutes left.  However, the average
case
> > might be better, so we might get 2-3 pages in the same time if we don't have
> > that limit.
> 
> I guess my meta point was that we have two things named "budget" that are used
> differently which I didn't expect so want to check if behavior is what you
> intended. Sounds like it is - so then I recommend making that a bit more clear
> even if it is just terminology tweak such as *ProcessingTimeBudget* vs.
> *PageTimeLimit* or some comment treatment.
> 

Done, see how you like new names.

[dougarnett, 2016-07-26 21:09:54]

lgtm

[Pete Williamson, 2016-07-26 21:30:34]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-07-26 21:30:35]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/2178143002/#ps40001
(title: "Tweak time limit names")

[commit-bot: I haz the power, 2016-07-26 21:31:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 22:28:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-26 22:30:47]

Description was changed from

==========
Check time budget before starting new requests, and unittest.

BUG=610521
==========

to

==========
Check time budget before starting new requests, and unittest.

BUG=610521

Committed: https://crrev.com/901652b78a269dab96fca010bdf1434587a72cfc
Cr-Commit-Position: refs/heads/master@{#407936}
==========

[commit-bot: I haz the power, 2016-07-26 22:30:48]

Patchset 3 (id:??) landed as
https://crrev.com/901652b78a269dab96fca010bdf1434587a72cfc
Cr-Commit-Position: refs/heads/master@{#407936}