Make content::ThrottlingURLLoader take a task runner and more efficient.

Make content::ThrottlingURLLoader take a task runner and more efficient.

- The task runner is used to dispatch URLLoaderClient messages.
- Previously it had got many members to cache params when operations were
  deferred. This CL groups those params into structs and only creates instances
  when necessary.

BUG=729769, 715673
Review-Url: https://codereview.chromium.org/2926693002
Cr-Commit-Position: refs/heads/master@{#477687}
Committed: https://chromium.googlesource.com/chromium/src/+/e2e435ef7d38319633f7a01d051f2760af8806c7

[yzshen1, 2017-06-06 23:36:28]

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

[commit-bot: I haz the power, 2017-06-06 23:37:10]

Dry run: CQ is trying da patch.

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

[yzshen1, 2017-06-06 23:37:28]

yzshen@chromium.org changed reviewers:
+ jam@chromium.org

[yzshen1, 2017-06-06 23:37:29]

Hi, John.

Would you please take a look? Thanks!

[commit-bot: I haz the power, 2017-06-07 01:01:19]

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

[commit-bot: I haz the power, 2017-06-07 01:01:20]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-07 01:24:21]

On 2017/06/06 23:37:29, yzshen1 wrote:
> Hi, John.
> 
> Would you please take a look? Thanks!

I'm curious, did you repro the slowdown locally or are these fixes by
inspection? I ask because moving member variables to structs makes the code
slightly less readable, so I'm wondering if you can first try the task runner
change and if it's enough on the perf bots then no need to make further changes?

lgtm, i leave it up to you

[yzshen1, 2017-06-07 16:57:37]

On 2017/06/07 01:24:21, jam wrote:
> On 2017/06/06 23:37:29, yzshen1 wrote:
> > Hi, John.
> > 
> > Would you please take a look? Thanks!
> 
> I'm curious, did you repro the slowdown locally or are these fixes by
> inspection? I ask because moving member variables to structs makes the code
> slightly less readable, so I'm wondering if you can first try the task runner
> change and if it's enough on the perf bots then no need to make further
changes?
> 
> lgtm, i leave it up to you

I tested on Windows (which is one of the platforms showing the slowdown) and
confirmed that this fix restored the timeToFirstMeaningfulPaint.
I didn't test "specifying task runner" and "using structs" separately though.

If you don't mind, I would like to keep the struct change. Because they are not
used in the majority case, it will reduce memory and also initialization cost.
One example is that ResourceResponseHead has exactly 40 fields, quite some of
which are classes/structs themselves. It should be more efficient to only create
such instances when necessary.

[yzshen1, 2017-06-07 16:59:29]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2017-06-07 16:59:48]

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-06-07 17:05:16]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496854769080520,
"parent_rev": "aecefa0882b4fe9dc4bf01e271991a289b7f99a7", "commit_rev":
"e2e435ef7d38319633f7a01d051f2760af8806c7"}

[commit-bot: I haz the power, 2017-06-07 17:05:24]

Description was changed from

==========
Make content::ThrottlingURLLoader take a task runner and more efficient.

- The task runner is used to dispatch URLLoaderClient messages.
- Previously it had got many members to cache params when operations were
  deferred. This CL groups those params into structs and only creates instances
  when necessary.

BUG=729769, 715673
==========

to

==========
Make content::ThrottlingURLLoader take a task runner and more efficient.

- The task runner is used to dispatch URLLoaderClient messages.
- Previously it had got many members to cache params when operations were
  deferred. This CL groups those params into structs and only creates instances
  when necessary.

BUG=729769, 715673

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

[commit-bot: I haz the power, 2017-06-07 17:05:26]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e2e435ef7d38319633f7a01d051f...

[jam, 2017-06-07 17:10:33]

On 2017/06/07 16:57:37, yzshen1 wrote:
> On 2017/06/07 01:24:21, jam wrote:
> > On 2017/06/06 23:37:29, yzshen1 wrote:
> > > Hi, John.
> > > 
> > > Would you please take a look? Thanks!
> > 
> > I'm curious, did you repro the slowdown locally or are these fixes by
> > inspection? I ask because moving member variables to structs makes the code
> > slightly less readable, so I'm wondering if you can first try the task
runner
> > change and if it's enough on the perf bots then no need to make further
> changes?
> > 
> > lgtm, i leave it up to you
> 
> I tested on Windows (which is one of the platforms showing the slowdown) and
> confirmed that this fix restored the timeToFirstMeaningfulPaint.
> I didn't test "specifying task runner" and "using structs" separately though.
> 
> If you don't mind, I would like to keep the struct change. Because they are
not
> used in the majority case, it will reduce memory and also initialization cost.
> One example is that ResourceResponseHead has exactly 40 fields, quite some of
> which are classes/structs themselves. It should be more efficient to only
create
> such instances when necessary.

ah, sounds good then!