Allow posting the CronetEngine UI Thread initialization on a custom Ui executor instead of the defa…

Allow posting the CronetEngine UI Thread initialization on a custom Ui executor instead of the default Looper.getMainLooper().

Internal bug: b/32122867

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

[sobrinho, 2017-04-07 05:23:17]

Description was changed from

==========
Allow posting the CronetEngine UI Thread initialization on a custom Ui executor
instead of the default Looper.getMainLooper().

BUG=709336
==========

to

==========
Allow posting the CronetEngine UI Thread initialization on a custom Ui executor
instead of the default Looper.getMainLooper().

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

[sobrinho, 2017-04-07 05:30:03]

sobrinho@google.com changed reviewers:
+ mef@google.com

[sobrinho, 2017-04-07 05:31:03]

Description was changed from

==========
Allow posting the CronetEngine UI Thread initialization on a custom Ui executor
instead of the default Looper.getMainLooper().

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

to

==========
Allow posting the CronetEngine UI Thread initialization on a custom Ui executor
instead of the default Looper.getMainLooper().

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

[sobrinho, 2017-04-07 05:31:03]

sobrinho@google.com changed reviewers:
+ mef@chromium.org

[sobrinho, 2017-04-07 05:31:32]

sobrinho@google.com changed reviewers:
- mef@google.com

[sobrinho, 2017-04-07 05:31:49]

The CQ bit was checked by sobrinho@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 05:32:17]

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

[sobrinho, 2017-04-07 05:34:00]

Description was changed from

==========
Allow posting the CronetEngine UI Thread initialization on a custom Ui executor
instead of the default Looper.getMainLooper().

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

to

==========
Allow posting the CronetEngine UI Thread initialization on a custom Ui executor
instead of the default Looper.getMainLooper().

BUG=709336, 32122867
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[sobrinho, 2017-04-07 05:34:55]

Description was changed from

==========
Allow posting the CronetEngine UI Thread initialization on a custom Ui executor
instead of the default Looper.getMainLooper().

BUG=709336, 32122867
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Allow posting the CronetEngine UI Thread initialization on a custom Ui executor
instead of the default Looper.getMainLooper().

Internal bug: b/32122867

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

[commit-bot: I haz the power, 2017-04-07 06:02:24]

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

[commit-bot: I haz the power, 2017-04-07 06:02:25]

Dry run: This issue passed the CQ dry run.

[pauljensen, 2017-04-07 11:51:40]

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

[pauljensen, 2017-04-07 11:51:41]

I don't understand the problem statement:
If embedders want to control when UI initialization happens, can't they launch
CronetEngine initialization on the UI thread when they desire?  Initializing
CronetEngine on the UI thread (e.g. in Application.onCreate) should not post to
the UI thread.
I also don't like the idea of exposing the internal workings of Cronet
initialization to embedders through an API.  I'd much prefer that we simply
remove the UI thread requirements of the NetworkChangeNotifier.

[kapishnikov, 2017-04-07 15:29:07]

On 2017/04/07 11:51:41, pauljensen wrote:
> I don't understand the problem statement:
> If embedders want to control when UI initialization happens, can't they launch
> CronetEngine initialization on the UI thread when they desire?  Initializing
> CronetEngine on the UI thread (e.g. in Application.onCreate) should not post
to
> the UI thread.
> I also don't like the idea of exposing the internal workings of Cronet
> initialization to embedders through an API.  I'd much prefer that we simply
> remove the UI thread requirements of the NetworkChangeNotifier.

As I understand the current problem is that the embedder now has only two
choices:

Option 1: Initialize the entire engine on the UI thread and lose the benefit of
multithreading. In this case, the app startup time will increase, since
everything will be executed on a single thread.

Option 2: Initialize the engine on a secondary thread. In that case, the
secondary thread will eventually post ensureInitializedOnMainThread() task to
the UI thread, which will be blocked until onCreate() method has finished. This
eliminates the benefit of the connection prewarming.

The proposed change tries to mitigate the disadvantages of both option 1 &
option 2. However, my concern is that this solution will introduce considerable
complexity to the embedders code, specifically to the onCreate() method. As I
understand, the onCreate() method will need to periodically poll the UiExcecutor
to see if there is anything that onCreate() method should execute. Is there any
other solution without polling the executor?

