Issue 357583003: Adding observable throttle logic without the signals. Defaults to current behavior.

aiolos (Not reviewing)

Let me know if there is specific testing you want done on this as well.

6 years, 6 months ago (2014-06-25 20:14:14 UTC) #1

mmenke

On 2014/06/25 20:14:14, aiolos wrote: > Let me know if there is specific testing you ...

6 years, 5 months ago (2014-06-27 14:50:46 UTC) #2

mmenke

Quick style nits. Only significant one (And why I'm sending these comments now, isntead of ...

6 years, 5 months ago (2014-06-27 15:07:35 UTC) #3

aiolos (Not reviewing)

On 2014/06/27 14:50:46, mmenke wrote: > On 2014/06/25 20:14:14, aiolos wrote: > > Let me ...

6 years, 5 months ago (2014-06-27 16:29:29 UTC) #4

aiolos (Not reviewing)

On 2014/06/27 15:07:35, mmenke wrote: > Quick style nits. Only significant one (And why I'm ...

6 years, 5 months ago (2014-06-27 17:04:27 UTC) #5

mmenke

Should have unit tests for this. https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/resource_scheduler.cc#newcode234 content/browser/loader/resource_scheduler.cc:234: Client(ResourceScheduler* scheduler) Single ...

6 years, 5 months ago (2014-06-30 19:20:27 UTC) #6

aiolos (Not reviewing)

On 2014/06/30 19:20:27, mmenke wrote: > Should have unit tests for this. All current tests ...

6 years, 5 months ago (2014-06-30 20:48:35 UTC) #7

On 2014/06/30 19:20:27, mmenke wrote:
> Should have unit tests for this.

All current tests are at the ResourceScheduler level, not the Client level. I
assume this is because the Client is an internal Private class. Is there a
specific way you'd like things tested?
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> File content/browser/loader/resource_scheduler.cc (right):
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:234: Client(ResourceScheduler*
> scheduler)
> Single argument constructors should be declared as explicit.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:242: {
> nit:  Open brace should go on previous line.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:258: UNTHROTTLED = 2,
> Suggest not assigning explicit numbers to these, unless they're really needed
> for some reason.  Looks like you're only depending on their order (And I
suggest
> you get rid of that, below), and not their actual values.

I mostly did this because the other enum in the Client assigned explicit numbers
to them, and I opted to match the file. Easily changed if avoiding explicit
values is more desired than this specific consistency. 

> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:300: }
> optional nit:  Accessors are generally put on one line, when they fit:
> 
> bool is_loaded() const { return is_loaded_; }
> 
> etc.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:303: if (audible != is_audible_)
{
> optional nit:  Early returns are generally preferred in the Chrome code base,
> all else being equal, as they reduce nesting, which can make for more readable
> code.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:309: void
OnObservabilityChanged()
> {
> optional nit:  Suggest putting this after the two functions that call it - in
> the middle of them just seems marginally harder to follow.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:310: if (is_active() &&
> throttle_state_ != UNTHROTTLED) {
> Checking the old throttle state isn't needed, since SetThrottleState already
has
> a check for leaving the state the same.  In general, best to keep code as
simple
> as possible - often results in faster code, too, since code size is pretty
> important on modern CPUs, due to cache sizes.
> 
> Suggest just:
> 
> if (is_active) {
>   SetThrottleState(...)
> } else if (blah || blah) {
>   SetThrottleState(...)
> } else...
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:313: if
> (!scheduler_->observable_clients_loaded() || !kShouldCoalesce) {
> Seems weird to be checking a const.

This is the flag used to keep Coalescing off until the signals are in place, and
throttling has already landed with no issues. Should I use SHOULD_COALESCE or
follow another naming convention instead to make this more obvious?
(kShouldThrottled is similarly used to keep throttling off until all needed
signals have landed, and I assume it should follow the same convention chosen.)
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:322: if (visible != is_visible_)
{
> optional nit:  Again, suggest an early return.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:379: //   COALESCED -> THROTTLED
> So when there's an active tab loading, a non-observable, finished loading
client
> can either be in COALESCED or THROTTLED state, depending on whether it
finished
> loading before or after the active tab started loading?

When there are no active tabs loading, all clients should be either COALESCED or
UNTHROTTLED. But if a Client becomes active, all background clients should
switch states so the don't contend with the active loading tab. This could
happen, for example, if a user is on a loaded page with background tabs still
loading, and they navigates to a new page in the frontmost tab. 
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:381: // woken up by a foreground
> request and then back into COALESCED
> Don't we throttle background clients when a foreground client starts loading? 
> I'm still quite confused.

Yes, you're right. This only happens when the Coalesced Client is woken up by
the timer. When a foreground Client starts loading, the Coalesced Clients become
THROTTLED, which will only allow them to send one request out with the active
Client's request. I'll fix the comment.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:383: if (throttle !=
> throttle_state_) {
> optional nit:  Suggest early return.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:386: if (throttle_state_ >
> old_throttle_state) {
> Suggest getting rid of this check, since ShouldStartRequest returns
> DO_NOT_START_REQUEST_AND_STOP_SEARCHING if we should throttle all requests,
> anyways.
> 
> Seems like premature optimization, and my general feeling is that if a check
is
> redundant, there's a fair chance it could be missed in future CLs that change
> the logic it attempts to duplicate, resulting in breakages.
> 
I think you have this backwards. The higher throttle states are the more
importnat Clients, not the more throttled ones. We really need this since
otherwise a Client that becomes active won't start loading requests until some
other action pokes the Client. If I remove this call and a PAUSED tab is
switched to by the user, there is no guarantee that it will start loading. Would
it be more intuitive if I switched the enum around so that UNTHROTTLED was the
lowest value instead of the highest? Either way, I think this means I should add
a comment here to be more clear.

>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:389: // TODO: (aiolos) Stop any
> started but not inflght requests when
> nit:  "TODO(aiolos):"
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:390: //       switching to
stricter
> throttle state?
> nit:  -2 indent.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:413: throttle_state_ = COALESCED;
> These should probably use SetThrottleState, even though they aren't kicking
off
> any loads.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:521: //  COALESCED Clients not on
a
> heartbeat
> I really don't understand this..  Just above you say "COALESCED Clients on a
> heartbeat or when the network is woken up".

I'd like to make the comments more clear. What about removing the earlier
references to COALESCED Clients, and have the following after PAUSED?

COALESCED Clients act like PAUSED requests with the following exceptions:
On a (currently 5 second) heart beat, they load all requests as an UNTHROTTLED
Client, and then return to being PAUSED. When an active Client makes a request,
they are THROTTLED until the active Client finishes loading.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:524: //
> nit:  Remove final blank comment line.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:624: bool is_visible_;
> optional nit:  Suggest putting is_audible_ and is_visible_ next to each other,
> since they're so closely related.

I assume they should at least be alphabetical in relation to each other?

mmenke

Quick responses. I'll respond to your other responses tomorrow (Just the comments about testing and ...

6 years, 5 months ago (2014-06-30 21:18:15 UTC) #8

Quick responses.  I'll respond to your other responses tomorrow (Just the
comments about testing and documentation remain, I believe).

On 2014/06/30 20:48:35, aiolos wrote:
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:258: UNTHROTTLED = 2,
> > Suggest not assigning explicit numbers to these, unless they're really
needed
> > for some reason.  Looks like you're only depending on their order (And I
> suggest
> > you get rid of that, below), and not their actual values.
> 
> I mostly did this because the other enum in the Client assigned explicit
numbers
> to them, and I opted to match the file. Easily changed if avoiding explicit
> values is more desired than this specific consistency. 

Fair point.  I'd prefer to get rid of them in both locations, but getting rid of
the old values is beyond the scope of this CL.  I'd say not include them, and we
can look rid of getting rid of the old ones in another CL (I'm happy to do this
myself).  Simpler not to start with them, rather than get rid of them later,
which will require validating they're not important.

>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:313: if
> > (!scheduler_->observable_clients_loaded() || !kShouldCoalesce) {
> > Seems weird to be checking a const.
> 
> This is the flag used to keep Coalescing off until the signals are in place,
and
> throttling has already landed with no issues. Should I use SHOULD_COALESCE or
> follow another naming convention instead to make this more obvious?
> (kShouldThrottled is similarly used to keep throttling off until all needed
> signals have landed, and I assume it should follow the same convention
chosen.)

So this is just a temporary constant to disable the code until its ready to be
hooked up?  I'd argue that it's better to have TODOs on the necessary lines,
than a global const to disable stuff.  As-is, you can't test that code, anyways,
and I'm reluctant to sign off on untested code.

>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:386: if (throttle_state_ >
> > old_throttle_state) {
> > Suggest getting rid of this check, since ShouldStartRequest returns
> > DO_NOT_START_REQUEST_AND_STOP_SEARCHING if we should throttle all requests,
> > anyways.
> > 
> > Seems like premature optimization, and my general feeling is that if a check
> is
> > redundant, there's a fair chance it could be missed in future CLs that
change
> > the logic it attempts to duplicate, resulting in breakages.
> > 
> I think you have this backwards. The higher throttle states are the more
> importnat Clients, not the more throttled ones. We really need this since
> otherwise a Client that becomes active won't start loading requests until some
> other action pokes the Client. If I remove this call and a PAUSED tab is
> switched to by the user, there is no guarantee that it will start loading.
Would
> it be more intuitive if I switched the enum around so that UNTHROTTLED was the
> lowest value instead of the highest? Either way, I think this means I should
add
> a comment here to be more clear.

I think you're misunderstanding me.  I meant get rid of the "if" statement, and
just always call LoadAnyStartablePendingRequests unconditionally.  If the new
throttle state is more restrictive than the old one, it won't do anything,
anyways.  The comparison doesn't really get us anything.

>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:624: bool is_visible_;
> > optional nit:  Suggest putting is_audible_ and is_visible_ next to each
other,
> > since they're so closely related.
> 
> I assume they should at least be alphabetical in relation to each other?

There's no rule about putting variables in alphabetical order (And in fact, you
often can't do this, when owned objects depend on each other).  I think it's
generally a good idea to put related values together, or put them in order of
use.  If you prefer the current order, that's fine with me, just thought it was
a little unusual.

aiolos (Not reviewing)

On 2014/06/30 21:18:15, mmenke wrote: > Quick responses. I'll respond to your other responses tomorrow ...

6 years, 5 months ago (2014-06-30 21:54:31 UTC) #9

On 2014/06/30 21:18:15, mmenke wrote:
> Quick responses.  I'll respond to your other responses tomorrow (Just the
> comments about testing and documentation remain, I believe).

Thanks.
> 
> On 2014/06/30 20:48:35, aiolos wrote:
> >
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:258: UNTHROTTLED = 2,
> > > Suggest not assigning explicit numbers to these, unless they're really
> needed
> > > for some reason.  Looks like you're only depending on their order (And I
> > suggest
> > > you get rid of that, below), and not their actual values.
> > 
> > I mostly did this because the other enum in the Client assigned explicit
> numbers
> > to them, and I opted to match the file. Easily changed if avoiding explicit
> > values is more desired than this specific consistency. 
> 
> Fair point.  I'd prefer to get rid of them in both locations, but getting rid
of
> the old values is beyond the scope of this CL.  I'd say not include them, and
we
> can look rid of getting rid of the old ones in another CL (I'm happy to do
this
> myself).  Simpler not to start with them, rather than get rid of them later,
> which will require validating they're not important.

Sounds good. I've spent a lot of time in this code recently, so it would be easy
for me to double check if there are any dependancies, and then send you a CL to
review for changing the other enum.
> 
> >
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:313: if
> > > (!scheduler_->observable_clients_loaded() || !kShouldCoalesce) {
> > > Seems weird to be checking a const.
> > 
> > This is the flag used to keep Coalescing off until the signals are in place,
> and
> > throttling has already landed with no issues. Should I use SHOULD_COALESCE
or
> > follow another naming convention instead to make this more obvious?
> > (kShouldThrottled is similarly used to keep throttling off until all needed
> > signals have landed, and I assume it should follow the same convention
> chosen.)
> 
> So this is just a temporary constant to disable the code until its ready to be
> hooked up?  I'd argue that it's better to have TODOs on the necessary lines,
> than a global const to disable stuff.  As-is, you can't test that code,
anyways,
> and I'm reluctant to sign off on untested code.

I can understand not signing off on code that hasn't been tested, but I'm
curious if by "untested" you mean a) That the code was not tested before it is
submitted, or b) That the code is submitted without the tests that have been run
on it to verify correctness? 

It's easier to test locally with a flag that I can flip than to change all of
the lines each time I want to switch between uploading a new CL and testing my
changes. It also reduces the chances that I miss one of the changes while
testing or uploading a CL. I would strongly prefer to keep it as is.

Would you prefer to have the ResourceScheduler changes to review at the same
time as the Client changes? I opted to do it in two phases so there wasn't just
one massive CL to be reviewed, and to get feedback before the whole CL was
finished. Two other options would be:

a) I could also break up the next CL so that I can include the active/loaded
hooks that can be used to add ResourceScheduler level testing to this CL. That
would leave the Coalescing logic for another CL.
b) We can get this CL to a point that you're happy with it besides testing, move
onto the review for the next CL, which includes all of the testing on the
ResourceScheduler level, and only land them together.
> 
> >
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:386: if (throttle_state_ >
> > > old_throttle_state) {
> > > Suggest getting rid of this check, since ShouldStartRequest returns
> > > DO_NOT_START_REQUEST_AND_STOP_SEARCHING if we should throttle all
requests,
> > > anyways.
> > > 
> > > Seems like premature optimization, and my general feeling is that if a
check
> > is
> > > redundant, there's a fair chance it could be missed in future CLs that
> change
> > > the logic it attempts to duplicate, resulting in breakages.
> > > 
> > I think you have this backwards. The higher throttle states are the more
> > importnat Clients, not the more throttled ones. We really need this since
> > otherwise a Client that becomes active won't start loading requests until
some
> > other action pokes the Client. If I remove this call and a PAUSED tab is
> > switched to by the user, there is no guarantee that it will start loading.
> Would
> > it be more intuitive if I switched the enum around so that UNTHROTTLED was
the
> > lowest value instead of the highest? Either way, I think this means I should
> add
> > a comment here to be more clear.
> 
> I think you're misunderstanding me.  I meant get rid of the "if" statement,
and
> just always call LoadAnyStartablePendingRequests unconditionally.  If the new
> throttle state is more restrictive than the old one, it won't do anything,
> anyways.  The comparison doesn't really get us anything.

I was misunderstanding you. Sounds good.
> 
> >
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:624: bool is_visible_;
> > > optional nit:  Suggest putting is_audible_ and is_visible_ next to each
> other,
> > > since they're so closely related.
> > 
> > I assume they should at least be alphabetical in relation to each other?
> 
> There's no rule about putting variables in alphabetical order (And in fact,
you
> often can't do this, when owned objects depend on each other).  I think it's
> generally a good idea to put related values together, or put them in order of
> use.  If you prefer the current order, that's fine with me, just thought it
was
> a little unusual.

Easily changed.

mmenke

On 2014/06/30 21:54:31, aiolos wrote: > On 2014/06/30 21:18:15, mmenke wrote: > > Quick responses. ...

6 years, 5 months ago (2014-06-30 22:15:27 UTC) #10

On 2014/06/30 21:54:31, aiolos wrote:
> On 2014/06/30 21:18:15, mmenke wrote:
> > Quick responses.  I'll respond to your other responses tomorrow (Just the
> > comments about testing and documentation remain, I believe).
> 
> Thanks.
> > 
> > On 2014/06/30 20:48:35, aiolos wrote:
> > >
> >
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > > content/browser/loader/resource_scheduler.cc:258: UNTHROTTLED = 2,
> > > > Suggest not assigning explicit numbers to these, unless they're really
> > needed
> > > > for some reason.  Looks like you're only depending on their order (And I
> > > suggest
> > > > you get rid of that, below), and not their actual values.
> > > 
> > > I mostly did this because the other enum in the Client assigned explicit
> > numbers
> > > to them, and I opted to match the file. Easily changed if avoiding
explicit
> > > values is more desired than this specific consistency. 
> > 
> > Fair point.  I'd prefer to get rid of them in both locations, but getting
rid
> of
> > the old values is beyond the scope of this CL.  I'd say not include them,
and
> we
> > can look rid of getting rid of the old ones in another CL (I'm happy to do
> this
> > myself).  Simpler not to start with them, rather than get rid of them later,
> > which will require validating they're not important.
> 
> Sounds good. I've spent a lot of time in this code recently, so it would be
easy
> for me to double check if there are any dependancies, and then send you a CL
to
> review for changing the other enum.

Sounds good to me.  I didn't want to just throw more work your way, but expect
it to be a pretty trivial effort.

>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > > content/browser/loader/resource_scheduler.cc:313: if
> > > > (!scheduler_->observable_clients_loaded() || !kShouldCoalesce) {
> > > > Seems weird to be checking a const.
> > > 
> > > This is the flag used to keep Coalescing off until the signals are in
place,
> > and
> > > throttling has already landed with no issues. Should I use SHOULD_COALESCE
> or
> > > follow another naming convention instead to make this more obvious?
> > > (kShouldThrottled is similarly used to keep throttling off until all
needed
> > > signals have landed, and I assume it should follow the same convention
> > chosen.)
> > 
> > So this is just a temporary constant to disable the code until its ready to
be
> > hooked up?  I'd argue that it's better to have TODOs on the necessary lines,
> > than a global const to disable stuff.  As-is, you can't test that code,
> anyways,
> > and I'm reluctant to sign off on untested code.
> 
> I can understand not signing off on code that hasn't been tested, but I'm
> curious if by "untested" you mean a) That the code was not tested before it is
> submitted, or b) That the code is submitted without the tests that have been
run
> on it to verify correctness? 

By "untested" I mean code without unit tests (Which will be automatically run on
every CL, and guard against regressions and potential brokenness in the initial
CL itself).  In general, pretty much everything should have unit tests.  There
are things that don't have unit tests tested well (Or at all), particularly when
it comes to the renderer process, and cross-process interactions, but we should
strive not to add to them.

And these tests should be included in the CL that adds the code, so it comes
with its own regression protection.  At times this means landing larger CLs,
which are more difficult to review, but protecting ourselves against regressions
is worth that cost.

> It's easier to test locally with a flag that I can flip than to change all of
> the lines each time I want to switch between uploading a new CL and testing my
> changes. It also reduces the chances that I miss one of the changes while
> testing or uploading a CL. I would strongly prefer to keep it as is.
> 
> Would you prefer to have the ResourceScheduler changes to review at the same
> time as the Client changes? I opted to do it in two phases so there wasn't
just
> one massive CL to be reviewed, and to get feedback before the whole CL was
> finished. Two other options would be:
> 
> a) I could also break up the next CL so that I can include the active/loaded
> hooks that can be used to add ResourceScheduler level testing to this CL. That
> would leave the Coalescing logic for another CL.
> b) We can get this CL to a point that you're happy with it besides testing,
move
> onto the review for the next CL, which includes all of the testing on the
> ResourceScheduler level, and only land them together.

I think both of these options make sense, and I'm fine with either.  If I were
doing it myself, I'd probably go with with "a", but if "b" works better for you,
that's perfectly fine.

Another option is to separate out the Client into its own file, and write tests
just for that, though I think that will be a bit more work for you than the
options you suggest, and don't see any reason to prefer it.

