Do not schedule to load pending startable request on RemoveRequest

Do not schedule to load pending startable request on RemoveRequest

This may be causing too many tasks to be posted in some situations. To
ease understanding, this patch also adds two trace events to the
relevant methods.

BUG=690290
Review-Url: https://codereview.chromium.org/2704243003
Cr-Commit-Position: refs/heads/master@{#453459}
Committed: https://chromium.googlesource.com/chromium/src/+/a17c6984f4b9369e8ce061018d31e4ecf64f7fdf

[Charlie Harrison, 2017-02-20 18:10:01]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 18:10:15]

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

[commit-bot: I haz the power, 2017-02-20 19:29:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 19:29:45]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-02-21 15:03:03]

Description was changed from

==========
Do not schedule to load pending startable request on RemoveRequest

BUG=
==========

to

==========
Do not schedule to load pending startable request on RemoveRequest

BUG=690290
==========

[Charlie Harrison, 2017-02-21 15:07:17]

Description was changed from

==========
Do not schedule to load pending startable request on RemoveRequest

BUG=690290
==========

to

==========
Do not schedule to load pending startable request on RemoveRequest

This may be causing too many tasks to be posted in some situations. To
ease understanding, this patch also adds two trace events to the
relevant methods.

BUG=690290
==========

[Charlie Harrison, 2017-02-21 15:08:00]

csharrison@chromium.org changed reviewers:
+ kinuko@chromium.org

[Charlie Harrison, 2017-02-21 15:08:01]

kinuko: PTAL?

[kinuko, 2017-02-22 01:55:31]