I agree with Paul that much better solution would be removing the UI thread
requirements of the NetworkChangeNotifier (which is probably not trivial at
all). It will significantly improve the apps start up time and don't introduce
any additional complexity to the embedders' code.

[sobrinho, 2017-04-07 18:36:28]

On 2017/04/07 15:29:07, kapishnikov wrote:
> On 2017/04/07 11:51:41, pauljensen wrote:
> > I don't understand the problem statement:
> > If embedders want to control when UI initialization happens, can't they
launch
> > CronetEngine initialization on the UI thread when they desire?  Initializing
> > CronetEngine on the UI thread (e.g. in Application.onCreate) should not post
> to
> > the UI thread.
> > I also don't like the idea of exposing the internal workings of Cronet
> > initialization to embedders through an API.  I'd much prefer that we simply
> > remove the UI thread requirements of the NetworkChangeNotifier.
> 
> As I understand the current problem is that the embedder now has only two
> choices:
> 
> Option 1: Initialize the entire engine on the UI thread and lose the benefit
of
> multithreading. In this case, the app startup time will increase, since
> everything will be executed on a single thread.
> 
> Option 2: Initialize the engine on a secondary thread. In that case, the
> secondary thread will eventually post ensureInitializedOnMainThread() task to
> the UI thread, which will be blocked until onCreate() method has finished.
This
> eliminates the benefit of the connection prewarming.
> 
> The proposed change tries to mitigate the disadvantages of both option 1 &
> option 2. However, my concern is that this solution will introduce
considerable
> complexity to the embedders code, specifically to the onCreate() method. As I
> understand, the onCreate() method will need to periodically poll the
UiExcecutor
> to see if there is anything that onCreate() method should execute. Is there
any
> other solution without polling the executor?
> 
> I agree with Paul that much better solution would be removing the UI thread
> requirements of the NetworkChangeNotifier (which is probably not trivial at
> all). It will significantly improve the apps start up time and don't introduce
> any additional complexity to the embedders' code.

Yes, that is exactly the problem. Being able to prewarm a connection while
inside the Application.onCreate() means we have to initialize the CronetEngine
in the UI thread. This is at least 100-120ms in a Nexus 4 running Lollipop.
Although I agree the better solution would be to remove the need of the UI
thread, I also believe this is a good starting point until that requirement is
removed.

If you prefer me to hide it from the normal CronetEngine and only allow if to be
set on the Experimental one, then just let me know.

[pauljensen, 2017-04-07 19:12:05]

I feel somewhat strongly that we shouldn't add a complex API for this, and
should instead put effort into fixing the underlying problem of removing the UI
thread initialization requirement.  I think only the
NetworkChangeNotifierAutoDetector requires initialization on the UI thread, and
I think we could remove the UI thread requirement without too much work.

[sobrinho, 2017-04-07 20:33:45]

On 2017/04/07 19:12:05, pauljensen wrote:
> I feel somewhat strongly that we shouldn't add a complex API for this, and
> should instead put effort into fixing the underlying problem of removing the
UI
> thread initialization requirement.  I think only the
> NetworkChangeNotifierAutoDetector requires initialization on the UI thread,
and
> I think we could remove the UI thread requirement without too much work.

I'm ok with removing the UI thread requirement altogether. That was always the
preferred long term solution.

There was also a native call to the library that also needs to run on the Ui
Thread.
Could you check what is being done currently on the Ui Thread and see if
everything can be removed? Also any ETAs on that?

Thanks

[pauljensen, 2017-04-10 18:47:48]

I started work to move a good chunk of Cronet initialization off the UI thread:
https://codereview.chromium.org/2811813003/

[pauljensen, 2017-04-11 17:47:22]

I started a new attempt today:
https://codereview.chromium.org/2812963002/

[chromium-reviews, 2017-04-11 18:51:00]

Thanks! Looks good and simple.
One thing I'm not sure is if you need to call the HandlerThread.start().

Also be careful with the Looper.myLooper() == anotherLooper comparison.
IIRC, in some cases, if you haven't called Looper.prepare() then that code
might return null and you might end up comparing null with null and assume
you are in the right thread when you are not.

On Tue, 11 Apr 2017, 10:47 , <pauljensen@chromium.org> wrote:

> I started a new attempt today:
> https://codereview.chromium.org/2812963002/
>
> https://codereview.chromium.org/2805743003/
>

-- 
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.