mmenke

What are we getting from the 5-second heartbeat that we wouldn't get from just throttling ...

6 years, 5 months ago (2014-07-01 17:25:39 UTC) #11

What are we getting from the 5-second heartbeat that we wouldn't get from just
throttling unobservable clients as needed?

On 2014/06/30 20:48:35, aiolos wrote:
> On 2014/06/30 19:20:27, mmenke wrote:
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:379: //   COALESCED ->
THROTTLED
> > So when there's an active tab loading, a non-observable, finished loading
> client
> > can either be in COALESCED or THROTTLED state, depending on whether it
> finished
> > loading before or after the active tab started loading?
> 
> When there are no active tabs loading, all clients should be either COALESCED
or
> UNTHROTTLED. But if a Client becomes active, all background clients should
> switch states so the don't contend with the active loading tab. This could
> happen, for example, if a user is on a loaded page with background tabs still
> loading, and they navigates to a new page in the frontmost tab.

It looks like we currently lose track of whether a client was COALESCED or
UNTHROTTLED when we switch the state to THROTTLED, which seems like a problem. 
If the only difference is that loaded unobservable clients should be COALESCED,
while unloaded unobservable clients should be UNTHROTTLED, I think we should
have the function:

ThrottleState Client::CalculateThrottleState() const {
  if (observable())
    return UNTHROTTLED;
  // May want to have the scheduler cache this value, so it doesn't have to poll
all clients.
  if (scheduler_->HasLoadingObservableClients())
    return THROTTLED;
  if (!is_loaded_)
    return UNTHROTTLED;
  return COALESCED;
}

I think this is much clearer than peppering the code with very specific state
transitions.  I'm not sure how PAUSED is supposed to fit into this picture.  If
it's a state set externally, I'd suggest a bool for it, and to check for it
here.

We could then either cache the current throttle state as we are now, so we can
check for changes and make the code to change state do so explicitly, or just
not cache it at all and call LoadAnyStartablePendingRequests() whenever it might
have changed - I'm on the fence about this.

https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:521: //  COALESCED Clients not
on
> a
> > heartbeat
> > I really don't understand this..  Just above you say "COALESCED Clients on a
> > heartbeat or when the network is woken up".
> 
> I'd like to make the comments more clear. What about removing the earlier
> references to COALESCED Clients, and have the following after PAUSED?
> 
> COALESCED Clients act like PAUSED requests with the following exceptions:
> On a (currently 5 second) heart beat, they load all requests as an UNTHROTTLED
> Client, and then return to being PAUSED. When an active Client makes a
request,
> they are THROTTLED until the active Client finishes loading.

I think this is much clearer.

https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:410: throttle_state_ = THROTTLED;
We're planning on throttling background clients that have finished loading?  So
they can never send XHRs, or load other stuff in the background, until they're
brought to the front, or activated?

How accurate is the loaded signal?

aiolos (Not reviewing)

On 2014/07/01 17:25:39, mmenke wrote: > What are we getting from the 5-second heartbeat that ...

6 years, 5 months ago (2014-07-01 18:16:42 UTC) #12

On 2014/07/01 17:25:39, mmenke wrote:
> What are we getting from the 5-second heartbeat that we wouldn't get from just
> throttling unobservable clients as needed?

The THROTTLED and PAUSED changes are for performance, but the COALESCED changes
are for battery savings. It allows us to avoid waking up the network stack every
time a background Clients makes a request. If you have active enough background
tabs currently, you could potentially keep the network stack up indefinitely.
Note that this is true while there are no active Clients as well, such as Chrome
being minimized.
> 
> On 2014/06/30 20:48:35, aiolos wrote:
> > On 2014/06/30 19:20:27, mmenke wrote:
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:379: //   COALESCED ->
> THROTTLED
> > > So when there's an active tab loading, a non-observable, finished loading
> > client
> > > can either be in COALESCED or THROTTLED state, depending on whether it
> > finished
> > > loading before or after the active tab started loading?
> > 
> > When there are no active tabs loading, all clients should be either
COALESCED
> or
> > UNTHROTTLED. But if a Client becomes active, all background clients should
> > switch states so the don't contend with the active loading tab. This could
> > happen, for example, if a user is on a loaded page with background tabs
still
> > loading, and they navigates to a new page in the frontmost tab.
> 
> It looks like we currently lose track of whether a client was COALESCED or
> UNTHROTTLED when we switch the state to THROTTLED, which seems like a problem.

> If the only difference is that loaded unobservable clients should be
COALESCED,
> while unloaded unobservable clients should be UNTHROTTLED, I think we should
> have the function:
> 
> ThrottleState Client::CalculateThrottleState() const {
>   if (observable())
>     return UNTHROTTLED;
>   // May want to have the scheduler cache this value, so it doesn't have to
poll
> all clients.
>   if (scheduler_->HasLoadingObservableClients())
>     return THROTTLED;
>   if (!is_loaded_)
>     return UNTHROTTLED;
>   return COALESCED;
> }
> 
> I think this is much clearer than peppering the code with very specific state
> transitions.  I'm not sure how PAUSED is supposed to fit into this picture. 
If
> it's a state set externally, I'd suggest a bool for it, and to check for it
> here.

PAUSED is set by the ResourceScheduler. 

Did you want me to rename observable_clients_loaded() to
HasLoadingObservableClients() ?

There are a few more variables that need to go into the state decisions, but
that should be fine. I changed OnObservabilityChanged to set the throttle state
based on state rather than the transition. I could keep that the same, or change
it to return the throttle state instead.
> 
> We could then either cache the current throttle state as we are now, so we can
> check for changes and make the code to change state do so explicitly, or just
> not cache it at all and call LoadAnyStartablePendingRequests() whenever it
might
> have changed - I'm on the fence about this.

Ah, I see. I think I'd still prefer to cache it since the throttle state appears
to be used quite a bit more often than it's changed.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:521: //  COALESCED Clients
not
> on
> > a
> > > heartbeat
> > > I really don't understand this..  Just above you say "COALESCED Clients on
a
> > > heartbeat or when the network is woken up".
> > 
> > I'd like to make the comments more clear. What about removing the earlier
> > references to COALESCED Clients, and have the following after PAUSED?
> > 
> > COALESCED Clients act like PAUSED requests with the following exceptions:
> > On a (currently 5 second) heart beat, they load all requests as an
UNTHROTTLED
> > Client, and then return to being PAUSED. When an active Client makes a
> request,
> > they are THROTTLED until the active Client finishes loading.
> 
> I think this is much clearer.
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> File content/browser/loader/resource_scheduler.cc (right):
> 
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:410: throttle_state_ = THROTTLED;
> We're planning on throttling background clients that have finished loading? 
So
> they can never send XHRs, or load other stuff in the background, until they're
> brought to the front, or activated?

