Remove the file thread dependency from FileNetLogObserver.

Remove the file thread dependency from FileNetLogObserver.

Instead use an internally created sequenced task runer.

BUG=689520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2966283002
Cr-Commit-Position: refs/heads/master@{#485105}
Committed: https://chromium.googlesource.com/chromium/src/+/fc4bcbaa15506e2bebffddc2ad89cb0f6bfe20c5

[eroman, 2017-07-05 19:25:54]

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

[commit-bot: I haz the power, 2017-07-05 19:26:14]

Dry run: CQ is trying da patch.

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

[eroman, 2017-07-05 19:28:43]

Description was changed from

==========
Remove the file thread dependency from FileNetLogObserver.

Instead use an internally created sequenced task runer.

BUG=689520
==========

to

==========
Remove the file thread dependency from FileNetLogObserver.

Instead use an internally created sequenced task runer.

BUG=689520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[eroman, 2017-07-05 19:28:48]

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

[commit-bot: I haz the power, 2017-07-05 19:29:08]

Dry run: CQ is trying da patch.

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

[eroman, 2017-07-05 19:55:58]

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

[commit-bot: I haz the power, 2017-07-05 19:56:17]

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-07-05 20:28:56]

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

[commit-bot: I haz the power, 2017-07-05 20:28:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[eroman, 2017-07-05 21:01:42]

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

[commit-bot: I haz the power, 2017-07-05 21:02:09]

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-07-05 21:31:14]

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

[commit-bot: I haz the power, 2017-07-05 21:31:15]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[eroman, 2017-07-05 21:55:40]

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

[commit-bot: I haz the power, 2017-07-05 21:56:12]

Dry run: CQ is trying da patch.

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

[eroman, 2017-07-05 22:37:01]

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

[commit-bot: I haz the power, 2017-07-05 22:37:37]

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-07-05 23:27:08]

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

[commit-bot: I haz the power, 2017-07-05 23:27:10]

Dry run: Try jobs failed on following builders:
  android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[eroman, 2017-07-05 23:34:28]

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

[commit-bot: I haz the power, 2017-07-05 23:34:52]

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-07-06 00:54:03]

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

[commit-bot: I haz the power, 2017-07-06 00:54:04]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-07-06 01:00:51]

eroman@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2017-07-06 01:50:01]

Just a quick response, haven't thought about other options.

https://codereview.chromium.org/2966283002/diff/170001/components/cronet/andr...
File components/cronet/android/cronet_library_loader.cc (right):

https://codereview.chromium.org/2966283002/diff/170001/components/cronet/andr...
components/cronet/android/cronet_library_loader.cc:71:
base::TaskScheduler::CreateAndStartWithDefaultParams("Cronet");
I don't think we want to create another thread in Cronet.

[mmenke, 2017-07-06 15:22:26]

https://codereview.chromium.org/2966283002/diff/170001/components/cronet/andr...
File components/cronet/android/cronet_library_loader.cc (right):

https://codereview.chromium.org/2966283002/diff/170001/components/cronet/andr...
components/cronet/android/cronet_library_loader.cc:71:
base::TaskScheduler::CreateAndStartWithDefaultParams("Cronet");
On 2017/07/06 01:50:01, mmenke wrote:
> I don't think we want to create another thread in Cronet.

I think we'll need to talk about the Cronet folks about what to do here, if we
want to use the task scheduler on Cronet at all - it looks like there's no way
to tell the task scheduler to only use 1 or 2 threads total.  Instead, we tell
it the max number of threads of each type, for 4 different thread types, meaning
a minimum of 4 threads (Though it can stop them when idle).

[eroman, 2017-07-06 17:05:45]

eroman@chromium.org changed reviewers:
+ mef@chromium.org

[eroman, 2017-07-06 17:05:46]

+mef for cronet quesiton.

Note that cronet is already using the TaskScheduler (on iOS):

https://cs.chromium.org/chromium/src/components/cronet/ios/cronet_environment...

I tried to match that initialization here, but for the Android code rather than
iOS.

[mef, 2017-07-06 19:53:01]

https://codereview.chromium.org/2966283002/diff/170001/components/cronet/andr...
File components/cronet/android/cronet_library_loader.cc (right):

https://codereview.chromium.org/2966283002/diff/170001/components/cronet/andr...
components/cronet/android/cronet_library_loader.cc:71:
base::TaskScheduler::CreateAndStartWithDefaultParams("Cronet");
On 2017/07/06 15:22:26, mmenke wrote:
> On 2017/07/06 01:50:01, mmenke wrote:
> > I don't think we want to create another thread in Cronet.
> 
> I think we'll need to talk about the Cronet folks about what to do here, if we
> want to use the task scheduler on Cronet at all - it looks like there's no way
> to tell the task scheduler to only use 1 or 2 threads total.  Instead, we tell
> it the max number of threads of each type, for 4 different thread types,
meaning
> a minimum of 4 threads (Though it can stop them when idle).

