[TaskScheduler] Temporarily co-init all TaskScheduler threads to match SequencedWorkerPool's behavi…

[TaskScheduler] Temporarily co-init all TaskScheduler threads to match SequencedWorkerPool's behaviour.

BUG=622400
Committed: https://crrev.com/94e031354f4fbe6b39dbe23072450986e0e4f022
Cr-Commit-Position: refs/heads/master@{#427748}

[gab, 2016-10-21 18:51:16]

gab@chromium.org changed reviewers:
+ fdoray@chromium.org, robliao@chromium.org

[gab, 2016-10-21 18:51:16]

Rob/Francois, we had talked about this but looks like we never did it. This is
sad but required for now I think...

@Rob: could we init COM only in the scope of tasks redirected from a SWP?
ScopedCOMInitializer is currently documented as
// WARNING: This should only be used once per thread, ideally scoped to a
// similar lifetime as the thread itself.

would it be expensive to CoInit / CoUninit every tasks for SWP tasks only? That
would better serve the intended purpose FWIU as it would at least terminate the
COM scope after a task (not letting the system think it can potentially handle
incoming messages although there is no pump in practice).

[robliao, 2016-10-21 19:02:49]

On 2016/10/21 18:51:16, gab wrote:
> Rob/Francois, we had talked about this but looks like we never did it. This is
> sad but required for now I think...
> 
> @Rob: could we init COM only in the scope of tasks redirected from a SWP?
> ScopedCOMInitializer is currently documented as
> // WARNING: This should only be used once per thread, ideally scoped to a
> // similar lifetime as the thread itself.
> 
> would it be expensive to CoInit / CoUninit every tasks for SWP tasks only?
That
> would better serve the intended purpose FWIU as it would at least terminate
the
> COM scope after a task (not letting the system think it can potentially handle
> incoming messages although there is no pump in practice).

My gut feeling is that rapid CoInit/CoUninit is an unexpected use of CoInit. 
As an alternative, we can just CoInit on ThreadMain and Uninit on Detach.

[gab, 2016-10-21 19:24:06]

On 2016/10/21 19:02:49, robliao wrote:
> On 2016/10/21 18:51:16, gab wrote:
> > Rob/Francois, we had talked about this but looks like we never did it. This
is
> > sad but required for now I think...
> > 
> > @Rob: could we init COM only in the scope of tasks redirected from a SWP?
> > ScopedCOMInitializer is currently documented as
> > // WARNING: This should only be used once per thread, ideally scoped to a
> > // similar lifetime as the thread itself.
> > 
> > would it be expensive to CoInit / CoUninit every tasks for SWP tasks only?
> That
> > would better serve the intended purpose FWIU as it would at least terminate
> the
> > COM scope after a task (not letting the system think it can potentially
handle
> > incoming messages although there is no pump in practice).
> 
> My gut feeling is that rapid CoInit/CoUninit is an unexpected use of CoInit. 
> As an alternative, we can just CoInit on ThreadMain and Uninit on Detach.

This is what this CL is doing with the ScopedCOMInitializer I believe.

[fdoray, 2016-10-21 20:12:43]

On 2016/10/21 19:24:06, gab wrote:
> On 2016/10/21 19:02:49, robliao wrote:
> > On 2016/10/21 18:51:16, gab wrote:
> > > Rob/Francois, we had talked about this but looks like we never did it.
This
> is
> > > sad but required for now I think...
> > > 
> > > @Rob: could we init COM only in the scope of tasks redirected from a SWP?
> > > ScopedCOMInitializer is currently documented as
> > > // WARNING: This should only be used once per thread, ideally scoped to a
> > > // similar lifetime as the thread itself.
> > > 
> > > would it be expensive to CoInit / CoUninit every tasks for SWP tasks only?
> > That
> > > would better serve the intended purpose FWIU as it would at least
terminate
> > the
> > > COM scope after a task (not letting the system think it can potentially
> handle
> > > incoming messages although there is no pump in practice).
> > 
> > My gut feeling is that rapid CoInit/CoUninit is an unexpected use of CoInit.

> > As an alternative, we can just CoInit on ThreadMain and Uninit on Detach.
> 
> This is what this CL is doing with the ScopedCOMInitializer I believe.

No. You CoInit/Uninit once per task because your ScopedCOMInitializer is inside
the while() loop. You should move it just after the OnMainEntry() call.

[gab, 2016-10-26 16:17:30]

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

[commit-bot: I haz the power, 2016-10-26 16:18:04]

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

[gab, 2016-10-26 16:19:06]

On 2016/10/21 20:12:43, fdoray wrote:
> On 2016/10/21 19:24:06, gab wrote:
> > On 2016/10/21 19:02:49, robliao wrote:
> > > On 2016/10/21 18:51:16, gab wrote:
> > > > Rob/Francois, we had talked about this but looks like we never did it.
> This
> > is
> > > > sad but required for now I think...
> > > > 
> > > > @Rob: could we init COM only in the scope of tasks redirected from a
SWP?
> > > > ScopedCOMInitializer is currently documented as
> > > > // WARNING: This should only be used once per thread, ideally scoped to
a
> > > > // similar lifetime as the thread itself.
> > > > 
> > > > would it be expensive to CoInit / CoUninit every tasks for SWP tasks
only?
> > > That
> > > > would better serve the intended purpose FWIU as it would at least
> terminate
> > > the
> > > > COM scope after a task (not letting the system think it can potentially
> > handle
> > > > incoming messages although there is no pump in practice).
> > > 
> > > My gut feeling is that rapid CoInit/CoUninit is an unexpected use of
CoInit.
> 
> > > As an alternative, we can just CoInit on ThreadMain and Uninit on Detach.
> > 
> > This is what this CL is doing with the ScopedCOMInitializer I believe.
> 
> No. You CoInit/Uninit once per task because your ScopedCOMInitializer is
inside
> the while() loop. You should move it just after the OnMainEntry() call.

Duh... of course..! Had somehow assumed mac::ScopedNSAutoreleasePool also had to
be in scope of ThreadMain and blindly dropped it there without looking at
surrounding context..!

Done, PTAnL

[fdoray, 2016-10-26 16:31:28]

lgtm

[robliao, 2016-10-26 17:02:26]

lgtm

[commit-bot: I haz the power, 2016-10-26 17:36:28]

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

[commit-bot: I haz the power, 2016-10-26 17:36:29]

Dry run: This issue passed the CQ dry run.

[gab, 2016-10-26 18:12:12]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-10-26 18:13:13]

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

[commit-bot: I haz the power, 2016-10-26 18:18:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-26 18:22:02]

Description was changed from

==========
[TaskScheduler] Temporarily co-init all TaskScheduler threads to match
SequencedWorkerPool's behaviour.

BUG=622400
==========

to

==========
[TaskScheduler] Temporarily co-init all TaskScheduler threads to match
SequencedWorkerPool's behaviour.

BUG=622400

Committed: https://crrev.com/94e031354f4fbe6b39dbe23072450986e0e4f022
Cr-Commit-Position: refs/heads/master@{#427748}
==========

[commit-bot: I haz the power, 2016-10-26 18:22:02]

Patchset 2 (id:??) landed as
https://crrev.com/94e031354f4fbe6b39dbe23072450986e0e4f022
Cr-Commit-Position: refs/heads/master@{#427748}