Not exactly. If a Client is THROTTLED (which means an active Client is still
loading, or we haven't turned coalescing on yet), they are only allowed to load
one non-SPDY request at a time (and as many SPDY-requests as they have). I
didn't want to starve background Clients, which is why I'm not just making them
PAUSED, but I want to limit the amount of bandwidth that can be stolen from
active Clients. 
> 
> How accurate is the loaded signal?

That depends on which one we use. The ones I've been looking at seem fairly
accurate, although some of them require polling-which I'm unwilling to use. I
don't think I'd be happy using them if we were potentially starving all
background Clients. But I think for our purposes they should be fine.

mmenke

On 2014/07/01 18:16:42, aiolos wrote: > On 2014/07/01 17:25:39, mmenke wrote: > > What are ...

6 years, 5 months ago (2014-07-01 18:34:32 UTC) #13

On 2014/07/01 18:16:42, aiolos wrote:
> On 2014/07/01 17:25:39, mmenke wrote:
> > What are we getting from the 5-second heartbeat that we wouldn't get from
just
> > throttling unobservable clients as needed?
> 
> The THROTTLED and PAUSED changes are for performance, but the COALESCED
changes
> are for battery savings. It allows us to avoid waking up the network stack
every
> time a background Clients makes a request. If you have active enough
background
> tabs currently, you could potentially keep the network stack up indefinitely.
> Note that this is true while there are no active Clients as well, such as
Chrome
> being minimized.

I thought that entering and leaving high power network radio state on phones
took on the order of several seconds, so if you have to go back into high power
state for a bunch of requests, odds are that 5 seconds later only gave you a
second or two of a full power radio before you let loose the next ones.  I'm a
bit skeptical of the 5-seconds doing any good here, but I admit to being pretty
ignorant when it comes to cell radios.

> > On 2014/06/30 20:48:35, aiolos wrote:
> > > On 2014/06/30 19:20:27, mmenke wrote:
> >
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > > content/browser/loader/resource_scheduler.cc:379: //   COALESCED ->
> > THROTTLED
> > > > So when there's an active tab loading, a non-observable, finished
loading
> > > client
> > > > can either be in COALESCED or THROTTLED state, depending on whether it
> > > finished
> > > > loading before or after the active tab started loading?
> > > 
> > > When there are no active tabs loading, all clients should be either
> COALESCED
> > or
> > > UNTHROTTLED. But if a Client becomes active, all background clients should
> > > switch states so the don't contend with the active loading tab. This could
> > > happen, for example, if a user is on a loaded page with background tabs
> still
> > > loading, and they navigates to a new page in the frontmost tab.
> > 
> > It looks like we currently lose track of whether a client was COALESCED or
> > UNTHROTTLED when we switch the state to THROTTLED, which seems like a
problem.
> 
> > If the only difference is that loaded unobservable clients should be
> COALESCED,
> > while unloaded unobservable clients should be UNTHROTTLED, I think we should
> > have the function:
> > 
> > ThrottleState Client::CalculateThrottleState() const {
> >   if (observable())
> >     return UNTHROTTLED;
> >   // May want to have the scheduler cache this value, so it doesn't have to
> poll
> > all clients.
> >   if (scheduler_->HasLoadingObservableClients())
> >     return THROTTLED;
> >   if (!is_loaded_)
> >     return UNTHROTTLED;
> >   return COALESCED;
> > }
> > 
> > I think this is much clearer than peppering the code with very specific
state
> > transitions.  I'm not sure how PAUSED is supposed to fit into this picture. 
> If
> > it's a state set externally, I'd suggest a bool for it, and to check for it
> > here.
> 
> PAUSED is set by the ResourceScheduler. 
> 
> Did you want me to rename observable_clients_loaded() to
> HasLoadingObservableClients() ?

No, observable_clients_loaded is fine, I wrote that code without
looking/thinking about how things were actually set up, was just intended as an
example.

> There are a few more variables that need to go into the state decisions, but
> that should be fine. I changed OnObservabilityChanged to set the throttle
state
> based on state rather than the transition. I could keep that the same, or
change
> it to return the throttle state instead.

That sounds reasonable as-is.

>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > File content/browser/loader/resource_scheduler.cc (right):
> > 
> >
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:410: throttle_state_ =
THROTTLED;
> > We're planning on throttling background clients that have finished loading? 
> So
> > they can never send XHRs, or load other stuff in the background, until
they're
> > brought to the front, or activated?
> 
> Not exactly. If a Client is THROTTLED (which means an active Client is still
> loading, or we haven't turned coalescing on yet), they are only allowed to
load
> one non-SPDY request at a time (and as many SPDY-requests as they have). I
> didn't want to starve background Clients, which is why I'm not just making
them
> PAUSED, but I want to limit the amount of bandwidth that can be stolen from
> active Clients. 

Ahh, that does much to allay my concerns, though we should have histograms for
observable/unobservable tabs and a field trial.

aiolos (Not reviewing)

A question I had earlier that I think got lost in the mix is: are ...

6 years, 5 months ago (2014-07-01 19:01:40 UTC) #14

A question I had earlier that I think got lost in the mix is: are there specific
things you want tested for this? Or just want there to be some general test
coverage?

On 2014/07/01 18:34:32, mmenke wrote:
> On 2014/07/01 18:16:42, aiolos wrote:
> > On 2014/07/01 17:25:39, mmenke wrote:
> > > What are we getting from the 5-second heartbeat that we wouldn't get from
> just
> > > throttling unobservable clients as needed?
> > 
> > The THROTTLED and PAUSED changes are for performance, but the COALESCED
> changes
> > are for battery savings. It allows us to avoid waking up the network stack
> every
> > time a background Clients makes a request. If you have active enough
> background
> > tabs currently, you could potentially keep the network stack up
indefinitely.
> > Note that this is true while there are no active Clients as well, such as
> Chrome
> > being minimized.
> 
> I thought that entering and leaving high power network radio state on phones
> took on the order of several seconds, so if you have to go back into high
power
> state for a bunch of requests, odds are that 5 seconds later only gave you a
> second or two of a full power radio before you let loose the next ones.  I'm a
> bit skeptical of the 5-seconds doing any good here, but I admit to being
pretty
> ignorant when it comes to cell radios.

One of the future steps is to become aware of the current network state, so that
we can only coalesce when the network isn't already active from work outside of
the ResourceScheduler. The 5 seconds was suggested as a first try by tonyg. I
also don't know the network bring up times for all of the devices this would be
on.I assumed this value may also get tuned to be longer later, but that we'd
start with baby steps. I'm happy to make it longer if you think that would be a
good idea.
> 
> > > On 2014/06/30 20:48:35, aiolos wrote:
> > > > On 2014/06/30 19:20:27, mmenke wrote:
> > >
> >
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > > > content/browser/loader/resource_scheduler.cc:379: //   COALESCED ->
> > > THROTTLED
> > > > > So when there's an active tab loading, a non-observable, finished
> loading
> > > > client
> > > > > can either be in COALESCED or THROTTLED state, depending on whether it
> > > > finished
> > > > > loading before or after the active tab started loading?
> > > > 
> > > > When there are no active tabs loading, all clients should be either
> > COALESCED
> > > or
> > > > UNTHROTTLED. But if a Client becomes active, all background clients
should
> > > > switch states so the don't contend with the active loading tab. This
could
> > > > happen, for example, if a user is on a loaded page with background tabs
> > still
> > > > loading, and they navigates to a new page in the frontmost tab.
> > > 
> > > It looks like we currently lose track of whether a client was COALESCED or
> > > UNTHROTTLED when we switch the state to THROTTLED, which seems like a
> problem.
> > 
> > > If the only difference is that loaded unobservable clients should be
> > COALESCED,
> > > while unloaded unobservable clients should be UNTHROTTLED, I think we
should
> > > have the function:
> > > 
> > > ThrottleState Client::CalculateThrottleState() const {
> > >   if (observable())
> > >     return UNTHROTTLED;
> > >   // May want to have the scheduler cache this value, so it doesn't have
to
> > poll
> > > all clients.
> > >   if (scheduler_->HasLoadingObservableClients())
> > >     return THROTTLED;
> > >   if (!is_loaded_)
> > >     return UNTHROTTLED;
> > >   return COALESCED;
> > > }
> > > 
> > > I think this is much clearer than peppering the code with very specific
> state
> > > transitions.  I'm not sure how PAUSED is supposed to fit into this
picture. 
> > If
> > > it's a state set externally, I'd suggest a bool for it, and to check for
it
> > > here.
> > 
> > PAUSED is set by the ResourceScheduler. 
> > 
> > Did you want me to rename observable_clients_loaded() to
> > HasLoadingObservableClients() ?
> 
> No, observable_clients_loaded is fine, I wrote that code without
> looking/thinking about how things were actually set up, was just intended as
an
> example.
> 
> > There are a few more variables that need to go into the state decisions, but
> > that should be fine. I changed OnObservabilityChanged to set the throttle
> state
> > based on state rather than the transition. I could keep that the same, or
> change
> > it to return the throttle state instead.
> 
> That sounds reasonable as-is.
> 
> >
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > File content/browser/loader/resource_scheduler.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/357583003/diff/100001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:410: throttle_state_ =
> THROTTLED;
> > > We're planning on throttling background clients that have finished
loading? 
> > So
> > > they can never send XHRs, or load other stuff in the background, until
> they're
> > > brought to the front, or activated?
> > 
> > Not exactly. If a Client is THROTTLED (which means an active Client is still
> > loading, or we haven't turned coalescing on yet), they are only allowed to
> load
> > one non-SPDY request at a time (and as many SPDY-requests as they have). I
> > didn't want to starve background Clients, which is why I'm not just making
> them
> > PAUSED, but I want to limit the amount of bandwidth that can be stolen from
> > active Clients. 
> 
> Ahh, that does much to allay my concerns, though we should have histograms for
> observable/unobservable tabs and a field trial.

mmenke

On 2014/07/01 19:01:40, aiolos wrote: > A question I had earlier that I think got ...

6 years, 5 months ago (2014-07-01 19:19:06 UTC) #15

On 2014/07/01 19:01:40, aiolos wrote:
> A question I had earlier that I think got lost in the mix is: are there
specific
> things you want tested for this? Or just want there to be some general test
> coverage?

Sorry about that, I thought you were asking more generally.  Here's a list of
suggested tests (Using visible/hidden instead of observable, since it sounds
less awkward).

Hidden clients are throttled when finished loading, and unthrottled when they
start loading again.

Hidden clients throttle requests when there's a visible loading client.  When
that client finishes loading, the clients are unthrottled.

A test with two visible clients, and one with two hidden (Can be the same test),
both visible clients finish loading and the two hidden should start.

A test that hidden to hidden state transitions don't unthrottle.

Hidden to visible transitions on one client unthrottle hidden clients (And don't
throttle the formerly-visible client).

Hidden are not throttled when no loading visible tabs.

Visible while loading to hidden throttles clients, when there's a visible
client.

When two hiddens are loading, and one becomes visible, the other tab is
throttled (And the visible one is not).

We'll want versions for most of these where one (Or the other, or both) of the
hidden tabs are done loading / not done loading, once we have coalescing set up.

I've undoubtedly missed some cases.  There are a lot of paths through the code,
so we'll need a lot of tests.

> On 2014/07/01 18:34:32, mmenke wrote:
> > On 2014/07/01 18:16:42, aiolos wrote:
> > > On 2014/07/01 17:25:39, mmenke wrote:
> > > > What are we getting from the 5-second heartbeat that we wouldn't get
from
> > just
> > > > throttling unobservable clients as needed?
> > > 
> > > The THROTTLED and PAUSED changes are for performance, but the COALESCED
> > changes
> > > are for battery savings. It allows us to avoid waking up the network stack
> > every
> > > time a background Clients makes a request. If you have active enough
> > background
> > > tabs currently, you could potentially keep the network stack up
> indefinitely.
> > > Note that this is true while there are no active Clients as well, such as
> > Chrome
> > > being minimized.
> > 
> > I thought that entering and leaving high power network radio state on phones
> > took on the order of several seconds, so if you have to go back into high
> power
> > state for a bunch of requests, odds are that 5 seconds later only gave you a
> > second or two of a full power radio before you let loose the next ones.  I'm
a
> > bit skeptical of the 5-seconds doing any good here, but I admit to being
> pretty
> > ignorant when it comes to cell radios.
> 
> One of the future steps is to become aware of the current network state, so
that
> we can only coalesce when the network isn't already active from work outside
of
> the ResourceScheduler. The 5 seconds was suggested as a first try by tonyg. I
> also don't know the network bring up times for all of the devices this would
be
> on.I assumed this value may also get tuned to be longer later, but that we'd
> start with baby steps. I'm happy to make it longer if you think that would be
a
> good idea.

I defer to tonyg here - he's undoubtedly more knowledgeable about this than I
am.  Ideally, we should do some investigation into whether this helps battery
life or not at some point, and play with different values.

aiolos (Not reviewing)

On 2014/07/01 19:01:40, aiolos wrote: > A question I had earlier that I think got ...

6 years, 5 months ago (2014-07-01 19:22:21 UTC) #16

mmenke

On 2014/07/01 19:22:21, aiolos wrote: > On 2014/07/01 19:01:40, aiolos wrote: > > A question ...

6 years, 5 months ago (2014-07-01 19:25:45 UTC) #17

aiolos (Not reviewing)

On 2014/07/01 19:25:45, mmenke wrote: > On 2014/07/01 19:22:21, aiolos wrote: > > On 2014/07/01 ...

6 years, 5 months ago (2014-07-01 19:34:49 UTC) #18

mmenke

On 2014/07/01 19:34:49, aiolos wrote: > On 2014/07/01 19:25:45, mmenke wrote: > > On 2014/07/01 ...

6 years, 5 months ago (2014-07-01 19:36:52 UTC) #19

aiolos (Not reviewing)

For some reason, this doesn't appear to have uploaded last week, or this afternoon. I'm ...

6 years, 5 months ago (2014-07-07 20:27:29 UTC) #20

aiolos (Not reviewing)

On 2014/07/07 20:27:29, aiolos wrote: > For some reason, this doesn't appear to have uploaded ...

6 years, 5 months ago (2014-07-07 20:29:50 UTC) #21

mmenke

On 2014/07/07 20:29:50, aiolos wrote: > On 2014/07/07 20:27:29, aiolos wrote: > > For some ...

6 years, 5 months ago (2014-07-07 20:36:42 UTC) #22

mmenke

I still need to carefully go over the tests, but at a glance, they look ...

6 years, 5 months ago (2014-07-08 19:37:41 UTC) #23

aiolos (Not reviewing)

> https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/resource_scheduler.cc#newcode807 > content/browser/loader/resource_scheduler.cc:807: if (was_active == > client->is_active() || client->is_loaded()) { > These extra ...

6 years, 5 months ago (2014-07-08 20:07:45 UTC) #24

>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:807: if (was_active ==
> client->is_active() ||  client->is_loaded()) {
> These extra checks on every case seem somewhat regression prone.
> 
> Here's my suggestion:  Just make this as braindead as possible, with as simple
> code as we can manage.  None of these optimizations for when there's no change
> are performance sensitive.  Even if there are 200 tabs, with each one having
its
> state chance 5 times a second, there's no real performance impact here from
> doing something simpler.
> 
>   ...
>   client->set_visibility(visibility);  // This is a simple setter, does not
> update throttle state.
>   this->UpdateClientThrottleStates();
> }
> 
> ResourceScheduler::UpdateClientThrottleStates() {
>   loading_active_clients_ = 0;
>   for (ClientMap::iterator client_it = client_map_.begin();
>        client_it != client_map_.end(); ++client_it) {
>     if (client->is_active() ||  client->is_loaded())
>       loading_active_clients_++;
>   }
> 
>   for (ClientMap::iterator client_it = client_map_.begin();
>        client_it != client_map_.end(); ++client_it) {
>     client->UpdateClientThrottleState();
>   }
> }
> 
> UpdateClientThrottleState doesn't do much when the throttle state doesn't
> change, and just walking through the clients isn't terribly expensive.
> 
> It's also much easier to verify this code is correct.  WDYT?

That code doesn't do what this function currently does. The only clients that
should be incrementing the loading_active_clients_ are the ones that are user
observable. That was why it was originally called loading_observable_clients_.
The point of this code was to keep track of the still loading clients that are
load-time sensitive (visible and audible), and turn on/off throttling of the
clients that aren't. If a background tab finishes loading right before a user
switches tabs from a loaded page to a new unloaded page, I assume we would want
to set the background tabs to throttle asap without having to iterate 6 times
through the number of clients (twice for the background tab loading, and twice
for the previously foreground tab going to the background, and twice for the new
tab becoming foreground). This will get even worse when we start increasing the
number of clients when we include non-tab clients for things like service
workers and sync. Thoughts?

>

mmenke

On 2014/07/08 20:07:45, aiolos wrote: > > > https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/resource_scheduler.cc#newcode807 > > content/browser/loader/resource_scheduler.cc:807: if (was_active == ...

6 years, 5 months ago (2014-07-08 20:32:12 UTC) #25

On 2014/07/08 20:07:45, aiolos wrote:
> >
>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:807: if (was_active ==
> > client->is_active() ||  client->is_loaded()) {
> > These extra checks on every case seem somewhat regression prone.
> > 
> > Here's my suggestion:  Just make this as braindead as possible, with as
simple
> > code as we can manage.  None of these optimizations for when there's no
change
> > are performance sensitive.  Even if there are 200 tabs, with each one having
> its
> > state chance 5 times a second, there's no real performance impact here from
> > doing something simpler.
> > 
> >   ...
> >   client->set_visibility(visibility);  // This is a simple setter, does not
> > update throttle state.
> >   this->UpdateClientThrottleStates();
> > }
> > 
> > ResourceScheduler::UpdateClientThrottleStates() {
> >   loading_active_clients_ = 0;
> >   for (ClientMap::iterator client_it = client_map_.begin();
> >        client_it != client_map_.end(); ++client_it) {
> >     if (client->is_active() ||  client->is_loaded())
> >       loading_active_clients_++;
> >   }
> > 
> >   for (ClientMap::iterator client_it = client_map_.begin();
> >        client_it != client_map_.end(); ++client_it) {
> >     client->UpdateClientThrottleState();
> >   }
> > }
> > 
> > UpdateClientThrottleState doesn't do much when the throttle state doesn't
> > change, and just walking through the clients isn't terribly expensive.
> > 
> > It's also much easier to verify this code is correct.  WDYT?
> 
> That code doesn't do what this function currently does. The only clients that
> should be incrementing the loading_active_clients_ are the ones that are user
> observable. That was why it was originally called loading_observable_clients_.
> The point of this code was to keep track of the still loading clients that are
> load-time sensitive (visible and audible), and turn on/off throttling of the
> clients that aren't. If a background tab finishes loading right before a user
> switches tabs from a loaded page to a new unloaded page, I assume we would
want
> to set the background tabs to throttle asap without having to iterate 6 times
> through the number of clients (twice for the background tab loading, and twice
> for the previously foreground tab going to the background, and twice for the
new
> tab becoming foreground). This will get even worse when we start increasing
the
> number of clients when we include non-tab clients for things like service
> workers and sync. Thoughts?

Sorry, that should be is_active && !is_loaded, just copied your code without
correctly modifying the logic.

I don't think that performance of walking through clients is a major concern,
even if we do it as inefficiently as I suggested.

However, one simple alternative would be to maintain a set of pointers to
loading clients, and just update that for the modified client, and then check if
it goes from 0 to 1 or 1 to 0 as needed.

Or could create separate states for unthrottled due to being active and
unloaded, and being unthrottled for any other reason (Or a separate bool for
being in the first state), and check/update that for only the one specific
client in ResourceScheduler::UpdateClientThrottleState.

aiolos (Not reviewing)

On 2014/07/08 20:32:12, mmenke wrote: > On 2014/07/08 20:07:45, aiolos wrote: > > > > ...

6 years, 5 months ago (2014-07-08 21:27:21 UTC) #26

On 2014/07/08 20:32:12, mmenke wrote:
> On 2014/07/08 20:07:45, aiolos wrote:
> > >
> >
>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:807: if (was_active ==
> > > client->is_active() ||  client->is_loaded()) {
> > > These extra checks on every case seem somewhat regression prone.
> > > 
> > > Here's my suggestion:  Just make this as braindead as possible, with as
> simple
> > > code as we can manage.  None of these optimizations for when there's no
> change
> > > are performance sensitive.  Even if there are 200 tabs, with each one
having
> > its
> > > state chance 5 times a second, there's no real performance impact here
from
> > > doing something simpler.
> > > 
> > >   ...
> > >   client->set_visibility(visibility);  // This is a simple setter, does
not
> > > update throttle state.
> > >   this->UpdateClientThrottleStates();
> > > }
> > > 
> > > ResourceScheduler::UpdateClientThrottleStates() {
> > >   loading_active_clients_ = 0;
> > >   for (ClientMap::iterator client_it = client_map_.begin();
> > >        client_it != client_map_.end(); ++client_it) {
> > >     if (client->is_active() ||  client->is_loaded())
> > >       loading_active_clients_++;
> > >   }
> > > 
> > >   for (ClientMap::iterator client_it = client_map_.begin();
> > >        client_it != client_map_.end(); ++client_it) {
> > >     client->UpdateClientThrottleState();
> > >   }
> > > }
> > > 
> > > UpdateClientThrottleState doesn't do much when the throttle state doesn't
> > > change, and just walking through the clients isn't terribly expensive.
> > > 
> > > It's also much easier to verify this code is correct.  WDYT?
> > 
> > That code doesn't do what this function currently does. The only clients
that
> > should be incrementing the loading_active_clients_ are the ones that are
user
> > observable. That was why it was originally called
loading_observable_clients_.
> > The point of this code was to keep track of the still loading clients that
are
> > load-time sensitive (visible and audible), and turn on/off throttling of the
> > clients that aren't. If a background tab finishes loading right before a
user
> > switches tabs from a loaded page to a new unloaded page, I assume we would
> want
> > to set the background tabs to throttle asap without having to iterate 6
times
> > through the number of clients (twice for the background tab loading, and
twice
> > for the previously foreground tab going to the background, and twice for the
> new
> > tab becoming foreground). This will get even worse when we start increasing
> the
> > number of clients when we include non-tab clients for things like service
> > workers and sync. Thoughts?
> 
> Sorry, that should be is_active && !is_loaded, just copied your code without
> correctly modifying the logic.
> 
> I don't think that performance of walking through clients is a major concern,
> even if we do it as inefficiently as I suggested.
> 
> However, one simple alternative would be to maintain a set of pointers to
> loading clients, and just update that for the modified client, and then check
if
> it goes from 0 to 1 or 1 to 0 as needed.
> 
> Or could create separate states for unthrottled due to being active and
> unloaded, and being unthrottled for any other reason (Or a separate bool for
> being in the first state), and check/update that for only the one specific
> client in ResourceScheduler::UpdateClientThrottleState.

I think I don't have a good enough understanding of why these would be
regression prone. Could you help me fix that? I'd really like to avoid adding
multiple linear walks to calls if I don't have to. That's especially true
without testing to back up it not being a perf hit. And I'd also like to avoid
added complexity that isn't needed. But I don't think I have a good enough grasp
of what the specific issue is to make better suggestions/decisions for fixing
it.

Is it that I'm checking multiple states in the Client before deciding to update
the states of the other clients from the Visibility or Audibility changed
functions? Or that I'm making the same checks in those two functions? Or that
there is a different check being done when we're changing loading state? Or that
the checks aren't explicit enough of to be clear that it's for changing the
number of observable loading clients? Or something else that I'm missing?

mmenke

On 2014/07/08 21:27:21, aiolos wrote: > On 2014/07/08 20:32:12, mmenke wrote: > > On 2014/07/08 ...

6 years, 5 months ago (2014-07-08 21:36:11 UTC) #27

On 2014/07/08 21:27:21, aiolos wrote:
> On 2014/07/08 20:32:12, mmenke wrote:
> > On 2014/07/08 20:07:45, aiolos wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> > > > content/browser/loader/resource_scheduler.cc:807: if (was_active ==
> > > > client->is_active() ||  client->is_loaded()) {
> > > > These extra checks on every case seem somewhat regression prone.
> > > > 
> > > > Here's my suggestion:  Just make this as braindead as possible, with as
> > simple
> > > > code as we can manage.  None of these optimizations for when there's no
> > change
> > > > are performance sensitive.  Even if there are 200 tabs, with each one
> having
> > > its
> > > > state chance 5 times a second, there's no real performance impact here
> from
> > > > doing something simpler.
> > > > 
> > > >   ...
> > > >   client->set_visibility(visibility);  // This is a simple setter, does
> not
> > > > update throttle state.
> > > >   this->UpdateClientThrottleStates();
> > > > }
> > > > 
> > > > ResourceScheduler::UpdateClientThrottleStates() {
> > > >   loading_active_clients_ = 0;
> > > >   for (ClientMap::iterator client_it = client_map_.begin();
> > > >        client_it != client_map_.end(); ++client_it) {
> > > >     if (client->is_active() ||  client->is_loaded())
> > > >       loading_active_clients_++;
> > > >   }
> > > > 
> > > >   for (ClientMap::iterator client_it = client_map_.begin();
> > > >        client_it != client_map_.end(); ++client_it) {
> > > >     client->UpdateClientThrottleState();
> > > >   }
> > > > }
> > > > 
> > > > UpdateClientThrottleState doesn't do much when the throttle state
doesn't
> > > > change, and just walking through the clients isn't terribly expensive.
> > > > 
> > > > It's also much easier to verify this code is correct.  WDYT?
> > > 
> > > That code doesn't do what this function currently does. The only clients
> that
> > > should be incrementing the loading_active_clients_ are the ones that are
> user
> > > observable. That was why it was originally called
> loading_observable_clients_.
> > > The point of this code was to keep track of the still loading clients that
> are
> > > load-time sensitive (visible and audible), and turn on/off throttling of
the
> > > clients that aren't. If a background tab finishes loading right before a
> user
> > > switches tabs from a loaded page to a new unloaded page, I assume we would
> > want
> > > to set the background tabs to throttle asap without having to iterate 6
> times
> > > through the number of clients (twice for the background tab loading, and
> twice
> > > for the previously foreground tab going to the background, and twice for
the
> > new
> > > tab becoming foreground). This will get even worse when we start
increasing
> > the
> > > number of clients when we include non-tab clients for things like service
> > > workers and sync. Thoughts?
> > 
> > Sorry, that should be is_active && !is_loaded, just copied your code without
> > correctly modifying the logic.
> > 
> > I don't think that performance of walking through clients is a major
concern,
> > even if we do it as inefficiently as I suggested.
> > 
> > However, one simple alternative would be to maintain a set of pointers to
> > loading clients, and just update that for the modified client, and then
check
> if
> > it goes from 0 to 1 or 1 to 0 as needed.
> > 
> > Or could create separate states for unthrottled due to being active and
> > unloaded, and being unthrottled for any other reason (Or a separate bool for
> > being in the first state), and check/update that for only the one specific
> > client in ResourceScheduler::UpdateClientThrottleState.
> 
> I think I don't have a good enough understanding of why these would be
> regression prone. Could you help me fix that? I'd really like to avoid adding
> multiple linear walks to calls if I don't have to. That's especially true
> without testing to back up it not being a perf hit. And I'd also like to avoid
> added complexity that isn't needed. But I don't think I have a good enough
grasp
> of what the specific issue is to make better suggestions/decisions for fixing
> it.
> 
> Is it that I'm checking multiple states in the Client before deciding to
update
> the states of the other clients from the Visibility or Audibility changed
> functions? Or that I'm making the same checks in those two functions? Or that
> there is a different check being done when we're changing loading state? Or
that
> the checks aren't explicit enough of to be clear that it's for changing the
> number of observable loading clients? Or something else that I'm missing?

It's that you have 3 copies of basically identical logic, in terms of when and
how to update the count of loading active clients.  You also have 3 copies of
calls into the clients that are basically the same as well, in each case.  I'm
less concerned about those 3 (Though I don't think they all need to have logic
to do nothing if there's no change - you already have that logic in
UpdateThrottle state, anyways.  Binary size often matters more than saving a few
dozen instructions, anyways).

mmenke

On 2014/07/08 21:36:11, mmenke wrote: > On 2014/07/08 21:27:21, aiolos wrote: > > On 2014/07/08 ...

6 years, 5 months ago (2014-07-08 21:45:02 UTC) #28

On 2014/07/08 21:36:11, mmenke wrote:
> On 2014/07/08 21:27:21, aiolos wrote:
> > On 2014/07/08 20:32:12, mmenke wrote:
> > > On 2014/07/08 20:07:45, aiolos wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> > > > > content/browser/loader/resource_scheduler.cc:807: if (was_active ==
> > > > > client->is_active() ||  client->is_loaded()) {
> > > > > These extra checks on every case seem somewhat regression prone.
> > > > > 
> > > > > Here's my suggestion:  Just make this as braindead as possible, with
as
> > > simple
> > > > > code as we can manage.  None of these optimizations for when there's
no
> > > change
> > > > > are performance sensitive.  Even if there are 200 tabs, with each one
> > having
> > > > its
> > > > > state chance 5 times a second, there's no real performance impact here
> > from
> > > > > doing something simpler.
> > > > > 
> > > > >   ...
> > > > >   client->set_visibility(visibility);  // This is a simple setter,
does
> > not
> > > > > update throttle state.
> > > > >   this->UpdateClientThrottleStates();
> > > > > }
> > > > > 
> > > > > ResourceScheduler::UpdateClientThrottleStates() {
> > > > >   loading_active_clients_ = 0;
> > > > >   for (ClientMap::iterator client_it = client_map_.begin();
> > > > >        client_it != client_map_.end(); ++client_it) {
> > > > >     if (client->is_active() ||  client->is_loaded())
> > > > >       loading_active_clients_++;
> > > > >   }
> > > > > 
> > > > >   for (ClientMap::iterator client_it = client_map_.begin();
> > > > >        client_it != client_map_.end(); ++client_it) {
> > > > >     client->UpdateClientThrottleState();
> > > > >   }
> > > > > }
> > > > > 
> > > > > UpdateClientThrottleState doesn't do much when the throttle state
> doesn't
> > > > > change, and just walking through the clients isn't terribly expensive.
> > > > > 
> > > > > It's also much easier to verify this code is correct.  WDYT?
> > > > 
> > > > That code doesn't do what this function currently does. The only clients
> > that
> > > > should be incrementing the loading_active_clients_ are the ones that are
> > user
> > > > observable. That was why it was originally called
> > loading_observable_clients_.
> > > > The point of this code was to keep track of the still loading clients
that
> > are
> > > > load-time sensitive (visible and audible), and turn on/off throttling of
> the
> > > > clients that aren't. If a background tab finishes loading right before a
> > user
> > > > switches tabs from a loaded page to a new unloaded page, I assume we
would
> > > want
> > > > to set the background tabs to throttle asap without having to iterate 6
> > times
> > > > through the number of clients (twice for the background tab loading, and
> > twice
> > > > for the previously foreground tab going to the background, and twice for
> the
> > > new
> > > > tab becoming foreground). This will get even worse when we start
> increasing
> > > the
> > > > number of clients when we include non-tab clients for things like
service
> > > > workers and sync. Thoughts?
> > > 
> > > Sorry, that should be is_active && !is_loaded, just copied your code
without
> > > correctly modifying the logic.
> > > 
> > > I don't think that performance of walking through clients is a major
> concern,
> > > even if we do it as inefficiently as I suggested.
> > > 
> > > However, one simple alternative would be to maintain a set of pointers to
> > > loading clients, and just update that for the modified client, and then
> check
> > if
> > > it goes from 0 to 1 or 1 to 0 as needed.
> > > 
> > > Or could create separate states for unthrottled due to being active and
> > > unloaded, and being unthrottled for any other reason (Or a separate bool
for
> > > being in the first state), and check/update that for only the one specific
> > > client in ResourceScheduler::UpdateClientThrottleState.
> > 
> > I think I don't have a good enough understanding of why these would be
> > regression prone. Could you help me fix that? I'd really like to avoid
adding
> > multiple linear walks to calls if I don't have to. That's especially true
> > without testing to back up it not being a perf hit. And I'd also like to
avoid
> > added complexity that isn't needed. But I don't think I have a good enough
> grasp
> > of what the specific issue is to make better suggestions/decisions for
fixing
> > it.
> > 
> > Is it that I'm checking multiple states in the Client before deciding to
> update
> > the states of the other clients from the Visibility or Audibility changed
> > functions? Or that I'm making the same checks in those two functions? Or
that
> > there is a different check being done when we're changing loading state? Or
> that
> > the checks aren't explicit enough of to be clear that it's for changing the
> > number of observable loading clients? Or something else that I'm missing?
> 
> It's that you have 3 copies of basically identical logic, in terms of when and
> how to update the count of loading active clients.  You also have 3 copies of
> calls into the clients that are basically the same as well, in each case.  I'm
> less concerned about those 3 (Though I don't think they all need to have logic
> to do nothing if there's no change - you already have that logic in
> UpdateThrottle state, anyways.  Binary size often matters more than saving a
few
> dozen instructions, anyways).

Anyhow, I did suggest a way to avoid walking through all clients on all changes
any more than your code currently does.

mmenke

On 2014/07/08 21:45:02, mmenke wrote: > Anyhow, I did suggest a way to avoid walking ...

6 years, 5 months ago (2014-07-08 22:24:27 UTC) #29

aiolos (Not reviewing)

On 2014/07/08 22:24:27, mmenke wrote: > On 2014/07/08 21:45:02, mmenke wrote: > > Anyhow, I ...

6 years, 5 months ago (2014-07-08 23:33:39 UTC) #30

On 2014/07/08 22:24:27, mmenke wrote:
> On 2014/07/08 21:45:02, mmenke wrote:
> > Anyhow, I did suggest a way to avoid walking through all clients on all
> changes
> > any more than your code currently does.
> 
> I don't mean to sound like I'm dead set on something - happy for other ideas,
> just think there's too much redundant code.

I'm currently working on factoring some of this out/looking into using your
suggestions.

>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> content/browser/loader/resource_scheduler_unittest.cc:133:
> scheduler_.SetThrottleOptions(true, false);
> Unless we're going to test them in this CL, I don't think we should land any
of
> the COALESCED or PAUSED logic in this CL (Include the enum values themselves).
> 
> Think there are only maybe 20-30 lines this affects, so shouldn't be a big
deal
> to just remove them from this CL.

Taking out PAUSED and adding it again later would be pretty easy since it's a
fairly orthogonal change. But I went back through the code, and I'd really like
to land the THROTTLED and COALESCED changes together. I actually had a talk
about submitting just the throttling changes with tonyg before uploading the
first CL, and the changes made more sense going in together. I'd rather add
another 700 lines of test code that will be redundant to the throttling-only
tests to be honest, although I thought that would be over-kill to review.

I can add the tests to make sure that COALESCED Clients operate as desired
fairly quickly (similar in function to the added
ThrottledSpdyProxySchedulesImmediately and OneRequestPerThrottledClient tests
for THROTTLED clients). All of the state changes should be tested with the same
functions that were already added to test that Clients were correctly
THROTTLED/UNTHROTTLED, and I had planned to simply update those tests to make
sure that the correct Clients were COALESCED instead of throttled when we were
switching coalescing on. I assumed this would be fine since those same unit
tests would fail if you were attempting to turn coalescing on without updating
the tests, thus stopping the changes from going through.

If you'd rather have redundant tests for throttled-only and
throttled-and-coalesced, I can go do a bunch of copy-pasting with some line
edits and calls to update the scheduler to coalesce at the same time. Would it
be sufficient to add COALESCED versions of some of the tests (ie, do you want
all combinations of loaded/visible/audible for just THROTTLED and
THROTTLED/COALESCED? And/or commented out lines of how the functions will be
updated for coalesced? Or is there something else you'd like me to do?

mmenke

On 2014/07/08 23:33:39, aiolos wrote: > On 2014/07/08 22:24:27, mmenke wrote: > > On 2014/07/08 ...

6 years, 5 months ago (2014-07-08 23:56:28 UTC) #31

On 2014/07/08 23:33:39, aiolos wrote:
> On 2014/07/08 22:24:27, mmenke wrote:
> > On 2014/07/08 21:45:02, mmenke wrote:
> > > Anyhow, I did suggest a way to avoid walking through all clients on all
> > changes
> > > any more than your code currently does.
> > 
> > I don't mean to sound like I'm dead set on something - happy for other
ideas,
> > just think there's too much redundant code.
> 
> I'm currently working on factoring some of this out/looking into using your
> suggestions.
> 
> >
>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> > content/browser/loader/resource_scheduler_unittest.cc:133:
> > scheduler_.SetThrottleOptions(true, false);
> > Unless we're going to test them in this CL, I don't think we should land any
> of
> > the COALESCED or PAUSED logic in this CL (Include the enum values
themselves).
> > 
> > Think there are only maybe 20-30 lines this affects, so shouldn't be a big
> deal
> > to just remove them from this CL.
> 
> Taking out PAUSED and adding it again later would be pretty easy since it's a
> fairly orthogonal change. But I went back through the code, and I'd really
like
> to land the THROTTLED and COALESCED changes together. I actually had a talk
> about submitting just the throttling changes with tonyg before uploading the
> first CL, and the changes made more sense going in together. I'd rather add
> another 700 lines of test code that will be redundant to the throttling-only
> tests to be honest, although I thought that would be over-kill to review.
> 
> I can add the tests to make sure that COALESCED Clients operate as desired
> fairly quickly (similar in function to the added
> ThrottledSpdyProxySchedulesImmediately and OneRequestPerThrottledClient tests
> for THROTTLED clients). All of the state changes should be tested with the
same
> functions that were already added to test that Clients were correctly
> THROTTLED/UNTHROTTLED, and I had planned to simply update those tests to make
> sure that the correct Clients were COALESCED instead of throttled when we were
> switching coalescing on. I assumed this would be fine since those same unit
> tests would fail if you were attempting to turn coalescing on without updating
> the tests, thus stopping the changes from going through.
> 
> If you'd rather have redundant tests for throttled-only and
> throttled-and-coalesced, I can go do a bunch of copy-pasting with some line
> edits and calls to update the scheduler to coalesce at the same time. Would it
> be sufficient to add COALESCED versions of some of the tests (ie, do you want
> all combinations of loaded/visible/audible for just THROTTLED and
> THROTTLED/COALESCED? And/or commented out lines of how the functions will be
> updated for coalesced? Or is there something else you'd like me to do?

I'm fine with landing them together, if that's what you prefer, as long as we
have tests.  I don't think we need tests for all combinations of both
throttled-only and throttled-and-coalesced (I may have gone a bit overboard in
that part of my earlier suggestions).  Think we mostly need to focused on where
behavior's markedly different, or the code follows different paths.

aiolos (Not reviewing)

On 2014/07/08 23:56:28, mmenke wrote: > On 2014/07/08 23:33:39, aiolos wrote: > > On 2014/07/08 ...

6 years, 5 months ago (2014-07-09 00:08:22 UTC) #32

On 2014/07/08 23:56:28, mmenke wrote:
> On 2014/07/08 23:33:39, aiolos wrote:
> > On 2014/07/08 22:24:27, mmenke wrote:
> > > On 2014/07/08 21:45:02, mmenke wrote:
> > > > Anyhow, I did suggest a way to avoid walking through all clients on all
> > > changes
> > > > any more than your code currently does.
> > > 
> > > I don't mean to sound like I'm dead set on something - happy for other
> ideas,
> > > just think there's too much redundant code.
> > 
> > I'm currently working on factoring some of this out/looking into using your
> > suggestions.
> > 
> > >
> >
>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler_unittest.cc:133:
> > > scheduler_.SetThrottleOptions(true, false);
> > > Unless we're going to test them in this CL, I don't think we should land
any
> > of
> > > the COALESCED or PAUSED logic in this CL (Include the enum values
> themselves).
> > > 
> > > Think there are only maybe 20-30 lines this affects, so shouldn't be a big
> > deal
> > > to just remove them from this CL.
> > 
> > Taking out PAUSED and adding it again later would be pretty easy since it's
a
> > fairly orthogonal change. But I went back through the code, and I'd really
> like
> > to land the THROTTLED and COALESCED changes together. I actually had a talk
> > about submitting just the throttling changes with tonyg before uploading the
> > first CL, and the changes made more sense going in together. I'd rather add
> > another 700 lines of test code that will be redundant to the throttling-only
> > tests to be honest, although I thought that would be over-kill to review.
> > 
> > I can add the tests to make sure that COALESCED Clients operate as desired
> > fairly quickly (similar in function to the added
> > ThrottledSpdyProxySchedulesImmediately and OneRequestPerThrottledClient
tests
> > for THROTTLED clients). All of the state changes should be tested with the
> same
> > functions that were already added to test that Clients were correctly
> > THROTTLED/UNTHROTTLED, and I had planned to simply update those tests to
make
> > sure that the correct Clients were COALESCED instead of throttled when we
were
> > switching coalescing on. I assumed this would be fine since those same unit
> > tests would fail if you were attempting to turn coalescing on without
updating
> > the tests, thus stopping the changes from going through.
> > 
> > If you'd rather have redundant tests for throttled-only and
> > throttled-and-coalesced, I can go do a bunch of copy-pasting with some line
> > edits and calls to update the scheduler to coalesce at the same time. Would
it
> > be sufficient to add COALESCED versions of some of the tests (ie, do you
want
> > all combinations of loaded/visible/audible for just THROTTLED and
> > THROTTLED/COALESCED? And/or commented out lines of how the functions will be
> > updated for coalesced? Or is there something else you'd like me to do?
> 
> I'm fine with landing them together, if that's what you prefer, as long as we
> have tests.  I don't think we need tests for all combinations of both
> throttled-only and throttled-and-coalesced (I may have gone a bit overboard in
> that part of my earlier suggestions).  Think we mostly need to focused on
where
> behavior's markedly different, or the code follows different paths.

OK, SGTM. I'll add tests for those cases. 

Also, would there be any issue with my using function pointers in the
visibility/audibility changes? I didn't see anything about them in the style
guidelines, and much of the redundancy could be pretty easily avoided if I used
them. I'm still transitioning my understanding of what is "too complex" from a
very different coding environment, and could use a bit of feedback on this.

mmenke

On 2014/07/09 00:08:22, aiolos wrote: > On 2014/07/08 23:56:28, mmenke wrote: > > On 2014/07/08 ...

6 years, 5 months ago (2014-07-09 00:10:45 UTC) #33

On 2014/07/09 00:08:22, aiolos wrote:
> On 2014/07/08 23:56:28, mmenke wrote:
> > On 2014/07/08 23:33:39, aiolos wrote:
> > > On 2014/07/08 22:24:27, mmenke wrote:
> > > > On 2014/07/08 21:45:02, mmenke wrote:
> > > > > Anyhow, I did suggest a way to avoid walking through all clients on
all
> > > > changes
> > > > > any more than your code currently does.
> > > > 
> > > > I don't mean to sound like I'm dead set on something - happy for other
> > ideas,
> > > > just think there's too much redundant code.
> > > 
> > > I'm currently working on factoring some of this out/looking into using
your
> > > suggestions.
> > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> > > > content/browser/loader/resource_scheduler_unittest.cc:133:
> > > > scheduler_.SetThrottleOptions(true, false);
> > > > Unless we're going to test them in this CL, I don't think we should land
> any
> > > of
> > > > the COALESCED or PAUSED logic in this CL (Include the enum values
> > themselves).
> > > > 
> > > > Think there are only maybe 20-30 lines this affects, so shouldn't be a
big
> > > deal
> > > > to just remove them from this CL.
> > > 
> > > Taking out PAUSED and adding it again later would be pretty easy since
it's
> a
> > > fairly orthogonal change. But I went back through the code, and I'd really
> > like
> > > to land the THROTTLED and COALESCED changes together. I actually had a
talk
> > > about submitting just the throttling changes with tonyg before uploading
the
> > > first CL, and the changes made more sense going in together. I'd rather
add
> > > another 700 lines of test code that will be redundant to the
throttling-only
> > > tests to be honest, although I thought that would be over-kill to review.
> > > 
> > > I can add the tests to make sure that COALESCED Clients operate as desired
> > > fairly quickly (similar in function to the added
> > > ThrottledSpdyProxySchedulesImmediately and OneRequestPerThrottledClient
> tests
> > > for THROTTLED clients). All of the state changes should be tested with the
> > same
> > > functions that were already added to test that Clients were correctly
> > > THROTTLED/UNTHROTTLED, and I had planned to simply update those tests to
> make
> > > sure that the correct Clients were COALESCED instead of throttled when we
> were
> > > switching coalescing on. I assumed this would be fine since those same
unit
> > > tests would fail if you were attempting to turn coalescing on without
> updating
> > > the tests, thus stopping the changes from going through.
> > > 
> > > If you'd rather have redundant tests for throttled-only and
> > > throttled-and-coalesced, I can go do a bunch of copy-pasting with some
line
> > > edits and calls to update the scheduler to coalesce at the same time.
Would
> it
> > > be sufficient to add COALESCED versions of some of the tests (ie, do you
> want
> > > all combinations of loaded/visible/audible for just THROTTLED and
> > > THROTTLED/COALESCED? And/or commented out lines of how the functions will
be
> > > updated for coalesced? Or is there something else you'd like me to do?
> > 
> > I'm fine with landing them together, if that's what you prefer, as long as
we
> > have tests.  I don't think we need tests for all combinations of both
> > throttled-only and throttled-and-coalesced (I may have gone a bit overboard
in
> > that part of my earlier suggestions).  Think we mostly need to focused on
> where
> > behavior's markedly different, or the code follows different paths.
> 
> OK, SGTM. I'll add tests for those cases. 
> 
> Also, would there be any issue with my using function pointers in the
> visibility/audibility changes? I didn't see anything about them in the style
> guidelines, and much of the redundancy could be pretty easily avoided if I
used
> them. I'm still transitioning my understanding of what is "too complex" from a
> very different coding environment, and could use a bit of feedback on this.

I'm fine with function pointers.  This isn't so much "too complex" in an
absolute manner, I'd say "more complex/duplicative than it needs to be".

aiolos (Not reviewing)

On 2014/07/09 00:10:45, mmenke wrote: > On 2014/07/09 00:08:22, aiolos wrote: > > On 2014/07/08 ...

6 years, 5 months ago (2014-07-09 18:08:23 UTC) #34

On 2014/07/09 00:10:45, mmenke wrote:
> On 2014/07/09 00:08:22, aiolos wrote:
> > On 2014/07/08 23:56:28, mmenke wrote:
> > > On 2014/07/08 23:33:39, aiolos wrote:
> > > > On 2014/07/08 22:24:27, mmenke wrote:
> > > > > On 2014/07/08 21:45:02, mmenke wrote:
> > > > > > Anyhow, I did suggest a way to avoid walking through all clients on
> all
> > > > > changes
> > > > > > any more than your code currently does.
> > > > > 
> > > > > I don't mean to sound like I'm dead set on something - happy for other
> > > ideas,
> > > > > just think there's too much redundant code.
> > > > 
> > > > I'm currently working on factoring some of this out/looking into using
> your
> > > > suggestions.
> > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> > > > > content/browser/loader/resource_scheduler_unittest.cc:133:
> > > > > scheduler_.SetThrottleOptions(true, false);
> > > > > Unless we're going to test them in this CL, I don't think we should
land
> > any
> > > > of
> > > > > the COALESCED or PAUSED logic in this CL (Include the enum values
> > > themselves).
> > > > > 
> > > > > Think there are only maybe 20-30 lines this affects, so shouldn't be a
> big
> > > > deal
> > > > > to just remove them from this CL.
> > > > 
> > > > Taking out PAUSED and adding it again later would be pretty easy since
> it's
> > a
> > > > fairly orthogonal change. But I went back through the code, and I'd
really
> > > like
> > > > to land the THROTTLED and COALESCED changes together. I actually had a
> talk
> > > > about submitting just the throttling changes with tonyg before uploading
> the
> > > > first CL, and the changes made more sense going in together. I'd rather
> add
> > > > another 700 lines of test code that will be redundant to the
> throttling-only
> > > > tests to be honest, although I thought that would be over-kill to
review.
> > > > 
> > > > I can add the tests to make sure that COALESCED Clients operate as
desired
> > > > fairly quickly (similar in function to the added
> > > > ThrottledSpdyProxySchedulesImmediately and OneRequestPerThrottledClient
> > tests
> > > > for THROTTLED clients). All of the state changes should be tested with
the
> > > same
> > > > functions that were already added to test that Clients were correctly
> > > > THROTTLED/UNTHROTTLED, and I had planned to simply update those tests to
> > make
> > > > sure that the correct Clients were COALESCED instead of throttled when
we
> > were
> > > > switching coalescing on. I assumed this would be fine since those same
> unit
> > > > tests would fail if you were attempting to turn coalescing on without
> > updating
> > > > the tests, thus stopping the changes from going through.
> > > > 
> > > > If you'd rather have redundant tests for throttled-only and
> > > > throttled-and-coalesced, I can go do a bunch of copy-pasting with some
> line
> > > > edits and calls to update the scheduler to coalesce at the same time.
> Would
> > it
> > > > be sufficient to add COALESCED versions of some of the tests (ie, do you
> > want
> > > > all combinations of loaded/visible/audible for just THROTTLED and
> > > > THROTTLED/COALESCED? And/or commented out lines of how the functions
will
> be
> > > > updated for coalesced? Or is there something else you'd like me to do?
> > > 
> > > I'm fine with landing them together, if that's what you prefer, as long as
> we
> > > have tests.  I don't think we need tests for all combinations of both
> > > throttled-only and throttled-and-coalesced (I may have gone a bit
overboard
> in
> > > that part of my earlier suggestions).  Think we mostly need to focused on
> > where
> > > behavior's markedly different, or the code follows different paths.
> > 
> > OK, SGTM. I'll add tests for those cases. 
> > 
> > Also, would there be any issue with my using function pointers in the
> > visibility/audibility changes? I didn't see anything about them in the style
> > guidelines, and much of the redundancy could be pretty easily avoided if I
> used
> > them. I'm still transitioning my understanding of what is "too complex" from
a
> > very different coding environment, and could use a bit of feedback on this.
> 
> I'm fine with function pointers.  This isn't so much "too complex" in an
> absolute manner, I'd say "more complex/duplicative than it needs to be".

I uploaded the testing changes and nits. I did a refactor of the
Visibility/Audibility changes that I didn't like. I reverted those changes and
will give you an updated version later today. Since the testing changes and nits
were so extensive, I figured I'd get those to you while I finish the other
change.

aiolos (Not reviewing)

On 2014/07/09 18:08:23, aiolos wrote: > On 2014/07/09 00:10:45, mmenke wrote: > > On 2014/07/09 ...

6 years, 5 months ago (2014-07-09 18:29:03 UTC) #35

On 2014/07/09 18:08:23, aiolos wrote:
> On 2014/07/09 00:10:45, mmenke wrote:
> > On 2014/07/09 00:08:22, aiolos wrote:
> > > On 2014/07/08 23:56:28, mmenke wrote:
> > > > On 2014/07/08 23:33:39, aiolos wrote:
> > > > > On 2014/07/08 22:24:27, mmenke wrote:
> > > > > > On 2014/07/08 21:45:02, mmenke wrote:
> > > > > > > Anyhow, I did suggest a way to avoid walking through all clients
on
> > all
> > > > > > changes
> > > > > > > any more than your code currently does.
> > > > > > 
> > > > > > I don't mean to sound like I'm dead set on something - happy for
other
> > > > ideas,
> > > > > > just think there's too much redundant code.
> > > > > 
> > > > > I'm currently working on factoring some of this out/looking into using
> > your
> > > > > suggestions.
> > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> > > > > > content/browser/loader/resource_scheduler_unittest.cc:133:
> > > > > > scheduler_.SetThrottleOptions(true, false);
> > > > > > Unless we're going to test them in this CL, I don't think we should
> land
> > > any
> > > > > of
> > > > > > the COALESCED or PAUSED logic in this CL (Include the enum values
> > > > themselves).
> > > > > > 
> > > > > > Think there are only maybe 20-30 lines this affects, so shouldn't be
a
> > big
> > > > > deal
> > > > > > to just remove them from this CL.
> > > > > 
> > > > > Taking out PAUSED and adding it again later would be pretty easy since
> > it's
> > > a
> > > > > fairly orthogonal change. But I went back through the code, and I'd
> really
> > > > like
> > > > > to land the THROTTLED and COALESCED changes together. I actually had a
> > talk
> > > > > about submitting just the throttling changes with tonyg before
uploading
> > the
> > > > > first CL, and the changes made more sense going in together. I'd
rather
> > add
> > > > > another 700 lines of test code that will be redundant to the
> > throttling-only
> > > > > tests to be honest, although I thought that would be over-kill to
> review.
> > > > > 
> > > > > I can add the tests to make sure that COALESCED Clients operate as
> desired
> > > > > fairly quickly (similar in function to the added
> > > > > ThrottledSpdyProxySchedulesImmediately and
OneRequestPerThrottledClient
> > > tests
> > > > > for THROTTLED clients). All of the state changes should be tested with
> the
> > > > same
> > > > > functions that were already added to test that Clients were correctly
> > > > > THROTTLED/UNTHROTTLED, and I had planned to simply update those tests
to
> > > make
> > > > > sure that the correct Clients were COALESCED instead of throttled when
> we
> > > were
> > > > > switching coalescing on. I assumed this would be fine since those same
> > unit
> > > > > tests would fail if you were attempting to turn coalescing on without
> > > updating
> > > > > the tests, thus stopping the changes from going through.
> > > > > 
> > > > > If you'd rather have redundant tests for throttled-only and
> > > > > throttled-and-coalesced, I can go do a bunch of copy-pasting with some
> > line
> > > > > edits and calls to update the scheduler to coalesce at the same time.
> > Would
> > > it
> > > > > be sufficient to add COALESCED versions of some of the tests (ie, do
you
> > > want
> > > > > all combinations of loaded/visible/audible for just THROTTLED and
> > > > > THROTTLED/COALESCED? And/or commented out lines of how the functions
> will
> > be
> > > > > updated for coalesced? Or is there something else you'd like me to do?
> > > > 
> > > > I'm fine with landing them together, if that's what you prefer, as long
as
> > we
> > > > have tests.  I don't think we need tests for all combinations of both
> > > > throttled-only and throttled-and-coalesced (I may have gone a bit
> overboard
> > in
> > > > that part of my earlier suggestions).  Think we mostly need to focused
on
> > > where
> > > > behavior's markedly different, or the code follows different paths.
> > > 
> > > OK, SGTM. I'll add tests for those cases. 
> > > 
> > > Also, would there be any issue with my using function pointers in the
> > > visibility/audibility changes? I didn't see anything about them in the
style
> > > guidelines, and much of the redundancy could be pretty easily avoided if I
> > used
> > > them. I'm still transitioning my understanding of what is "too complex"
from
> a
> > > very different coding environment, and could use a bit of feedback on
this.
> > 
> > I'm fine with function pointers.  This isn't so much "too complex" in an
> > absolute manner, I'd say "more complex/duplicative than it needs to be".
> 
> I uploaded the testing changes and nits. I did a refactor of the
> Visibility/Audibility changes that I didn't like. I reverted those changes and
> will give you an updated version later today. Since the testing changes and
nits
> were so extensive, I figured I'd get those to you while I finish the other
> change.

I also noticed that the enum has spesified values. those will be taken out in
the next cl as well.

aiolos (Not reviewing)

On 2014/07/09 18:29:03, aiolos wrote: > On 2014/07/09 18:08:23, aiolos wrote: > > On 2014/07/09 ...

6 years, 5 months ago (2014-07-09 22:25:57 UTC) #36

On 2014/07/09 18:29:03, aiolos wrote:
> On 2014/07/09 18:08:23, aiolos wrote:
> > On 2014/07/09 00:10:45, mmenke wrote:
> > > On 2014/07/09 00:08:22, aiolos wrote:
> > > > On 2014/07/08 23:56:28, mmenke wrote:
> > > > > On 2014/07/08 23:33:39, aiolos wrote:
> > > > > > On 2014/07/08 22:24:27, mmenke wrote:
> > > > > > > On 2014/07/08 21:45:02, mmenke wrote:
> > > > > > > > Anyhow, I did suggest a way to avoid walking through all clients
> on
> > > all
> > > > > > > changes
> > > > > > > > any more than your code currently does.
> > > > > > > 
> > > > > > > I don't mean to sound like I'm dead set on something - happy for
> other
> > > > > ideas,
> > > > > > > just think there's too much redundant code.
> > > > > > 
> > > > > > I'm currently working on factoring some of this out/looking into
using
> > > your
> > > > > > suggestions.
> > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/120001/content/browser/loader/...
> > > > > > > content/browser/loader/resource_scheduler_unittest.cc:133:
> > > > > > > scheduler_.SetThrottleOptions(true, false);
> > > > > > > Unless we're going to test them in this CL, I don't think we
should
> > land
> > > > any
> > > > > > of
> > > > > > > the COALESCED or PAUSED logic in this CL (Include the enum values
> > > > > themselves).
> > > > > > > 
> > > > > > > Think there are only maybe 20-30 lines this affects, so shouldn't
be
> a
> > > big
> > > > > > deal
> > > > > > > to just remove them from this CL.
> > > > > > 
> > > > > > Taking out PAUSED and adding it again later would be pretty easy
since
> > > it's
> > > > a
> > > > > > fairly orthogonal change. But I went back through the code, and I'd
> > really
> > > > > like
> > > > > > to land the THROTTLED and COALESCED changes together. I actually had
a
> > > talk
> > > > > > about submitting just the throttling changes with tonyg before
> uploading
> > > the
> > > > > > first CL, and the changes made more sense going in together. I'd
> rather
> > > add
> > > > > > another 700 lines of test code that will be redundant to the
> > > throttling-only
> > > > > > tests to be honest, although I thought that would be over-kill to
> > review.
> > > > > > 
> > > > > > I can add the tests to make sure that COALESCED Clients operate as
> > desired
> > > > > > fairly quickly (similar in function to the added
> > > > > > ThrottledSpdyProxySchedulesImmediately and
> OneRequestPerThrottledClient
> > > > tests
> > > > > > for THROTTLED clients). All of the state changes should be tested
with
> > the
> > > > > same
> > > > > > functions that were already added to test that Clients were
correctly
> > > > > > THROTTLED/UNTHROTTLED, and I had planned to simply update those
tests
> to
> > > > make
> > > > > > sure that the correct Clients were COALESCED instead of throttled
when
> > we
> > > > were
> > > > > > switching coalescing on. I assumed this would be fine since those
same
> > > unit
> > > > > > tests would fail if you were attempting to turn coalescing on
without
> > > > updating
> > > > > > the tests, thus stopping the changes from going through.
> > > > > > 
> > > > > > If you'd rather have redundant tests for throttled-only and
> > > > > > throttled-and-coalesced, I can go do a bunch of copy-pasting with
some
> > > line
> > > > > > edits and calls to update the scheduler to coalesce at the same
time.
> > > Would
> > > > it
> > > > > > be sufficient to add COALESCED versions of some of the tests (ie, do
> you
> > > > want
> > > > > > all combinations of loaded/visible/audible for just THROTTLED and
> > > > > > THROTTLED/COALESCED? And/or commented out lines of how the functions
> > will
> > > be
> > > > > > updated for coalesced? Or is there something else you'd like me to
do?
> > > > > 
> > > > > I'm fine with landing them together, if that's what you prefer, as
long
> as
> > > we
> > > > > have tests.  I don't think we need tests for all combinations of both
> > > > > throttled-only and throttled-and-coalesced (I may have gone a bit
> > overboard
> > > in
> > > > > that part of my earlier suggestions).  Think we mostly need to focused
> on
> > > > where
> > > > > behavior's markedly different, or the code follows different paths.
> > > > 
> > > > OK, SGTM. I'll add tests for those cases. 
> > > > 
> > > > Also, would there be any issue with my using function pointers in the
> > > > visibility/audibility changes? I didn't see anything about them in the
> style
> > > > guidelines, and much of the redundancy could be pretty easily avoided if
I
> > > used
> > > > them. I'm still transitioning my understanding of what is "too complex"
> from
> > a
> > > > very different coding environment, and could use a bit of feedback on
> this.
> > > 
> > > I'm fine with function pointers.  This isn't so much "too complex" in an
> > > absolute manner, I'd say "more complex/duplicative than it needs to be".
> > 
> > I uploaded the testing changes and nits. I did a refactor of the
> > Visibility/Audibility changes that I didn't like. I reverted those changes
and
> > will give you an updated version later today. Since the testing changes and
> nits
> > were so extensive, I figured I'd get those to you while I finish the other
> > change.
> 
> I also noticed that the enum has spesified values. those will be taken out in
> the next cl as well.

The refactor is done, and testing is updated to include the Coalesced, and new
active_loading states.
I also realized that until coalescing should go in, it would be better to have
the should-be coalesced clients unthrottled instead of throttled since we don't
have any user observable clients to contend with. The paused state has also been
removed.

mmenke

I plan to carefully go over the tests this afternoon. https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.cc#newcode298 ...

6 years, 5 months ago (2014-07-10 15:28:52 UTC) #37

mmenke

Two quick naming comments. https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.cc#newcode400 content/browser/loader/resource_scheduler.cc:400: void ClientIsLoaded(bool loaded) { On ...

6 years, 5 months ago (2014-07-10 15:35:38 UTC) #39

aiolos (Not reviewing)

> > https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.cc#newcode487 > content/browser/loader/resource_scheduler.cc:487: // * Non-HTTP[S] requests. > None of these are true ...

6 years, 5 months ago (2014-07-10 16:13:53 UTC) #40

> 
>
https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:487: //   * Non-HTTP[S] requests.
> None of these are true any more.  Should modify the code so that sync requests
> and non-HTTP[S] requests are always started, and adjust description so it's
> correct about high priority requests are handled.
> 
I had originally planned to keep the changes that I've made and forgot to update
the comments. I should have picked something other than "non-delayable." From
your comments below, I definitely agree that the sync requests should be allowed
through. The non-HTTP[S] ones would be nice to coalesce, since waking up the
disk can also be power hungry, and those requests don't have an effect on other
renders. There should be more logic added later to make the RS aware of dick
wake-ups, similar to the planned network wakeups awareness.
> 
>
https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:544: if
> (!url_request.url().SchemeIsHTTPOrHTTPS()) {
> This was intended not to throttle requests that don't go over the network at
> all.  Suggest keeping that behavior, for now, and investigating changing it
> later, independently histograming it, once we have histograms..  This is a big
> enough change, as-is.  See last star of rule 2.
> 
> Should add a test for this.

Is there a specific test for this (and the Async commented below) you want
added? I'm guessing it's that they aren't blocked as appropriate them? It would
be nice if we could coalesce clients going to disk as well, although I'm not
sure getting that right is in the scope of these CL's.
> 
>
https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:549:
> !ResourceRequestInfo::ForRequest(&url_request)->IsAsync()) {
> Async requests must never be throttled.  The entire renderer with the request
> will lock up (Which may have visible tabs as well) until its current sync
> request completes.
> 
> Should have a test for this.

Good to know.

> 
>
https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:642: if (!client) {
> The Chrome philosophy is not to handle things that shouldn't happen, modulo
> security considerations.  Think this code falls into that case.  Can
optionally
> add a "DCHECK(client);" to document that fact, and/or add a comment about it.
> 
That's really interesting, thanks.

> 
>
https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:838:
> client->LoadCoalescedRequests();
> Why load coalesced requests here instead of waiting for the timer?
> 
> And where is the timer for coalesced requests?
> 
In the next CL. :) We want to do it both on the timer and when the network is
brought up. We know the network is up when we send out another request. This may
be removed when we make the scheduler aware of when the network gets turned on,
but that is not in the scope of these CL's.

All other comments will be fixed in the next CL.

Nice call on adding the other throttle state btw. I think the code became more
readable with that change.

mmenke

On 2014/07/10 16:13:53, aiolos wrote: > > > > > https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.cc#newcode487 > > content/browser/loader/resource_scheduler.cc:487: // ...

6 years, 5 months ago (2014-07-10 16:43:11 UTC) #41

On 2014/07/10 16:13:53, aiolos wrote:
> > 
> >
>
https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:487: //   * Non-HTTP[S]
requests.
> > None of these are true any more.  Should modify the code so that sync
requests
> > and non-HTTP[S] requests are always started, and adjust description so it's
> > correct about high priority requests are handled.
> > 
> I had originally planned to keep the changes that I've made and forgot to
update
> the comments. I should have picked something other than "non-delayable." From
> your comments below, I definitely agree that the sync requests should be
allowed
> through. The non-HTTP[S] ones would be nice to coalesce, since waking up the
> disk can also be power hungry, and those requests don't have an effect on
other
> renders. There should be more logic added later to make the RS aware of dick
> wake-ups, similar to the planned network wakeups awareness.

Hrm...hadn't considered the cost of waking up SSDs/CPUs as opposed to doing
everything in one go.  I'm fine with experimenting with coalescing them
independently for network requets, just think we should try to keep changes
small, and look at histograms as we incrementally change things.  When looking
at histograms, I want to make sure we know what we're looking at, and just
restricting changes (for now) to HTTP/HTTPS requests seems like a good way to do
this.


>
https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:544: if
> > (!url_request.url().SchemeIsHTTPOrHTTPS()) {
> > This was intended not to throttle requests that don't go over the network at
> > all.  Suggest keeping that behavior, for now, and investigating changing it
> > later, independently histograming it, once we have histograms..  This is a
big
> > enough change, as-is.  See last star of rule 2.
> > 
> > Should add a test for this.
> 
> Is there a specific test for this (and the Async commented below) you want
> added? I'm guessing it's that they aren't blocked as appropriate them? It
would
> be nice if we could coalesce clients going to disk as well, although I'm not
> sure getting that right is in the scope of these CL's.

Just that we don't block them when coalescing or throttling requests.


>
https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:549:
> > !ResourceRequestInfo::ForRequest(&url_request)->IsAsync()) {
> > Async requests must never be throttled.  The entire renderer with the
request
> > will lock up (Which may have visible tabs as well) until its current sync
> > request completes.
> > 
> > Should have a test for this.
> 
> Good to know.
> 
>
https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:642: if (!client) {
> > The Chrome philosophy is not to handle things that shouldn't happen, modulo
> > security considerations.  Think this code falls into that case.  Can
> optionally
> > add a "DCHECK(client);" to document that fact, and/or add a comment about
it.
> > 
> That's really interesting, thanks.

No problem.  The idea is for them to show up as crash dumps, and we can figure
out if there's a bug, or the assumption is wrong.  Then fix and write tests,
either way.

mmenke

Oh, and one other important issue. https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.cc#newcode242 content/browser/loader/resource_scheduler.cc:242: ~Client() {} You ...

6 years, 5 months ago (2014-07-10 16:44:20 UTC) #42

aiolos (Not reviewing)

> https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.cc#newcode537 > content/browser/loader/resource_scheduler.cc:537: if (throttle_state_ == > THROTTLED && > Think this should be ...

6 years, 5 months ago (2014-07-10 17:10:22 UTC) #43

mmenke

On 2014/07/10 17:10:22, aiolos wrote: > > > https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.cc#newcode537 > > content/browser/loader/resource_scheduler.cc:537: if (throttle_state_ == ...

6 years, 5 months ago (2014-07-10 17:25:08 UTC) #44

aiolos (Not reviewing)

> https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.h > File content/browser/loader/resource_scheduler.h (right): > > https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.h#newcode69 > content/browser/loader/resource_scheduler.h:69: ACTIVE_LOADING, > nit: ACTIVELY_LOADING ...

6 years, 5 months ago (2014-07-10 17:35:58 UTC) #45

mmenke

On 2014/07/10 17:35:58, aiolos wrote: > > > https://codereview.chromium.org/357583003/diff/160001/content/browser/loader/resource_scheduler.h > > File content/browser/loader/resource_scheduler.h (right): > ...

6 years, 5 months ago (2014-07-10 17:46:42 UTC) #46

aiolos (Not reviewing)

On 2014/07/10 17:46:42, mmenke wrote: > On 2014/07/10 17:35:58, aiolos wrote: > > > > ...

6 years, 5 months ago (2014-07-10 17:48:04 UTC) #47

aiolos (Not reviewing)

I added the requested tests, plus the following: UNTHROTTLED and ACTIVE_AND_LOADING versions of the Sync ...

6 years, 5 months ago (2014-07-10 19:24:09 UTC) #48

mmenke

Still haven't had a chance to go through all the tests, but don't expect to ...

6 years, 5 months ago (2014-07-10 19:37:36 UTC) #49

Still haven't had a chance to go through all the tests, but don't expect to have
much feedback there, other than maybe suggesting a couple more that will take
all of 5 seconds each to write.  Will either get to them later today, or
tomorrow (I am sheriffing tomorrow, but should still have time).

https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:242: ~Client() {
nit:  Should probably have a blank line here, now that the destructor actually
does something.

https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:244:
scheduler_->DecrementActiveClientsLoading();
BUG:  Clients are currently deleted before they are erased.  Need to switch that
ordering.

https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:490: // 2. Requests to SPDY-capable
origin servers.
These are delayable for all but unthrottled clients, actually.

https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:493: //   * Higher priority
requests (>= net::LOW).
These are delayable for all but unthrottled clients, too.

https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:527: // user-observable Clients
nit:  +period

https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:561: in_flight_requests_.size() >=
kMaxNumThrottledRequestsPerClient) {
Hrm...we're not giving any preference to higher priority requests on throttled
clients, are we?  We're just letting the first one through.

https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:817: DCHECK(loading_active_clients_
>= 0);
Per earlier commend, suggest also a:

DCHECK_EQ(loading_active_clients_, CountLoadingActiveClients());

Where CountLoadingActiveClients walks through all clients and counts just the
loading and active ones.  This will take O(n) time on debug builds, but I don't
think we really care.

https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
File content/browser/loader/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:641: kBackgroundRouteId));
Seems like a bad idea to be setting options when there are pre-existing clients.
 Could just make SetThrottleOptions update the state of all existing throttles,
or could add a DCHECK in SetThrottleOptions that this doesn't happen, and adjust
the test not to do this.

A couple ways to go about the latter - either need an initialization function or
or a child test fixture (Initialization function could just re-create the
scheduler, so don't have to always call it).  In theory, I prefer the DCHECK
approach, to make sure we aren't testing the behavior of SetThrottleOptions
itself, but in practice, updating all the existing clients is probably simpler.

aiolos (Not reviewing)

On 2014/07/10 19:37:36, mmenke wrote: > Still haven't had a chance to go through all ...

6 years, 5 months ago (2014-07-10 20:13:33 UTC) #50

On 2014/07/10 19:37:36, mmenke wrote:
> Still haven't had a chance to go through all the tests, but don't expect to
have
> much feedback there, other than maybe suggesting a couple more that will take
> all of 5 seconds each to write.  Will either get to them later today, or
> tomorrow (I am sheriffing tomorrow, but should still have time).
> 
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> File content/browser/loader/resource_scheduler.cc (right):
> 
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:242: ~Client() {
> nit:  Should probably have a blank line here, now that the destructor actually
> does something.
> 
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:244:
> scheduler_->DecrementActiveClientsLoading();
> BUG:  Clients are currently deleted before they are erased.  Need to switch
that
> ordering.
> 
I moved the logic to OnClientDeleted instead.

>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:490: // 2. Requests to
SPDY-capable
> origin servers.
> These are delayable for all but unthrottled clients, actually.

That's not quite what we do here though, since we let them through on THROTTLED
clients as well. Letting the spdy capable requests through then was my decision
based on talks with willchan. I think we can still let them through, especially
since there will be a later CL that limits the number of Throttled clients
loading while there is and active and loading client.
> 
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:493: //   * Higher priority
> requests (>= net::LOW).
> These are delayable for all but unthrottled clients, too.

Throttled Clients let one request through at a time, with the exception that all
sync, non-http(s) and spdy-capable requests go through immediately.

I'm relabeling the "delayable" requests to"low-priority" to help avoid
confusion.
> 
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:527: // user-observable Clients
> nit:  +period
> 
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:561: in_flight_requests_.size()
>=
> kMaxNumThrottledRequestsPerClient) {
> Hrm...we're not giving any preference to higher priority requests on throttled
> clients, are we?  We're just letting the first one through.

The requests are sorted by priority, so I believe we would let the first
received request through first, and then let them through based on priority.
I'll write another test for this on Throttled Clients just to be sure.
> 
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:817:
DCHECK(loading_active_clients_
> >= 0);
> Per earlier commend, suggest also a:
> 
> DCHECK_EQ(loading_active_clients_, CountLoadingActiveClients());
> 
> Where CountLoadingActiveClients walks through all clients and counts just the
> loading and active ones.  This will take O(n) time on debug builds, but I
don't
> think we really care.
> 
Thanks for catching that again. Changing.

>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> File content/browser/loader/resource_scheduler_unittest.cc (right):
> 
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> content/browser/loader/resource_scheduler_unittest.cc:641:
kBackgroundRouteId));
> Seems like a bad idea to be setting options when there are pre-existing
clients.
>  Could just make SetThrottleOptions update the state of all existing
throttles,
> or could add a DCHECK in SetThrottleOptions that this doesn't happen, and
adjust
> the test not to do this.
> 
> A couple ways to go about the latter - either need an initialization function
or
> or a child test fixture (Initialization function could just re-create the
> scheduler, so don't have to always call it).  In theory, I prefer the DCHECK
> approach, to make sure we aren't testing the behavior of SetThrottleOptions
> itself, but in practice, updating all the existing clients is probably
simpler.

The latter would be preferable, especially since that function should only get
called for testing (unit or Finch) purposes. I think I'll add ForTesting to the
end of it to make that obvious. I believe I have checks to make sure the Clients
are in the expected states after that is called, but I'll double check.

aiolos (Not reviewing)

On 2014/07/10 20:13:33, aiolos wrote: > On 2014/07/10 19:37:36, mmenke wrote: > > Still haven't ...

6 years, 5 months ago (2014-07-11 16:12:43 UTC) #51

On 2014/07/10 20:13:33, aiolos wrote:
> On 2014/07/10 19:37:36, mmenke wrote:
> > Still haven't had a chance to go through all the tests, but don't expect to
> have
> > much feedback there, other than maybe suggesting a couple more that will
take
> > all of 5 seconds each to write.  Will either get to them later today, or
> > tomorrow (I am sheriffing tomorrow, but should still have time).
> > 
> >
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> > File content/browser/loader/resource_scheduler.cc (right):
> > 
> >
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:242: ~Client() {
> > nit:  Should probably have a blank line here, now that the destructor
actually
> > does something.
> > 
> >
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:244:
> > scheduler_->DecrementActiveClientsLoading();
> > BUG:  Clients are currently deleted before they are erased.  Need to switch
> that
> > ordering.
> > 
> I moved the logic to OnClientDeleted instead.
> 
> >
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:490: // 2. Requests to
> SPDY-capable
> > origin servers.
> > These are delayable for all but unthrottled clients, actually.
> 
> That's not quite what we do here though, since we let them through on
THROTTLED
> clients as well. Letting the spdy capable requests through then was my
decision
> based on talks with willchan. I think we can still let them through,
especially
> since there will be a later CL that limits the number of Throttled clients
> loading while there is and active and loading client.
> > 
> >
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:493: //   * Higher priority
> > requests (>= net::LOW).
> > These are delayable for all but unthrottled clients, too.
> 
> Throttled Clients let one request through at a time, with the exception that
all
> sync, non-http(s) and spdy-capable requests go through immediately.
> 
> I'm relabeling the "delayable" requests to"low-priority" to help avoid
> confusion.
> > 
> >
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:527: // user-observable Clients
> > nit:  +period
> > 
> >
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:561: in_flight_requests_.size()
> >=
> > kMaxNumThrottledRequestsPerClient) {
> > Hrm...we're not giving any preference to higher priority requests on
throttled
> > clients, are we?  We're just letting the first one through.
> 
> The requests are sorted by priority, so I believe we would let the first
> received request through first, and then let them through based on priority.
> I'll write another test for this on Throttled Clients just to be sure.
> > 
> >
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:817:
> DCHECK(loading_active_clients_
> > >= 0);
> > Per earlier commend, suggest also a:
> > 
> > DCHECK_EQ(loading_active_clients_, CountLoadingActiveClients());
> > 
> > Where CountLoadingActiveClients walks through all clients and counts just
the
> > loading and active ones.  This will take O(n) time on debug builds, but I
> don't
> > think we really care.
> > 
> Thanks for catching that again. Changing.
> 
> >
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> > File content/browser/loader/resource_scheduler_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/357583003/diff/180001/content/browser/loader/...
> > content/browser/loader/resource_scheduler_unittest.cc:641:
> kBackgroundRouteId));
> > Seems like a bad idea to be setting options when there are pre-existing
> clients.
> >  Could just make SetThrottleOptions update the state of all existing
> throttles,
> > or could add a DCHECK in SetThrottleOptions that this doesn't happen, and
> adjust
> > the test not to do this.
> > 
> > A couple ways to go about the latter - either need an initialization
function
> or
> > or a child test fixture (Initialization function could just re-create the
> > scheduler, so don't have to always call it).  In theory, I prefer the DCHECK
> > approach, to make sure we aren't testing the behavior of SetThrottleOptions
> > itself, but in practice, updating all the existing clients is probably
> simpler.
> 
> The latter would be preferable, especially since that function should only get
> called for testing (unit or Finch) purposes. I think I'll add ForTesting to
the
> end of it to make that obvious. I believe I have checks to make sure the
Clients
> are in the expected states after that is called, but I'll double check.

Sorry, I meant the latter would *not* be preferable.

mmenke

Sorry for not getting to this last week. Was busier as a sheriff than I ...

6 years, 5 months ago (2014-07-14 15:47:21 UTC) #52

aiolos (Not reviewing)

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/resource_scheduler.cc#newcode492 content/browser/loader/resource_scheduler.cc:492: // for scheduleing purposes in UNTHROTTLED and ACTIVE_AND_LOADING cilents. ...

6 years, 5 months ago (2014-07-14 16:48:21 UTC) #53

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:492: //   for scheduleing purposes
in UNTHROTTLED and ACTIVE_AND_LOADING cilents.
On 2014/07/14 15:47:20, mmenke wrote:
> Having "delayable" not include everything that's not "non-delayable" is very
> confusing.  Suggest just calling them "low priority requests" and stick with
> that.

The problem is that there is a separate "non-delayable" mechanism already going
on here. I would prefer to keep references to that in the comments. That's why I
labeled them as low-priority and had an explanation as to what that means for
the UNTHROTTLED and ACTIVE_AND_LOADING Clients. I'll keep them labeled as just
low-priority here, and move the delayable/non-delayable information into the
ThrottleState specific comments to clear this up.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
File content/browser/loader/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:925: }
On 2014/07/14 15:47:21, mmenke wrote:
> Do we have any tests where requests are started as a result of the loading
state
> of another client changing?

FullVisibleLoadedCorrectlyUnthrottle checks that THROTTLED tabs become
UNTHROTTLED.
If you're asking about COALESCED, I thought I'd written one, but don't see it.
I'll fix that.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:983:
scheduler_.OnClientCreated(kBackgroundChildId2, kBackgroundRouteId2);
On 2014/07/14 15:47:21, mmenke wrote:
> Do we need kBackgroundChildId2 in addition to kBackgroundChildId in the tests
> where neither it nor kBackgroundChildId ever change sate, after being hidden
> initially?

kBackgroundChildId2 is loaded that entire time, and kBackgroundChildId is
loading. I had both of them to cover both cases, even though they should both be
in the THROTTLED state.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:1475:
TEST_F(ResourceSchedulerTest, PartialVisibleClientLoadedDoesNotUnthrottle) {
On 2014/07/14 15:47:21, mmenke wrote:
> What do you mean by partial visible?

Partial (VisibleClientLoaded)

There are two visible clients, and one finishes loading.

aiolos (Not reviewing)

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/resource_scheduler.cc#newcode281 content/browser/loader/resource_scheduler.cc:281: void OnLoadingStateChanged(bool loaded) { On 2014/07/14 15:47:20, mmenke wrote: ...

6 years, 5 months ago (2014-07-14 19:22:57 UTC) #54

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:281: void
OnLoadingStateChanged(bool loaded) {
On 2014/07/14 15:47:20, mmenke wrote:
> nit:  Suggest renaming all these arguments to is_loaded / is_audible / etc, to
> be consistent.

Done

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:287: }
On 2014/07/14 15:47:20, mmenke wrote:
> nit:  Suggest putting this after the other two, to match the declaration order
> of the three variables.

Done

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:324: }
On 2014/07/14 15:47:20, mmenke wrote:
> optional:  This is fine, but another way to do this would be:
> 
> if (throttle state == old_throttle_state)
>   return;
> // Do other checks here, without the check for whether or not the state
changed.
> 
> Think less redundancy and single-line ifs are marginally clearer, feel free to
> disagree.

Done

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:412: START_REQUEST = 1,
On 2014/07/14 15:47:20, mmenke wrote:
> Need to fix the merge.

Done.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:492: //   for scheduleing purposes
in UNTHROTTLED and ACTIVE_AND_LOADING cilents.
On 2014/07/14 15:47:20, mmenke wrote:
> nit: clients

Done

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:492: //   for scheduleing purposes
in UNTHROTTLED and ACTIVE_AND_LOADING cilents.
On 2014/07/14 15:47:20, mmenke wrote:
> nit:  Scheduling.

Done

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:498: //      immediately
On 2014/07/14 15:47:20, mmenke wrote:
> nit:  Should use consistent indentation for the bullet-point continuation
lines.
>  Some you're indenting 3 relative to the asterisk, others you're indenting
two.

Done.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:509: //     priority non-SPDY
request already being loaded
On 2014/07/14 15:47:20, mmenke wrote:
> And if there's an equal priority non-SPDY request, the behavior depends on
> whether or not the non-SPDY request was issued first.  I think this is a
> somewhat weird requirement.  Think we should be more consistent about our
> behavior here.

All SPDY-capable requests are now issued. Comments updated to reflect this.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:815: DCHECK(loading_active_clients_
>= 0);
On 2014/07/14 15:47:20, mmenke wrote:
> nit:  DCHECK_GE (x2)

Does this check even make sense? I thought size_t was an unsigned long?

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:837:
client->LoadCoalescedRequests();
On 2014/07/14 15:47:20, mmenke wrote:
> From before:
> 
> >> content/browser/loader/resource_scheduler.cc:838:
> >> client->LoadCoalescedRequests();
> >> Why load coalesced requests here instead of waiting for the timer?
> >> 
> >> And where is the timer for coalesced requests?
> >> 
> > In the next CL. :) We want to do it both on the timer and when the network
is
> > brought up. We know the network is up when we send out another request. This
> may
> > be removed when we make the scheduler aware of when the network gets turned
> on,
> > but that is not in the scope of these CL's.
> 
> How do we know the network is up when an active client becomes inactive, or
> vice-versa?  I remain pretty skeptical of the utility of this.

I don't mind taking this out. The next CL was going to add logic that would load
coalesced requests on ACTIVE_AND_LOADING clients, but it would probably be
cleaner just to wait until I hook up signals from the network turning on. Do you
have a preference on that?

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.h:118: void
IncrementActiveClientsLoading();
On 2014/07/14 15:47:20, mmenke wrote:
> Think it's worth noting that these may update the states of all existing
clients
> (i.e. it's re-entrant, which is a potential source of bugs, and thus worth
> commenting on)

Done.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.h:118: void
IncrementActiveClientsLoading();
On 2014/07/14 15:47:20, mmenke wrote:
> These should be private.  Subclasses have access to their parents interal
> functions.

Done.
> 
> Same goes for should_coalesce() and should_throttle() (Clients can actually
> access the fields directly, but I think that having them use accessor methods
is
> a cleaner).

These are actually used in the unittests as well as the Client. Plus, they are
going to be removed when throttling and coalescing are turned on by default
anyway. I think it makes more sense to leave them next to
SetThrottleOptionsForTesting until then.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.h:124: void OnVisibilityChanged(int
child_id, int route_id, bool visibility);
On 2014/07/14 15:47:20, mmenke wrote:
> optional:  Suggest is_audible / is_visible instead.  The names are a little
> clearer, and match what's used in Client as well.

Done.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.h:168: size_t loading_active_clients_;
On 2014/07/14 15:47:20, mmenke wrote:
> nit:  This should probable match Decrement/IncrementActiveClientsLoading and
> CountLoadingActiveClients.  I don't really care about the order
> (loading_active_clients vs active_clients_loading_), should just be
consistent.

Done.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
File content/browser/loader/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:553:
scoped_ptr<TestRequest> low(NewRequest("http://host/high", net::LOWEST));
On 2014/07/14 15:47:21, mmenke wrote:
> Should these be for the same url?  Using "http://host/high" for the low
priority
> request also seems a little strange (For for a number of these tests).

Done.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:754:
TEST_F(ResourceSchedulerTest, OneRequestPerThrottledClient) {
On 2014/07/14 15:47:21, mmenke wrote:
> I feel that when test B cannot possible pass unless test A also passes, test A
> should go first, particularly when test A is a subset of test B.
> 
> So suggest putting this before UnthrottleNewlyVisibleClient.

Done.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:901:
TEST_F(ResourceSchedulerTest, CoalescedClientIssuesNoRequests) {
On 2014/07/14 15:47:21, mmenke wrote:
> nit:  Per earlier comment, this should probably go before
> CoalescedSyncSchedulesImmediately

Done. Moved before CoalescedSpdyProxyWaits, since that is the first Coalesced
request test that depends on similar logic.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:925: }
On 2014/07/14 16:48:21, aiolos wrote:
> On 2014/07/14 15:47:21, mmenke wrote:
> > Do we have any tests where requests are started as a result of the loading
> state
> > of another client changing?
> 
> FullVisibleLoadedCorrectlyUnthrottle checks that THROTTLED tabs become
> UNTHROTTLED.
> If you're asking about COALESCED, I thought I'd written one, but don't see it.
> I'll fix that.

With removing the line that loads coalesced requests on ACTIVE_AND_LOADING state
changes, we no longer need this test. Added coalesced changes in the next CL
will have testing for those changes.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:928:
UnLoadedClientVisibiltyChangedCorrectlyUnthrottles) {
On 2014/07/14 15:47:21, mmenke wrote:
> nit:  Visibility.  Check other test names for that, too.

Done.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:928:
UnLoadedClientVisibiltyChangedCorrectlyUnthrottles) {
On 2014/07/14 15:47:21, mmenke wrote:
> nit:  Unloaded is one word.  Same goes for other tests.

Done.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:981:
UnLoadedClientAudibiltyChangedCorrectlyUnthrottles) {
On 2014/07/14 15:47:21, mmenke wrote:
> nit:  Audibility

Done.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:1580:
TEST_F(ResourceSchedulerTest, AllBackgroundClientsUnthrottle) {
Moved before UnloadedClientVisibilityChangedCorrectlyUnthrottles.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:1618: }
On 2014/07/14 15:47:21, mmenke wrote:
> Suggest one more test:  Throttled become unthrottled when an observable
loading
> tab is destroyed.

Done.

mmenke

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/resource_scheduler.cc#newcode815 content/browser/loader/resource_scheduler.cc:815: DCHECK(loading_active_clients_ >= 0); On 2014/07/14 19:22:55, aiolos wrote: > ...

6 years, 5 months ago (2014-07-14 19:38:12 UTC) #55

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:815: DCHECK(loading_active_clients_
>= 0);
On 2014/07/14 19:22:55, aiolos wrote:
> On 2014/07/14 15:47:20, mmenke wrote:
> > nit:  DCHECK_GE (x2)
> 
> Does this check even make sense? I thought size_t was an unsigned long?

You're right, it doesn't, so should get rid of them.

You may want to put a DCHECK_NE(0u, loading_active_clients_) here.  It's
redundant with the CountLoadingActiveClients check, but I think it's reasonable,
though not necessary, to have a separate check for that particular case.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:837:
client->LoadCoalescedRequests();
On 2014/07/14 19:22:55, aiolos wrote:
> On 2014/07/14 15:47:20, mmenke wrote:
> > From before:
> > 
> > >> content/browser/loader/resource_scheduler.cc:838:
> > >> client->LoadCoalescedRequests();
> > >> Why load coalesced requests here instead of waiting for the timer?
> > >> 
> > >> And where is the timer for coalesced requests?
> > >> 
> > > In the next CL. :) We want to do it both on the timer and when the network
> is
> > > brought up. We know the network is up when we send out another request.
This
> > may
> > > be removed when we make the scheduler aware of when the network gets
turned
> > on,
> > > but that is not in the scope of these CL's.
> > 
> > How do we know the network is up when an active client becomes inactive, or
> > vice-versa?  I remain pretty skeptical of the utility of this.
> 
> I don't mind taking this out. The next CL was going to add logic that would
load
> coalesced requests on ACTIVE_AND_LOADING clients, but it would probably be
> cleaner just to wait until I hook up signals from the network turning on. Do
you
> have a preference on that?

I'm fine with waiting.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler.h:118: void
IncrementActiveClientsLoading();
On 2014/07/14 19:22:56, aiolos wrote:
> On 2014/07/14 15:47:20, mmenke wrote:
> > These should be private.  Subclasses have access to their parents interal
> > functions.
> 
> Done.
> > 
> > Same goes for should_coalesce() and should_throttle() (Clients can actually
> > access the fields directly, but I think that having them use accessor
methods
> is
> > a cleaner).
> 
> These are actually used in the unittests as well as the Client. Plus, they are
> going to be removed when throttling and coalescing are turned on by default
> anyway. I think it makes more sense to leave them next to
> SetThrottleOptionsForTesting until then.

SGTM.

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
File content/browser/loader/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:925: }
On 2014/07/14 16:48:21, aiolos wrote:
> On 2014/07/14 15:47:21, mmenke wrote:
> > Do we have any tests where requests are started as a result of the loading
> state
> > of another client changing?
> 
> FullVisibleLoadedCorrectlyUnthrottle checks that THROTTLED tabs become
> UNTHROTTLED.
> If you're asking about COALESCED, I thought I'd written one, but don't see it.
> I'll fix that.

I was thinking about UNTROTTLED.  FullVisibleLoadedCorrectlyUnthrottle  doesn't
check that requests are actually unthrottled, just that the state of the tab
changes.  As-is, I'm not sure any test would fail if
OnLoadingActiveClientsStateChanged just updated the publicly visible throttle
state, without starting newly unthrottled requests.

More generally, it's generally suggested that tests should test the public API
of a class, rather than how the internals work.  It would be sufficiently
painful to do that for all tests here that I'm not going to suggest you change
all tests to do this, but think we should have at least one where we make sure
unthrottling a client actually unthrottles pre-existing requests.

https://codereview.chromium.org/357583003/diff/220001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/357583003/diff/220001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:642: DCHECK(client_map_.empty());
Maybe DCHECK_EQ(0u, loading_active_clients_)?

aiolos (Not reviewing)

On 2014/07/14 19:38:12, mmenke wrote: > https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/resource_scheduler.cc > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/resource_scheduler.cc#newcode815 > ...

6 years, 5 months ago (2014-07-14 20:37:00 UTC) #56

On 2014/07/14 19:38:12, mmenke wrote:
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> File content/browser/loader/resource_scheduler.cc (right):
> 
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:815:
DCHECK(loading_active_clients_
> >= 0);
> On 2014/07/14 19:22:55, aiolos wrote:
> > On 2014/07/14 15:47:20, mmenke wrote:
> > > nit:  DCHECK_GE (x2)
> > 
> > Does this check even make sense? I thought size_t was an unsigned long?
> 
> You're right, it doesn't, so should get rid of them.
> 
> You may want to put a DCHECK_NE(0u, loading_active_clients_) here.  It's
> redundant with the CountLoadingActiveClients check, but I think it's
reasonable,
> though not necessary, to have a separate check for that particular case.
> 
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:837:
> client->LoadCoalescedRequests();
> On 2014/07/14 19:22:55, aiolos wrote:
> > On 2014/07/14 15:47:20, mmenke wrote:
> > > From before:
> > > 
> > > >> content/browser/loader/resource_scheduler.cc:838:
> > > >> client->LoadCoalescedRequests();
> > > >> Why load coalesced requests here instead of waiting for the timer?
> > > >> 
> > > >> And where is the timer for coalesced requests?
> > > >> 
> > > > In the next CL. :) We want to do it both on the timer and when the
network
> > is
> > > > brought up. We know the network is up when we send out another request.
> This
> > > may
> > > > be removed when we make the scheduler aware of when the network gets
> turned
> > > on,
> > > > but that is not in the scope of these CL's.
> > > 
> > > How do we know the network is up when an active client becomes inactive,
or
> > > vice-versa?  I remain pretty skeptical of the utility of this.
> > 
> > I don't mind taking this out. The next CL was going to add logic that would
> load
> > coalesced requests on ACTIVE_AND_LOADING clients, but it would probably be
> > cleaner just to wait until I hook up signals from the network turning on. Do
> you
> > have a preference on that?
> 
> I'm fine with waiting.
> 
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> File content/browser/loader/resource_scheduler.h (right):
> 
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> content/browser/loader/resource_scheduler.h:118: void
> IncrementActiveClientsLoading();
> On 2014/07/14 19:22:56, aiolos wrote:
> > On 2014/07/14 15:47:20, mmenke wrote:
> > > These should be private.  Subclasses have access to their parents interal
> > > functions.
> > 
> > Done.
> > > 
> > > Same goes for should_coalesce() and should_throttle() (Clients can
actually
> > > access the fields directly, but I think that having them use accessor
> methods
> > is
> > > a cleaner).
> > 
> > These are actually used in the unittests as well as the Client. Plus, they
are
> > going to be removed when throttling and coalescing are turned on by default
> > anyway. I think it makes more sense to leave them next to
> > SetThrottleOptionsForTesting until then.
> 
> SGTM.
> 
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> File content/browser/loader/resource_scheduler_unittest.cc (right):
> 
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> content/browser/loader/resource_scheduler_unittest.cc:925: }
> On 2014/07/14 16:48:21, aiolos wrote:
> > On 2014/07/14 15:47:21, mmenke wrote:
> > > Do we have any tests where requests are started as a result of the loading
> > state
> > > of another client changing?
> > 
> > FullVisibleLoadedCorrectlyUnthrottle checks that THROTTLED tabs become
> > UNTHROTTLED.
> > If you're asking about COALESCED, I thought I'd written one, but don't see
it.
> > I'll fix that.
> 
> I was thinking about UNTROTTLED.  FullVisibleLoadedCorrectlyUnthrottle 
doesn't
> check that requests are actually unthrottled, just that the state of the tab
> changes.  As-is, I'm not sure any test would fail if
> OnLoadingActiveClientsStateChanged just updated the publicly visible throttle
> state, without starting newly unthrottled requests.

I'll add request checks.

> 
> More generally, it's generally suggested that tests should test the public API
> of a class, rather than how the internals work.  It would be sufficiently
> painful to do that for all tests here that I'm not going to suggest you change
> all tests to do this, but think we should have at least one where we make sure
> unthrottling a client actually unthrottles pre-existing requests.
> 
I agree, there are enough state change/request types combinations that I don't
believe it would be feasible to get complete coverage for all combinations.

>
https://codereview.chromium.org/357583003/diff/220001/content/browser/loader/...
> File content/browser/loader/resource_scheduler.cc (right):
> 
>
https://codereview.chromium.org/357583003/diff/220001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:642: DCHECK(client_map_.empty());
> Maybe DCHECK_EQ(0u, loading_active_clients_)?

But there can be 0 active loading clients without there being an error. Is there
an overflow value you want me checking instead? It might just be best to let the
CountActiveClientsLoading check handle that case.

mmenke

On 2014/07/14 20:37:00, aiolos wrote: > On 2014/07/14 19:38:12, mmenke wrote: > > > https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/resource_scheduler.cc ...

6 years, 5 months ago (2014-07-14 20:41:29 UTC) #57

On 2014/07/14 20:37:00, aiolos wrote:
> On 2014/07/14 19:38:12, mmenke wrote:
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > File content/browser/loader/resource_scheduler.cc (right):
> > 
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:815:
> DCHECK(loading_active_clients_
> > >= 0);
> > On 2014/07/14 19:22:55, aiolos wrote:
> > > On 2014/07/14 15:47:20, mmenke wrote:
> > > > nit:  DCHECK_GE (x2)
> > > 
> > > Does this check even make sense? I thought size_t was an unsigned long?
> > 
> > You're right, it doesn't, so should get rid of them.
> > 
> > You may want to put a DCHECK_NE(0u, loading_active_clients_) here.  It's
> > redundant with the CountLoadingActiveClients check, but I think it's
> reasonable,
> > though not necessary, to have a separate check for that particular case.
> > 
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:837:
> > client->LoadCoalescedRequests();
> > On 2014/07/14 19:22:55, aiolos wrote:
> > > On 2014/07/14 15:47:20, mmenke wrote:
> > > > From before:
> > > > 
> > > > >> content/browser/loader/resource_scheduler.cc:838:
> > > > >> client->LoadCoalescedRequests();
> > > > >> Why load coalesced requests here instead of waiting for the timer?
> > > > >> 
> > > > >> And where is the timer for coalesced requests?
> > > > >> 
> > > > > In the next CL. :) We want to do it both on the timer and when the
> network
> > > is
> > > > > brought up. We know the network is up when we send out another
request.
> > This
> > > > may
> > > > > be removed when we make the scheduler aware of when the network gets
> > turned
> > > > on,
> > > > > but that is not in the scope of these CL's.
> > > > 
> > > > How do we know the network is up when an active client becomes inactive,
> or
> > > > vice-versa?  I remain pretty skeptical of the utility of this.
> > > 
> > > I don't mind taking this out. The next CL was going to add logic that
would
> > load
> > > coalesced requests on ACTIVE_AND_LOADING clients, but it would probably be
> > > cleaner just to wait until I hook up signals from the network turning on.
Do
> > you
> > > have a preference on that?
> > 
> > I'm fine with waiting.
> > 
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > File content/browser/loader/resource_scheduler.h (right):
> > 
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.h:118: void
> > IncrementActiveClientsLoading();
> > On 2014/07/14 19:22:56, aiolos wrote:
> > > On 2014/07/14 15:47:20, mmenke wrote:
> > > > These should be private.  Subclasses have access to their parents
interal
> > > > functions.
> > > 
> > > Done.
> > > > 
> > > > Same goes for should_coalesce() and should_throttle() (Clients can
> actually
> > > > access the fields directly, but I think that having them use accessor
> > methods
> > > is
> > > > a cleaner).
> > > 
> > > These are actually used in the unittests as well as the Client. Plus, they
> are
> > > going to be removed when throttling and coalescing are turned on by
default
> > > anyway. I think it makes more sense to leave them next to
> > > SetThrottleOptionsForTesting until then.
> > 
> > SGTM.
> > 
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > File content/browser/loader/resource_scheduler_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > content/browser/loader/resource_scheduler_unittest.cc:925: }
> > On 2014/07/14 16:48:21, aiolos wrote:
> > > On 2014/07/14 15:47:21, mmenke wrote:
> > > > Do we have any tests where requests are started as a result of the
loading
> > > state
> > > > of another client changing?
> > > 
> > > FullVisibleLoadedCorrectlyUnthrottle checks that THROTTLED tabs become
> > > UNTHROTTLED.
> > > If you're asking about COALESCED, I thought I'd written one, but don't see
> it.
> > > I'll fix that.
> > 
> > I was thinking about UNTROTTLED.  FullVisibleLoadedCorrectlyUnthrottle 
> doesn't
> > check that requests are actually unthrottled, just that the state of the tab
> > changes.  As-is, I'm not sure any test would fail if
> > OnLoadingActiveClientsStateChanged just updated the publicly visible
throttle
> > state, without starting newly unthrottled requests.
> 
> I'll add request checks.
> 
> > 
> > More generally, it's generally suggested that tests should test the public
API
> > of a class, rather than how the internals work.  It would be sufficiently
> > painful to do that for all tests here that I'm not going to suggest you
change
> > all tests to do this, but think we should have at least one where we make
sure
> > unthrottling a client actually unthrottles pre-existing requests.
> > 
> I agree, there are enough state change/request types combinations that I don't
> believe it would be feasible to get complete coverage for all combinations.
> 
> >
>
https://codereview.chromium.org/357583003/diff/220001/content/browser/loader/...
> > File content/browser/loader/resource_scheduler.cc (right):
> > 
> >
>
https://codereview.chromium.org/357583003/diff/220001/content/browser/loader/...
> > content/browser/loader/resource_scheduler.cc:642:
DCHECK(client_map_.empty());
> > Maybe DCHECK_EQ(0u, loading_active_clients_)?
> 
> But there can be 0 active loading clients without there being an error. Is
there
> an overflow value you want me checking instead? It might just be best to let
the
> CountActiveClientsLoading check handle that case.

Right.  There must be 0 active loading clients on destruction, since there are
no clients.

Or did you mean to respond to my other comment?  There can't be 0 active loading
clients before the "--active_clients_loading_" line is executed.  Otherwise,
you'd have underflow.

aiolos (Not reviewing)

On 2014/07/14 20:41:29, mmenke wrote: > On 2014/07/14 20:37:00, aiolos wrote: > > On 2014/07/14 ...

6 years, 5 months ago (2014-07-14 20:44:22 UTC) #58

On 2014/07/14 20:41:29, mmenke wrote:
> On 2014/07/14 20:37:00, aiolos wrote:
> > On 2014/07/14 19:38:12, mmenke wrote:
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > File content/browser/loader/resource_scheduler.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:815:
> > DCHECK(loading_active_clients_
> > > >= 0);
> > > On 2014/07/14 19:22:55, aiolos wrote:
> > > > On 2014/07/14 15:47:20, mmenke wrote:
> > > > > nit:  DCHECK_GE (x2)
> > > > 
> > > > Does this check even make sense? I thought size_t was an unsigned long?
> > > 
> > > You're right, it doesn't, so should get rid of them.
> > > 
> > > You may want to put a DCHECK_NE(0u, loading_active_clients_) here.  It's
> > > redundant with the CountLoadingActiveClients check, but I think it's
> > reasonable,
> > > though not necessary, to have a separate check for that particular case.
> > > 
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:837:
> > > client->LoadCoalescedRequests();
> > > On 2014/07/14 19:22:55, aiolos wrote:
> > > > On 2014/07/14 15:47:20, mmenke wrote:
> > > > > From before:
> > > > > 
> > > > > >> content/browser/loader/resource_scheduler.cc:838:
> > > > > >> client->LoadCoalescedRequests();
> > > > > >> Why load coalesced requests here instead of waiting for the timer?
> > > > > >> 
> > > > > >> And where is the timer for coalesced requests?
> > > > > >> 
> > > > > > In the next CL. :) We want to do it both on the timer and when the
> > network
> > > > is
> > > > > > brought up. We know the network is up when we send out another
> request.
> > > This
> > > > > may
> > > > > > be removed when we make the scheduler aware of when the network gets
> > > turned
> > > > > on,
> > > > > > but that is not in the scope of these CL's.
> > > > > 
> > > > > How do we know the network is up when an active client becomes
inactive,
> > or
> > > > > vice-versa?  I remain pretty skeptical of the utility of this.
> > > > 
> > > > I don't mind taking this out. The next CL was going to add logic that
> would
> > > load
> > > > coalesced requests on ACTIVE_AND_LOADING clients, but it would probably
be
> > > > cleaner just to wait until I hook up signals from the network turning
on.
> Do
> > > you
> > > > have a preference on that?
> > > 
> > > I'm fine with waiting.
> > > 
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > File content/browser/loader/resource_scheduler.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.h:118: void
> > > IncrementActiveClientsLoading();
> > > On 2014/07/14 19:22:56, aiolos wrote:
> > > > On 2014/07/14 15:47:20, mmenke wrote:
> > > > > These should be private.  Subclasses have access to their parents
> interal
> > > > > functions.
> > > > 
> > > > Done.
> > > > > 
> > > > > Same goes for should_coalesce() and should_throttle() (Clients can
> > actually
> > > > > access the fields directly, but I think that having them use accessor
> > > methods
> > > > is
> > > > > a cleaner).
> > > > 
> > > > These are actually used in the unittests as well as the Client. Plus,
they
> > are
> > > > going to be removed when throttling and coalescing are turned on by
> default
> > > > anyway. I think it makes more sense to leave them next to
> > > > SetThrottleOptionsForTesting until then.
> > > 
> > > SGTM.
> > > 
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > File content/browser/loader/resource_scheduler_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler_unittest.cc:925: }
> > > On 2014/07/14 16:48:21, aiolos wrote:
> > > > On 2014/07/14 15:47:21, mmenke wrote:
> > > > > Do we have any tests where requests are started as a result of the
> loading
> > > > state
> > > > > of another client changing?
> > > > 
> > > > FullVisibleLoadedCorrectlyUnthrottle checks that THROTTLED tabs become
> > > > UNTHROTTLED.
> > > > If you're asking about COALESCED, I thought I'd written one, but don't
see
> > it.
> > > > I'll fix that.
> > > 
> > > I was thinking about UNTROTTLED.  FullVisibleLoadedCorrectlyUnthrottle 
> > doesn't
> > > check that requests are actually unthrottled, just that the state of the
tab
> > > changes.  As-is, I'm not sure any test would fail if
> > > OnLoadingActiveClientsStateChanged just updated the publicly visible
> throttle
> > > state, without starting newly unthrottled requests.
> > 
> > I'll add request checks.
> > 
> > > 
> > > More generally, it's generally suggested that tests should test the public
> API
> > > of a class, rather than how the internals work.  It would be sufficiently
> > > painful to do that for all tests here that I'm not going to suggest you
> change
> > > all tests to do this, but think we should have at least one where we make
> sure
> > > unthrottling a client actually unthrottles pre-existing requests.
> > > 
> > I agree, there are enough state change/request types combinations that I
don't
> > believe it would be feasible to get complete coverage for all combinations.
> > 
> > >
> >
>
https://codereview.chromium.org/357583003/diff/220001/content/browser/loader/...
> > > File content/browser/loader/resource_scheduler.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/357583003/diff/220001/content/browser/loader/...
> > > content/browser/loader/resource_scheduler.cc:642:
> DCHECK(client_map_.empty());
> > > Maybe DCHECK_EQ(0u, loading_active_clients_)?
> > 
> > But there can be 0 active loading clients without there being an error. Is
> there
> > an overflow value you want me checking instead? It might just be best to let
> the
> > CountActiveClientsLoading check handle that case.
> 
> Right.  There must be 0 active loading clients on destruction, since there are
> no clients.
> 
> Or did you mean to respond to my other comment?  There can't be 0 active
loading
> clients before the "--active_clients_loading_" line is executed.  Otherwise,
> you'd have underflow.

I thought you meant to have it for the Increment as well as in Decrement. Having
it just for the latter makes sense though.

aiolos (Not reviewing)

On 2014/07/14 20:44:22, aiolos wrote: > On 2014/07/14 20:41:29, mmenke wrote: > > On 2014/07/14 ...

6 years, 5 months ago (2014-07-15 01:29:22 UTC) #59

On 2014/07/14 20:44:22, aiolos wrote:
> On 2014/07/14 20:41:29, mmenke wrote:
> > On 2014/07/14 20:37:00, aiolos wrote:
> > > On 2014/07/14 19:38:12, mmenke wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > > File content/browser/loader/resource_scheduler.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > > content/browser/loader/resource_scheduler.cc:815:
> > > DCHECK(loading_active_clients_
> > > > >= 0);
> > > > On 2014/07/14 19:22:55, aiolos wrote:
> > > > > On 2014/07/14 15:47:20, mmenke wrote:
> > > > > > nit:  DCHECK_GE (x2)
> > > > > 
> > > > > Does this check even make sense? I thought size_t was an unsigned
long?
> > > > 
> > > > You're right, it doesn't, so should get rid of them.
> > > > 
> > > > You may want to put a DCHECK_NE(0u, loading_active_clients_) here.  It's
> > > > redundant with the CountLoadingActiveClients check, but I think it's
> > > reasonable,
> > > > though not necessary, to have a separate check for that particular case.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > > content/browser/loader/resource_scheduler.cc:837:
> > > > client->LoadCoalescedRequests();
> > > > On 2014/07/14 19:22:55, aiolos wrote:
> > > > > On 2014/07/14 15:47:20, mmenke wrote:
> > > > > > From before:
> > > > > > 
> > > > > > >> content/browser/loader/resource_scheduler.cc:838:
> > > > > > >> client->LoadCoalescedRequests();
> > > > > > >> Why load coalesced requests here instead of waiting for the
timer?
> > > > > > >> 
> > > > > > >> And where is the timer for coalesced requests?
> > > > > > >> 
> > > > > > > In the next CL. :) We want to do it both on the timer and when the
> > > network
> > > > > is
> > > > > > > brought up. We know the network is up when we send out another
> > request.
> > > > This
> > > > > > may
> > > > > > > be removed when we make the scheduler aware of when the network
gets
> > > > turned
> > > > > > on,
> > > > > > > but that is not in the scope of these CL's.
> > > > > > 
> > > > > > How do we know the network is up when an active client becomes
> inactive,
> > > or
> > > > > > vice-versa?  I remain pretty skeptical of the utility of this.
> > > > > 
> > > > > I don't mind taking this out. The next CL was going to add logic that
> > would
> > > > load
> > > > > coalesced requests on ACTIVE_AND_LOADING clients, but it would
probably
> be
> > > > > cleaner just to wait until I hook up signals from the network turning
> on.
> > Do
> > > > you
> > > > > have a preference on that?
> > > > 
> > > > I'm fine with waiting.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > > File content/browser/loader/resource_scheduler.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > > content/browser/loader/resource_scheduler.h:118: void
> > > > IncrementActiveClientsLoading();
> > > > On 2014/07/14 19:22:56, aiolos wrote:
> > > > > On 2014/07/14 15:47:20, mmenke wrote:
> > > > > > These should be private.  Subclasses have access to their parents
> > interal
> > > > > > functions.
> > > > > 
> > > > > Done.
> > > > > > 
> > > > > > Same goes for should_coalesce() and should_throttle() (Clients can
> > > actually
> > > > > > access the fields directly, but I think that having them use
accessor
> > > > methods
> > > > > is
> > > > > > a cleaner).
> > > > > 
> > > > > These are actually used in the unittests as well as the Client. Plus,
> they
> > > are
> > > > > going to be removed when throttling and coalescing are turned on by
> > default
> > > > > anyway. I think it makes more sense to leave them next to
> > > > > SetThrottleOptionsForTesting until then.
> > > > 
> > > > SGTM.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > > File content/browser/loader/resource_scheduler_unittest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/200001/content/browser/loader/...
> > > > content/browser/loader/resource_scheduler_unittest.cc:925: }
> > > > On 2014/07/14 16:48:21, aiolos wrote:
> > > > > On 2014/07/14 15:47:21, mmenke wrote:
> > > > > > Do we have any tests where requests are started as a result of the
> > loading
> > > > > state
> > > > > > of another client changing?
> > > > > 
> > > > > FullVisibleLoadedCorrectlyUnthrottle checks that THROTTLED tabs become
> > > > > UNTHROTTLED.
> > > > > If you're asking about COALESCED, I thought I'd written one, but don't
> see
> > > it.
> > > > > I'll fix that.
> > > > 
> > > > I was thinking about UNTROTTLED.  FullVisibleLoadedCorrectlyUnthrottle 
> > > doesn't
> > > > check that requests are actually unthrottled, just that the state of the
> tab
> > > > changes.  As-is, I'm not sure any test would fail if
> > > > OnLoadingActiveClientsStateChanged just updated the publicly visible
> > throttle
> > > > state, without starting newly unthrottled requests.
> > > 
> > > I'll add request checks.
> > > 
> > > > 
> > > > More generally, it's generally suggested that tests should test the
public
> > API
> > > > of a class, rather than how the internals work.  It would be
sufficiently
> > > > painful to do that for all tests here that I'm not going to suggest you
> > change
> > > > all tests to do this, but think we should have at least one where we
make
> > sure
> > > > unthrottling a client actually unthrottles pre-existing requests.
> > > > 
> > > I agree, there are enough state change/request types combinations that I
> don't
> > > believe it would be feasible to get complete coverage for all
combinations.
> > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/220001/content/browser/loader/...
> > > > File content/browser/loader/resource_scheduler.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/357583003/diff/220001/content/browser/loader/...
> > > > content/browser/loader/resource_scheduler.cc:642:
> > DCHECK(client_map_.empty());
> > > > Maybe DCHECK_EQ(0u, loading_active_clients_)?
> > > 
> > > But there can be 0 active loading clients without there being an error. Is
> > there
> > > an overflow value you want me checking instead? It might just be best to
let
> > the
> > > CountActiveClientsLoading check handle that case.
> > 
> > Right.  There must be 0 active loading clients on destruction, since there
are
> > no clients.
> > 
> > Or did you mean to respond to my other comment?  There can't be 0 active
> loading
> > clients before the "--active_clients_loading_" line is executed.  Otherwise,
> > you'd have underflow.
> 
> I thought you meant to have it for the Increment as well as in Decrement.
Having
> it just for the latter makes sense though.

All try bots succeeded (win_chromium_x64_rel and linux_chromium_gn_rel on the
second time due to a flaky perf test and a build failure, respectively, on the
first pass.)
Are there any other changes you want at this point? I think either I've changed
or gotten your approval not to change all comments so far.

mmenke

Sorry, didn't realize you'd updated the CL after set 12. LGTM!

6 years, 5 months ago (2014-07-15 14:55:15 UTC) #60

mmenke

On 2014/07/15 14:55:15, mmenke wrote: > Sorry, didn't realize you'd updated the CL after set ...

6 years, 5 months ago (2014-07-15 14:57:05 UTC) #61

aiolos (Not reviewing)

The CQ bit was checked by aiolos@chromium.org

6 years, 5 months ago (2014-07-15 15:24:13 UTC) #62

aiolos (Not reviewing)

On 2014/07/15 14:57:05, mmenke wrote: > On 2014/07/15 14:55:15, mmenke wrote: > > Sorry, didn't ...

6 years, 5 months ago (2014-07-15 15:25:49 UTC) #63

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aiolos@chromium.org/357583003/240001

6 years, 5 months ago (2014-07-15 15:26:23 UTC) #64

Alexander Potapenko

On 2014/07/15 22:30:59, I haz the power (commit-bot) wrote: > Change committed as 283259 This ...

6 years, 5 months ago (2014-07-16 15:10:34 UTC) #66

Message was sent while issue was closed.

On 2014/07/15 22:30:59, I haz the power (commit-bot) wrote:
> Change committed as 283259

This CL has broken the whole bunch of DrMemory bots. They're reporting errors
like the following one:

UNINITIALIZED READ: reading 0x033682f8-0x033682f9 1 byte(s)
# 0 content.dll!content::ResourceScheduler::Client::UpdateThrottleState       
[content\browser\loader\resource_scheduler.cc:313]
# 1 content.dll!content::ResourceScheduler::OnClientDeleted                   
[content\browser\loader\resource_scheduler.cc:723]
# 2 content.dll!content::ResourceDispatcherHostImpl::OnRenderViewHostDeleted  
[content\browser\loader\resource_dispatcher_host_impl.cc:1332]
# 3 content.dll!base::internal::Invoker<>::Run                                
[base\bind_internal.h:1383]
# 4 base.dll!base::MessageLoop::RunTask                                       
[base\message_loop\message_loop.cc:458]
# 5 base.dll!base::MessageLoop::DeferOrRunPendingTask                         
[base\message_loop\message_loop.cc:470]
# 6 base.dll!base::MessageLoop::DoWork                                        
[base\message_loop\message_loop.cc:584]
# 7 base.dll!base::MessagePumpForIO::DoRunLoop                                
[base\message_loop\message_pump_win.cc:498]
# 8 base.dll!base::MessagePumpWin::Run                                        
[base\message_loop\message_pump_win.h:47]
# 9 base.dll!base::MessageLoop::RunHandler                                    
[base\message_loop\message_loop.cc:408]
#10 base.dll!base::Thread::Run                                                
[base\threading\thread.cc:174]
#11 content.dll!content::BrowserThreadImpl::IOThreadRun                       
[content\browser\browser_thread_impl.cc:217]
#12 content.dll!content::BrowserThreadImpl::Run                               
[content\browser\browser_thread_impl.cc:252]
#13 base.dll!base::Thread::ThreadMain                                         
[base\threading\thread.cc:228]
#14 base.dll!base::`anonymous namespace'::ThreadFunc                          
[base\threading\platform_thread_win.cc:78]
#15 KERNEL32.dll!BaseThreadInitThunk                                         
+0x11     (0x75cb338a <KERNEL32.dll+0x1338a>)
Note: @0:01:39.495 in thread 3924
Note: instruction: cmp    (%eax) $0x00
The report came from the `PluginTest.ResizeDuringPaint` test.

aiolos (Not reviewing)

On 2014/07/16 15:10:34, Alexander Potapenko wrote: > On 2014/07/15 22:30:59, I haz the power (commit-bot) ...

6 years, 5 months ago (2014-07-16 16:30:27 UTC) #67

Message was sent while issue was closed.

On 2014/07/16 15:10:34, Alexander Potapenko wrote:
> On 2014/07/15 22:30:59, I haz the power (commit-bot) wrote:
> > Change committed as 283259
> 
> This CL has broken the whole bunch of DrMemory bots. They're reporting errors
> like the following one:
> 
> UNINITIALIZED READ: reading 0x033682f8-0x033682f9 1 byte(s)
> # 0 content.dll!content::ResourceScheduler::Client::UpdateThrottleState       
> [content\browser\loader\resource_scheduler.cc:313]
> # 1 content.dll!content::ResourceScheduler::OnClientDeleted                   
> [content\browser\loader\resource_scheduler.cc:723]
> # 2 content.dll!content::ResourceDispatcherHostImpl::OnRenderViewHostDeleted  
> [content\browser\loader\resource_dispatcher_host_impl.cc:1332]
> # 3 content.dll!base::internal::Invoker<>::Run                                
> [base\bind_internal.h:1383]
> # 4 base.dll!base::MessageLoop::RunTask                                       
> [base\message_loop\message_loop.cc:458]
> # 5 base.dll!base::MessageLoop::DeferOrRunPendingTask                         
> [base\message_loop\message_loop.cc:470]
> # 6 base.dll!base::MessageLoop::DoWork                                        
> [base\message_loop\message_loop.cc:584]
> # 7 base.dll!base::MessagePumpForIO::DoRunLoop                                
> [base\message_loop\message_pump_win.cc:498]
> # 8 base.dll!base::MessagePumpWin::Run                                        
> [base\message_loop\message_pump_win.h:47]
> # 9 base.dll!base::MessageLoop::RunHandler                                    
> [base\message_loop\message_loop.cc:408]
> #10 base.dll!base::Thread::Run                                                
> [base\threading\thread.cc:174]
> #11 content.dll!content::BrowserThreadImpl::IOThreadRun                       
> [content\browser\browser_thread_impl.cc:217]
> #12 content.dll!content::BrowserThreadImpl::Run                               
> [content\browser\browser_thread_impl.cc:252]
> #13 base.dll!base::Thread::ThreadMain                                         
> [base\threading\thread.cc:228]
> #14 base.dll!base::`anonymous namespace'::ThreadFunc                          
> [base\threading\platform_thread_win.cc:78]
> #15 KERNEL32.dll!BaseThreadInitThunk                                         
> +0x11     (0x75cb338a <KERNEL32.dll+0x1338a>)
> Note: @0:01:39.495 in thread 3924
> Note: instruction: cmp    (%eax) $0x00
> The report came from the `PluginTest.ResizeDuringPaint` test.

Thanks, found the cause. It's an easy fix, I'm changing the tests so we would
have caught this.

aiolos (Not reviewing)

On 2014/07/16 16:30:27, aiolos wrote: > On 2014/07/16 15:10:34, Alexander Potapenko wrote: > > On ...

6 years, 5 months ago (2014-07-16 17:22:10 UTC) #68

Message was sent while issue was closed.

On 2014/07/16 16:30:27, aiolos wrote:
> On 2014/07/16 15:10:34, Alexander Potapenko wrote:
> > On 2014/07/15 22:30:59, I haz the power (commit-bot) wrote:
> > > Change committed as 283259
> > 
> > This CL has broken the whole bunch of DrMemory bots. They're reporting
errors
> > like the following one:
> > 
> > UNINITIALIZED READ: reading 0x033682f8-0x033682f9 1 byte(s)
> > # 0 content.dll!content::ResourceScheduler::Client::UpdateThrottleState     
 
> > [content\browser\loader\resource_scheduler.cc:313]
> > # 1 content.dll!content::ResourceScheduler::OnClientDeleted                 
 
> > [content\browser\loader\resource_scheduler.cc:723]
> > # 2 content.dll!content::ResourceDispatcherHostImpl::OnRenderViewHostDeleted
 
> > [content\browser\loader\resource_dispatcher_host_impl.cc:1332]
> > # 3 content.dll!base::internal::Invoker<>::Run                              
 
> > [base\bind_internal.h:1383]
> > # 4 base.dll!base::MessageLoop::RunTask                                     
 
> > [base\message_loop\message_loop.cc:458]
> > # 5 base.dll!base::MessageLoop::DeferOrRunPendingTask                       
 
> > [base\message_loop\message_loop.cc:470]
> > # 6 base.dll!base::MessageLoop::DoWork                                      
 
> > [base\message_loop\message_loop.cc:584]
> > # 7 base.dll!base::MessagePumpForIO::DoRunLoop                              
 
> > [base\message_loop\message_pump_win.cc:498]
> > # 8 base.dll!base::MessagePumpWin::Run                                      
 
> > [base\message_loop\message_pump_win.h:47]
> > # 9 base.dll!base::MessageLoop::RunHandler                                  
 
> > [base\message_loop\message_loop.cc:408]
> > #10 base.dll!base::Thread::Run                                              
 
> > [base\threading\thread.cc:174]
> > #11 content.dll!content::BrowserThreadImpl::IOThreadRun                     
 
> > [content\browser\browser_thread_impl.cc:217]
> > #12 content.dll!content::BrowserThreadImpl::Run                             
 
> > [content\browser\browser_thread_impl.cc:252]
> > #13 base.dll!base::Thread::ThreadMain                                       
 
> > [base\threading\thread.cc:228]
> > #14 base.dll!base::`anonymous namespace'::ThreadFunc                        
 
> > [base\threading\platform_thread_win.cc:78]
> > #15 KERNEL32.dll!BaseThreadInitThunk                                        

> > +0x11     (0x75cb338a <KERNEL32.dll+0x1338a>)
> > Note: @0:01:39.495 in thread 3924
> > Note: instruction: cmp    (%eax) $0x00
> > The report came from the `PluginTest.ResizeDuringPaint` test.
> 
> Thanks, found the cause. It's an easy fix, I'm changing the tests so we would
> have caught this.

Is there a bug number for that? Change here:
https://codereview.chromium.org/391393002/

mmenke

6 years, 5 months ago (2014-07-16 17:38:33 UTC) #69

Message was sent while issue was closed.

On 2014/07/16 17:22:10, aiolos wrote:
> On 2014/07/16 16:30:27, aiolos wrote:
> > On 2014/07/16 15:10:34, Alexander Potapenko wrote:
> > > On 2014/07/15 22:30:59, I haz the power (commit-bot) wrote:
> > > > Change committed as 283259
> > > 
> > > This CL has broken the whole bunch of DrMemory bots. They're reporting
> errors
> > > like the following one:
> > > 
> > > UNINITIALIZED READ: reading 0x033682f8-0x033682f9 1 byte(s)
> > > # 0 content.dll!content::ResourceScheduler::Client::UpdateThrottleState   
 
>  
> > > [content\browser\loader\resource_scheduler.cc:313]
> > > # 1 content.dll!content::ResourceScheduler::OnClientDeleted               
 
>  
> > > [content\browser\loader\resource_scheduler.cc:723]
> > > # 2
content.dll!content::ResourceDispatcherHostImpl::OnRenderViewHostDeleted
>  
> > > [content\browser\loader\resource_dispatcher_host_impl.cc:1332]
> > > # 3 content.dll!base::internal::Invoker<>::Run                            
 
>  
> > > [base\bind_internal.h:1383]
> > > # 4 base.dll!base::MessageLoop::RunTask                                   
 
>  
> > > [base\message_loop\message_loop.cc:458]
> > > # 5 base.dll!base::MessageLoop::DeferOrRunPendingTask                     
 
>  
> > > [base\message_loop\message_loop.cc:470]
> > > # 6 base.dll!base::MessageLoop::DoWork                                    
 
>  
> > > [base\message_loop\message_loop.cc:584]
> > > # 7 base.dll!base::MessagePumpForIO::DoRunLoop                            
 
>  
> > > [base\message_loop\message_pump_win.cc:498]
> > > # 8 base.dll!base::MessagePumpWin::Run                                    
 
>  
> > > [base\message_loop\message_pump_win.h:47]
> > > # 9 base.dll!base::MessageLoop::RunHandler                                
 
>  
> > > [base\message_loop\message_loop.cc:408]
> > > #10 base.dll!base::Thread::Run                                            
 
>  
> > > [base\threading\thread.cc:174]
> > > #11 content.dll!content::BrowserThreadImpl::IOThreadRun                   
 
>  
> > > [content\browser\browser_thread_impl.cc:217]
> > > #12 content.dll!content::BrowserThreadImpl::Run                           
 
>  
> > > [content\browser\browser_thread_impl.cc:252]
> > > #13 base.dll!base::Thread::ThreadMain                                     
 
>  
> > > [base\threading\thread.cc:228]
> > > #14 base.dll!base::`anonymous namespace'::ThreadFunc                      
 
>  
> > > [base\threading\platform_thread_win.cc:78]
> > > #15 KERNEL32.dll!BaseThreadInitThunk                                      
 
> 
> > > +0x11     (0x75cb338a <KERNEL32.dll+0x1338a>)
> > > Note: @0:01:39.495 in thread 3924
> > > Note: instruction: cmp    (%eax) $0x00
> > > The report came from the `PluginTest.ResizeDuringPaint` test.
> > 
> > Thanks, found the cause. It's an easy fix, I'm changing the tests so we
would
> > have caught this.
> 
> Is there a bug number for that? Change here:
> https://codereview.chromium.org/391393002/

Bug looks to be http://crbug.com/394390

Issue 357583003: Adding observable throttle logic without the signals. Defaults to current behavior. (Closed)

Description

Patch Set 1 #

Patch Set 2 : CL 1: Client specific changes. #

Patch Set 3 : nits and such #

Patch Set 4 : missed a nit #

Patch Set 5 : header file fix #

Patch Set 6 : Try 2 on uploading the nits #

Patch Set 7 : Testing #

Patch Set 8 : nits plus more testing #

Patch Set 9 : Active Loadinf refactor, plus updated testing #

Patch Set 10 : More tests and code changes #

Patch Set 11 : New test, requested changes. #

Patch Set 12 : Next review iteration #

Patch Set 13 : Update FullVisibleLoadedCorrectlyUnthrottle test #

Patch Set 14 : Underflow check added. #

Messages