Could help me understand-- so RemoveRequest is called when request's done or
when it's cancelled, and in the benchmark case you're guessing it's caused by
storm of post task's because all requests are short-lived in the benchmark, is
that right?  The concurrency in the benchmark was 100 (the SW in the test
doesn't make network request), I worry if we might have similar regression for
ReprioritizeRequest- what's your thought on it?  I wonder if we want to remove
both changes but add trace points possibly with some UMAs.

[Charlie Harrison, 2017-02-22 02:05:25]

On 2017/02/22 01:55:31, kinuko wrote:
> Could help me understand-- so RemoveRequest is called when request's done or
> when it's cancelled, and in the benchmark case you're guessing it's caused by
> storm of post task's because all requests are short-lived in the benchmark, is
> that right?  The concurrency in the benchmark was 100 (the SW in the test
> doesn't make network request), I worry if we might have similar regression for
> ReprioritizeRequest- what's your thought on it?  I wonder if we want to remove
> both changes but add trace points possibly with some UMAs.

Yes your understanding is correct (as I can see it). I think we could be
experiencing this in ReprioritizeRequest, but it depends on how likely it is
called. Since RemoveRequest is called whenever a request ends, we could end up
(worst case) calling it for every request: O(n) tasks for n requests.

ReprioritizeRequest follows a different pattern, where we just send a batch of M
of them to the browser. This pattern seems ideal for the post task yielding, as
it should only cause a single extra task to be run. WDYT? I am happy to revert
both changes if you'd like.

[kinuko, 2017-02-22 03:01:53]

On 2017/02/22 02:05:25, Charlie Harrison wrote:
> On 2017/02/22 01:55:31, kinuko wrote:
> > Could help me understand-- so RemoveRequest is called when request's done or
> > when it's cancelled, and in the benchmark case you're guessing it's caused
by
> > storm of post task's because all requests are short-lived in the benchmark,
is
> > that right?  The concurrency in the benchmark was 100 (the SW in the test
> > doesn't make network request), I worry if we might have similar regression
for
> > ReprioritizeRequest- what's your thought on it?  I wonder if we want to
remove
> > both changes but add trace points possibly with some UMAs.
> 
> Yes your understanding is correct (as I can see it). I think we could be
> experiencing this in ReprioritizeRequest, but it depends on how likely it is
> called. Since RemoveRequest is called whenever a request ends, we could end up
> (worst case) calling it for every request: O(n) tasks for n requests.
> 
> ReprioritizeRequest follows a different pattern, where we just send a batch of
M
> of them to the browser. This pattern seems ideal for the post task yielding,
as
> it should only cause a single extra task to be run. WDYT? I am happy to revert
> both changes if you'd like.

Let me make extra sure: have you actually seen that's what's happening for
ReprioritizeRequest cases?  I might also want to have a UMA or something that
indicates how many pending tasks are batched.

[kinuko, 2017-02-22 05:32:40]

On 2017/02/22 03:01:53, kinuko wrote:
> On 2017/02/22 02:05:25, Charlie Harrison wrote:
> > On 2017/02/22 01:55:31, kinuko wrote:
> > > Could help me understand-- so RemoveRequest is called when request's done
or
> > > when it's cancelled, and in the benchmark case you're guessing it's caused
> by
> > > storm of post task's because all requests are short-lived in the
benchmark,
> is
> > > that right?  The concurrency in the benchmark was 100 (the SW in the test
> > > doesn't make network request), I worry if we might have similar regression
> for
> > > ReprioritizeRequest- what's your thought on it?  I wonder if we want to
> remove
> > > both changes but add trace points possibly with some UMAs.
> > 
> > Yes your understanding is correct (as I can see it). I think we could be
> > experiencing this in ReprioritizeRequest, but it depends on how likely it is
> > called. Since RemoveRequest is called whenever a request ends, we could end
up
> > (worst case) calling it for every request: O(n) tasks for n requests.
> > 
> > ReprioritizeRequest follows a different pattern, where we just send a batch
of
> M
> > of them to the browser. This pattern seems ideal for the post task yielding,
> as
> > it should only cause a single extra task to be run. WDYT? I am happy to
revert
> > both changes if you'd like.
> 
> Let me make extra sure: have you actually seen that's what's happening for
> ReprioritizeRequest cases?  I might also want to have a UMA or something that
> indicates how many pending tasks are batched.

Btw since this is to fix regression I can give you earlier lgtm if you're sure
about these^^.  We can add more UMAs later too

[Charlie Harrison, 2017-02-22 13:03:50]

On 2017/02/22 05:32:40, kinuko wrote:
> On 2017/02/22 03:01:53, kinuko wrote:
> > On 2017/02/22 02:05:25, Charlie Harrison wrote:
> > > On 2017/02/22 01:55:31, kinuko wrote:
> > > > Could help me understand-- so RemoveRequest is called when request's
done
> or
> > > > when it's cancelled, and in the benchmark case you're guessing it's
caused
> > by
> > > > storm of post task's because all requests are short-lived in the
> benchmark,
> > is
> > > > that right?  The concurrency in the benchmark was 100 (the SW in the
test
> > > > doesn't make network request), I worry if we might have similar
regression
> > for
> > > > ReprioritizeRequest- what's your thought on it?  I wonder if we want to
> > remove
> > > > both changes but add trace points possibly with some UMAs.
> > > 
> > > Yes your understanding is correct (as I can see it). I think we could be
> > > experiencing this in ReprioritizeRequest, but it depends on how likely it
is
> > > called. Since RemoveRequest is called whenever a request ends, we could
end
> up
> > > (worst case) calling it for every request: O(n) tasks for n requests.
> > > 
> > > ReprioritizeRequest follows a different pattern, where we just send a
batch
> of
> > M
> > > of them to the browser. This pattern seems ideal for the post task
yielding,
> > as
> > > it should only cause a single extra task to be run. WDYT? I am happy to
> revert
> > > both changes if you'd like.
> > 
> > Let me make extra sure: have you actually seen that's what's happening for
> > ReprioritizeRequest cases?  I might also want to have a UMA or something
that
> > indicates how many pending tasks are batched.
> 
> Btw since this is to fix regression I can give you earlier lgtm if you're sure
> about these^^.  We can add more UMAs later too

I've updated the linked bug with some local evidence of this happening (on Mac).

[kinuko, 2017-02-23 02:50:30]

On 2017/02/22 13:03:50, Charlie Harrison wrote:
> On 2017/02/22 05:32:40, kinuko wrote:
> > On 2017/02/22 03:01:53, kinuko wrote:
> > > On 2017/02/22 02:05:25, Charlie Harrison wrote:
> > > > On 2017/02/22 01:55:31, kinuko wrote:
> > > > > Could help me understand-- so RemoveRequest is called when request's
> done
> > or
> > > > > when it's cancelled, and in the benchmark case you're guessing it's
> caused
> > > by
> > > > > storm of post task's because all requests are short-lived in the
> > benchmark,
> > > is
> > > > > that right?  The concurrency in the benchmark was 100 (the SW in the
> test
> > > > > doesn't make network request), I worry if we might have similar
> regression
> > > for
> > > > > ReprioritizeRequest- what's your thought on it?  I wonder if we want
to
> > > remove
> > > > > both changes but add trace points possibly with some UMAs.
> > > > 
> > > > Yes your understanding is correct (as I can see it). I think we could be
> > > > experiencing this in ReprioritizeRequest, but it depends on how likely
it
> is
> > > > called. Since RemoveRequest is called whenever a request ends, we could
> end
> > up
> > > > (worst case) calling it for every request: O(n) tasks for n requests.
> > > > 
> > > > ReprioritizeRequest follows a different pattern, where we just send a
> batch
> > of
> > > M
> > > > of them to the browser. This pattern seems ideal for the post task
> yielding,
> > > as
> > > > it should only cause a single extra task to be run. WDYT? I am happy to
> > revert
> > > > both changes if you'd like.
> > > 
> > > Let me make extra sure: have you actually seen that's what's happening for
> > > ReprioritizeRequest cases?  I might also want to have a UMA or something
> that
> > > indicates how many pending tasks are batched.
> > 
> > Btw since this is to fix regression I can give you earlier lgtm if you're
sure
> > about these^^.  We can add more UMAs later too
> 
> I've updated the linked bug with some local evidence of this happening (on
Mac).

Thanks.  As I replied on the bug I also took some local histograms with custom
ToT as the example trace you attached was for artificial workload.  To roughly
summarize the decision to only schedule tasks in reprioritization cases looks
good (they don't come often, much less than RemoveRequest, and when they can I
was able to observe modest effect of coalescing).  I still think it might be
good to have some histograms, but up to you.

[Charlie Harrison, 2017-02-23 17:32:06]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 17:32:27]

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

[Charlie Harrison, 2017-02-23 17:40:27]

Hey kinuko@, this has changed a bit since you last reviewed, since I added a
histogram, PTAL?

[commit-bot: I haz the power, 2017-02-23 19:24:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 19:24:10]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-02-24 01:38:28]

(Sorry for high review latency)

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

https://codereview.chromium.org/2704243003/diff/40001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:778:
num_skipped_request_scans_for_delayed_start_ += pending_requests_.size();
Why do we add pending_requests_ here?

[kinuko, 2017-02-24 01:55:45]

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

https://codereview.chromium.org/2704243003/diff/40001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:778:
num_skipped_request_scans_for_delayed_start_ += pending_requests_.size();
On 2017/02/24 01:38:28, kinuko wrote:
> Why do we add pending_requests_ here?

I think this wants to get the numbers that we might have been needed to
calculate if we didn't delay?  But if we called LoadAnyStartablePendingRequests
the # of pending_requests_ would have been changed, which feels a bit unfair?

[Charlie Harrison, 2017-02-24 02:02:00]

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

https://codereview.chromium.org/2704243003/diff/40001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:778:
num_skipped_request_scans_for_delayed_start_ += pending_requests_.size();
On 2017/02/24 01:55:45, kinuko wrote:
> On 2017/02/24 01:38:28, kinuko wrote:
> > Why do we add pending_requests_ here?
> 
> I think this wants to get the numbers that we might have been needed to
> calculate if we didn't delay?  But if we called
LoadAnyStartablePendingRequests
> the # of pending_requests_ would have been changed, which feels a bit unfair?

Hm... I'm not sure what better signal to use. Note that it is already an upper
bound, because we won't necessarily scan through all of these in
LoadAnyStartablePendingRequests.

[kinuko, 2017-02-24 02:47:24]

On 2017/02/24 02:02:00, Charlie Harrison wrote:
>
https://codereview.chromium.org/2704243003/diff/40001/content/browser/loader/...
> File content/browser/loader/resource_scheduler.cc (right):
> 
>
https://codereview.chromium.org/2704243003/diff/40001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:778:
> num_skipped_request_scans_for_delayed_start_ += pending_requests_.size();
> On 2017/02/24 01:55:45, kinuko wrote:
> > On 2017/02/24 01:38:28, kinuko wrote:
> > > Why do we add pending_requests_ here?
> > 
> > I think this wants to get the numbers that we might have been needed to
> > calculate if we didn't delay?  But if we called
> LoadAnyStartablePendingRequests
> > the # of pending_requests_ would have been changed, which feels a bit
unfair?
> 
> Hm... I'm not sure what better signal to use. Note that it is already an upper
> bound, because we won't necessarily scan through all of these in
> LoadAnyStartablePendingRequests.

Would it make sense to just log num_skipped_request_scans_for_delayed_start_ (in
different names) so that we can see how many LoadAnyStartablePendingRequests
have been batched?

I feel pending_requests_.size() could be separately logged if we wants to track
these.

[Charlie Harrison, 2017-02-24 21:25:09]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[Charlie Harrison, 2017-02-24 21:25:54]

I've changed it to just count the number of skipped scans, PTAL?

[commit-bot: I haz the power, 2017-02-24 21:26:18]

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

[commit-bot: I haz the power, 2017-02-24 22:56:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 22:57:00]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-02-25 02:28:47]

Thanks! lgtm

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

https://codereview.chromium.org/2704243003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:186: size_t size() { return
queue_.size(); }
Not used?

[Charlie Harrison, 2017-02-26 22:37:52]

csharrison@chromium.org changed reviewers:
+ holte@chromium.org

[Charlie Harrison, 2017-02-26 22:37:53]

Thanks kinuko.

+holte for a new histogram.

[Steven Holte, 2017-02-28 00:37:20]

lgtm

[Charlie Harrison, 2017-02-28 00:39:32]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-02-28 00:39:32]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2704243003/#ps80001
(title: "remove unused code")

[commit-bot: I haz the power, 2017-02-28 00:40:00]

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

[commit-bot: I haz the power, 2017-02-28 02:18:19]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488242372213780,
"parent_rev": "a84f21653890b621907da8446a217875d2bf51c1", "commit_rev":
"a17c6984f4b9369e8ce061018d31e4ecf64f7fdf"}

[commit-bot: I haz the power, 2017-02-28 02:19:06]

Description was changed from

==========
Do not schedule to load pending startable request on RemoveRequest

This may be causing too many tasks to be posted in some situations. To
ease understanding, this patch also adds two trace events to the
relevant methods.

BUG=690290
==========

to

==========
Do not schedule to load pending startable request on RemoveRequest

This may be causing too many tasks to be posted in some situations. To
ease understanding, this patch also adds two trace events to the
relevant methods.

BUG=690290

Review-Url: https://codereview.chromium.org/2704243003
Cr-Commit-Position: refs/heads/master@{#453459}
Committed:
https://chromium.googlesource.com/chromium/src/+/a17c6984f4b9369e8ce061018d31...
==========

[commit-bot: I haz the power, 2017-02-28 02:19:08]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a17c6984f4b9369e8ce061018d31...