Good point. 
I didn't realize that base::TaskScheduler::CreateAndStartWithDefaultParams()
creates a bunch of threads. 
It seems like a way of the future though, right? 
Maybe Cronet should create task scheduler with custom params?

[eroman, 2017-07-06 22:01:38]

Thanks for the feedback.

I have removed the cronet initialization of TaskScheduler from this CL, and will
tackle that separately.

So we can continue discussing that topic on crbug.com/739896.

I will extract proposed cronet changes to an independent CL.

[mmenke, 2017-07-07 15:32:59]

LGTM

https://codereview.chromium.org/2966283002/diff/190001/net/log/file_net_log_o...
File net/log/file_net_log_observer.cc (right):

https://codereview.chromium.org/2966283002/diff/190001/net/log/file_net_log_o...
net/log/file_net_log_observer.cc:160: const
scoped_refptr<base::SequencedTaskRunner>& task_runner);
I think scoped_refptr<base::SequencedTaskRunner> + std::move is preferred, if
you're holding onto a pointer to it?  Same goes for other instances of this.

[eroman, 2017-07-07 21:03:08]

https://codereview.chromium.org/2966283002/diff/190001/net/log/file_net_log_o...
File net/log/file_net_log_observer.cc (right):

https://codereview.chromium.org/2966283002/diff/190001/net/log/file_net_log_o...
net/log/file_net_log_observer.cc:160: const
scoped_refptr<base::SequencedTaskRunner>& task_runner);
On 2017/07/07 15:32:59, mmenke wrote:
> I think scoped_refptr<base::SequencedTaskRunner> + std::move is preferred, if
> you're holding onto a pointer to it?  Same goes for other instances of this.

I switched to const scoped_refptr<> throughout, because it is easier to use
properly. Previously this function was pass by value, however failed to call
std::move() in the initializer.

The user of this ctor (CreateBounded()) ends up sharing a task runner between
BoundedFileWriter and FileNetLogObserver anyway,  so the code is cleaner to just
pass by reference and have each make a copy in initializer list.

The alternative would be to pass by value, and then std::move() internally in
the ctors. This gets the same number of addrefs (assuming you don't forget the
std::moves). Sure, you could remove the final addref in CreateBounded() by
std::move()ing the local task runner for the second ctor.... but that ugliness
isn't wortwhile.

[mmenke, 2017-07-07 21:05:22]

On 2017/07/07 21:03:08, eroman wrote:
>
https://codereview.chromium.org/2966283002/diff/190001/net/log/file_net_log_o...
> File net/log/file_net_log_observer.cc (right):
> 
>
https://codereview.chromium.org/2966283002/diff/190001/net/log/file_net_log_o...
> net/log/file_net_log_observer.cc:160: const
> scoped_refptr<base::SequencedTaskRunner>& task_runner);
> On 2017/07/07 15:32:59, mmenke wrote:
> > I think scoped_refptr<base::SequencedTaskRunner> + std::move is preferred,
if
> > you're holding onto a pointer to it?  Same goes for other instances of this.
> 
> I switched to const scoped_refptr<> throughout, because it is easier to use
> properly. Previously this function was pass by value, however failed to call
> std::move() in the initializer.
> 
> The user of this ctor (CreateBounded()) ends up sharing a task runner between
> BoundedFileWriter and FileNetLogObserver anyway,  so the code is cleaner to
just
> pass by reference and have each make a copy in initializer list.
> 
> The alternative would be to pass by value, and then std::move() internally in
> the ctors. This gets the same number of addrefs (assuming you don't forget the
> std::moves). Sure, you could remove the final addref in CreateBounded() by
> std::move()ing the local task runner for the second ctor.... but that ugliness
> isn't wortwhile.

I think the conclusion in
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/TlL1D-Djta0
was to prefer passing scoped_refptrs to const scoped_refptrs?

[mmenke, 2017-07-07 21:06:12]

On 2017/07/07 21:05:22, mmenke wrote:
> On 2017/07/07 21:03:08, eroman wrote:
> >
>
https://codereview.chromium.org/2966283002/diff/190001/net/log/file_net_log_o...
> > File net/log/file_net_log_observer.cc (right):
> > 
> >
>
https://codereview.chromium.org/2966283002/diff/190001/net/log/file_net_log_o...
> > net/log/file_net_log_observer.cc:160: const
> > scoped_refptr<base::SequencedTaskRunner>& task_runner);
> > On 2017/07/07 15:32:59, mmenke wrote:
> > > I think scoped_refptr<base::SequencedTaskRunner> + std::move is preferred,
> if
> > > you're holding onto a pointer to it?  Same goes for other instances of
this.
> > 
> > I switched to const scoped_refptr<> throughout, because it is easier to use
> > properly. Previously this function was pass by value, however failed to call
> > std::move() in the initializer.
> > 
> > The user of this ctor (CreateBounded()) ends up sharing a task runner
between
> > BoundedFileWriter and FileNetLogObserver anyway,  so the code is cleaner to
> just
> > pass by reference and have each make a copy in initializer list.
> > 
> > The alternative would be to pass by value, and then std::move() internally
in
> > the ctors. This gets the same number of addrefs (assuming you don't forget
the
> > std::moves). Sure, you could remove the final addref in CreateBounded() by
> > std::move()ing the local task runner for the second ctor.... but that
ugliness
> > isn't wortwhile.
> 
> I think the conclusion in
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/TlL1D-Djta0
> was to prefer passing scoped_refptrs to const scoped_refptrs?

"If the function (at least sometimes) takes a ref on a refcounted object,
declare the param as scoped_refptr<T>. The caller can decide whether it wishes
to transfer ownership (by calling std::move(t) when passing t) or retain its ref
(by simply passing t directly)."

(From
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md)

[eroman, 2017-07-07 21:20:20]

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

[eroman, 2017-07-07 21:20:35]

Done. Thanks for the links.

On Fri, Jul 7, 2017 at 2:06 PM, <mmenke@chromium.org> wrote:

> On 2017/07/07 21:05:22, mmenke wrote:
> > On 2017/07/07 21:03:08, eroman wrote:
> > >
> >
> https://codereview.chromium.org/2966283002/diff/190001/net/
> log/file_net_log_observer.cc
> > > File net/log/file_net_log_observer.cc (right):
> > >
> > >
> >
> https://codereview.chromium.org/2966283002/diff/190001/net/
> log/file_net_log_observer.cc#newcode160
> > > net/log/file_net_log_observer.cc:160: const
> > > scoped_refptr<base::SequencedTaskRunner>& task_runner);
> > > On 2017/07/07 15:32:59, mmenke wrote:
> > > > I think scoped_refptr<base::SequencedTaskRunner> + std::move is
> preferred,
> > if
> > > > you're holding onto a pointer to it? Same goes for other instances of
> this.
> > >
> > > I switched to const scoped_refptr<> throughout, because it is easier
> to use
> > > properly. Previously this function was pass by value, however failed
> to call
> > > std::move() in the initializer.
> > >
> > > The user of this ctor (CreateBounded()) ends up sharing a task runner
> between
> > > BoundedFileWriter and FileNetLogObserver anyway, so the code is
> cleaner to
> > just
> > > pass by reference and have each make a copy in initializer list.
> > >
> > > The alternative would be to pass by value, and then std::move()
> internally
> in
> > > the ctors. This gets the same number of addrefs (assuming you don't
> forget
> the
> > > std::moves). Sure, you could remove the final addref in
> CreateBounded() by
> > > std::move()ing the local task runner for the second ctor.... but that
> ugliness
> > > isn't wortwhile.
> >
> > I think the conclusion in
> >
> https://groups.google.com/a/chromium.org/forum/#!topic/chrom
> ium-dev/TlL1D-Djta0
> > was to prefer passing scoped_refptrs to const scoped_refptrs?
>
> "If the function (at least sometimes) takes a ref on a refcounted object,
> declare the param as scoped_refptr<T>. The caller can decide whether it
> wishes
> to transfer ownership (by calling std::move(t) when passing t) or retain
> its ref
> (by simply passing t directly)."
>
> (From
> https://chromium.googlesource.com/chromium/src/+/master/styl
> eguide/c++/c++.md)
>
> https://codereview.chromium.org/2966283002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-07-07 21:20:50]

Dry run: CQ is trying da patch.

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

[eroman, 2017-07-07 21:20:53]

Description was changed from

==========
Remove the file thread dependency from FileNetLogObserver.

Instead use an internally created sequenced task runer.

BUG=689520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Remove the file thread dependency from FileNetLogObserver.

Instead use an internally created sequenced task runer.

BUG=689520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[eroman, 2017-07-07 21:20:53]

eroman@chromium.org changed reviewers:
- mef@chromium.org

[mef, 2017-07-07 21:21:37]

mef@chromium.org changed reviewers:
+ mef@chromium.org

[mef, 2017-07-07 21:21:38]

lgtm

[mmenke, 2017-07-07 21:22:13]

On 2017/07/07 21:20:35, eroman wrote:
> Done. Thanks for the links.

No problem, still LGTM!

[eroman, 2017-07-07 21:46:30]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2017-07-07 22:05:14]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2017-07-07 22:05:36]

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-07-07 23:48:41]

CQ is committing da patch.

Bot data: {"patchset_id": 210001, "attempt_start_ts": 1499465114805390,
"parent_rev": "43ce1648d0b0f165758683963f5648c86eb6b775", "commit_rev":
"fc4bcbaa15506e2bebffddc2ad89cb0f6bfe20c5"}

[commit-bot: I haz the power, 2017-07-07 23:48:54]

Description was changed from

==========
Remove the file thread dependency from FileNetLogObserver.

Instead use an internally created sequenced task runer.

BUG=689520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Remove the file thread dependency from FileNetLogObserver.

Instead use an internally created sequenced task runer.

BUG=689520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

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

[commit-bot: I haz the power, 2017-07-07 23:48:56]

Committed patchset #12 (id:210001) as
https://chromium.googlesource.com/chromium/src/+/fc4bcbaa15506e2bebffddc2ad89...