[Cronet] Move initialization to a new thread rather than the UI thread.

[Cronet] Move initialization to a new thread rather than the UI thread.

The UI thread is generally overbooked (esp at app startup time) so
avoiding it can greatly improve Cronet startup time.  The UI thread
was used for initialization previously because it simplified the
NetworkChangeNotifierAutoDetect and ProxyChangeListener logic because
BroadcastReceiver onReceived callbacks always happen on the UI thread
so those classes could be completely single-threaded.  This change
leaves those classes single-threaded except for their onRecieved()
methods which now immediately post to the new initialization thread
and check whether their BroadcastReceivers are currently registered.

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

Review-Url: https://codereview.chromium.org/2812963002
Cr-Commit-Position: refs/heads/master@{#469578}
Committed: https://chromium.googlesource.com/chromium/src/+/6815aefeae88a9e62065ca8084075e3e98dec267

[pauljensen, 2017-04-11 17:46:20]

Description was changed from

==========
[Cronet] Move initialization to a new thread rather than the UI thread.

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

to

==========
[Cronet] Move initialization to a new thread rather than the UI thread.

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

[pauljensen, 2017-04-12 17:49:51]

Description was changed from

==========
[Cronet] Move initialization to a new thread rather than the UI thread.

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

to

==========
[Cronet] Move initialization to a new thread rather than the UI thread.

The UI thread is generally overbooked (esp at app startup time) so
avoiding it can greatly improve Cronet startup time.  The UI thread
was used for initialization previously because it simplified the
NetworkChangeNotifierAutoDetect and ProxyChangeListener logic because
BroadcastReceiver onReceived callbacks always happen on the UI thread
so those classes could be completely single-threaded.  This change
leaves those classes single-threaded except for their onRecieved()
methods which now immediately post to the new initialization thread
and check whether their BroadcastReceivers are currently registered.

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

[pauljensen, 2017-04-12 17:56:50]

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

[pauljensen, 2017-04-12 17:56:51]

Helen, could you review this?  I think you're best suited to review as you've
been reviewing NetworkChangeNotifierAutoDetect changes recently and that file
has the most lines changed, but if you want someone else to review, feel free.

[xunjieli, 2017-04-13 15:09:49]

xunjieli@chromium.org changed reviewers:
+ kapishnikov@chromium.org

[xunjieli, 2017-04-13 15:12:59]

On 2017/04/12 17:56:51, pauljensen wrote:
> Helen, could you review this?  I think you're best suited to review as you've
> been reviewing NetworkChangeNotifierAutoDetect changes recently and that file
> has the most lines changed, but if you want someone else to review, feel free.

Sorry for the delay! I looked at the CL briefly and I think this looks good. I
need a bit more time to understand the code. I am looping in kapishnikov@ to get
another pair of eyes on this CL.

[pauljensen, 2017-04-13 16:32:48]

On 2017/04/13 15:12:59, xunjieli wrote:
> On 2017/04/12 17:56:51, pauljensen wrote:
> > Helen, could you review this?  I think you're best suited to review as
you've
> > been reviewing NetworkChangeNotifierAutoDetect changes recently and that
file
> > has the most lines changed, but if you want someone else to review, feel
free.
> 
> Sorry for the delay! I looked at the CL briefly and I think this looks good. I
> need a bit more time to understand the code. I am looping in kapishnikov@ to
get
> another pair of eyes on this CL.

No problem; I'm OOO until the 24th so no rush.

[xunjieli, 2017-04-13 18:44:55]

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

https://codereview.chromium.org/2812963002/diff/100001/components/cronet/andr...
components/cronet/android/cronet_library_loader.cc:75: bool OnInitThread() {
This global static function will return true if g_init_message_loop and
base::MessageLoop::current() are both not initialized.

Can you add a comment on when this function can be called? Alternatively could
add a check on |g_init_message_loop != nullptr|

https://codereview.chromium.org/2812963002/diff/100001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/2812963002/diff/100001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java:30:
private static final HandlerThread sInitThread = new
HandlerThread("CronetInit");
I am fine with the word "init thread." However, since the thread lives forever
and handles messages from the UI thread, calling it initialization thread is a
slightly misleading.

https://codereview.chromium.org/2812963002/diff/100001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java:63:
if (!sInitThread.isAlive()) {
We could also lazily create sInitThread here to avoid the isAlive() check/state.

if (!sInitThread) {
   sInitThread = new HandlerThread();
   sInitThread.start();
}

https://codereview.chromium.org/2812963002/diff/100001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java:106:
new Handler(sInitThread.getLooper()).post(r);
Do we need to test to see if we aren't already on Init thread?
I know that embedder won't have access to Init thread, but if in the future we
might end up posting from the same thread. Maybe add a check so we can detect
that.

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:605: 
nit: maybe get rid of this blank line.

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:682:
* Constructs a NetworkChangeNotifierAutoDetect.  Lives on calling thread.
Could you add the part about receiving notifications from the UI thread?

something like:
Lives on the calling thread, receives broadcast notifications on the UI thread
,and forwards the notifications to the calling thread.

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:1027:
if (!mRegistered) {
I think this needs a comment to note why this check is needed. It's not obvious
from a glance that this receiver might be unregistered when the task runs.

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right):

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/ProxyChangeListener.java:194: if
(!sEnabled || proxyReceiver != mProxyReceiver) {
Same here. Maybe add a comment on we need this extra "proxyReceived !=
mProxyReceiver" check ?

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/ProxyChangeListener.java:238: "Must be
called on NetworkChangeNotifierAutoDetect thread.");
nit: s/NetworkChangeNotifierAutoDetect/ProxyChangeListener

https://codereview.chromium.org/2812963002/diff/100001/net/proxy/proxy_config...
File net/proxy/proxy_config_service_android_unittest.cc (right):

https://codereview.chromium.org/2812963002/diff/100001/net/proxy/proxy_config...
net/proxy/proxy_config_service_android_unittest.cc:71: java_looper_preparer_(),
hmm.. I guess this isn't safe to omit? I'd assume the compiler initializes
|java_looper_preparer_| before |service_| because that's the order specified
below.

[pauljensen, 2017-05-01 15:35:53]

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

https://codereview.chromium.org/2812963002/diff/100001/components/cronet/andr...
components/cronet/android/cronet_library_loader.cc:75: bool OnInitThread() {
On 2017/04/13 18:44:54, xunjieli wrote:
> This global static function will return true if g_init_message_loop and
> base::MessageLoop::current() are both not initialized.
> 
> Can you add a comment on when this function can be called? Alternatively could
> add a check on |g_init_message_loop != nullptr|

Done.

https://codereview.chromium.org/2812963002/diff/100001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/2812963002/diff/100001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java:30:
private static final HandlerThread sInitThread = new
HandlerThread("CronetInit");
On 2017/04/13 18:44:54, xunjieli wrote:
> I am fine with the word "init thread." However, since the thread lives forever
> and handles messages from the UI thread, calling it initialization thread is a
> slightly misleading. 
> 

True, I've updated the comment appropriately.  I couldn't think of a variable
name that wasn't ridiculously long so I've left it called sInitThread.  Let me
know if you think I should rename the variable too.

https://codereview.chromium.org/2812963002/diff/100001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java:63:
if (!sInitThread.isAlive()) {
On 2017/04/13 18:44:54, xunjieli wrote:
> We could also lazily create sInitThread here to avoid the isAlive()
check/state.
> 
> if (!sInitThread) {
>    sInitThread = new HandlerThread();
>    sInitThread.start();
> }

We could but this would require making sInitThread non-final which somewhat
complicates reasoning about sInitThread.  For example one might then wonder if
synchronization to access sInitThread is necessary in postToInitThread() and
ensureInitializedOnInitThread() which I'd like to avoid as it goes against the
Chromium threading approach: "We discourage locking and threadsafe objects."
https://chromium.googlesource.com/chromium/src/+/master/docs/design/threading.md

I think sInitThread.isAlive() will generally only be called once, so I don't
think the performance impact will be significant.

https://codereview.chromium.org/2812963002/diff/100001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java:106:
new Handler(sInitThread.getLooper()).post(r);
On 2017/04/13 18:44:54, xunjieli wrote:
> Do we need to test to see if we aren't already on Init thread?
> I know that embedder won't have access to Init thread, but if in the future we
> might end up posting from the same thread. Maybe add a check so we can detect
> that.

Done.

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:605: 
On 2017/04/13 18:44:54, xunjieli wrote:
> nit: maybe get rid of this blank line.

Done, I also added some comments.

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:682:
* Constructs a NetworkChangeNotifierAutoDetect.  Lives on calling thread.
On 2017/04/13 18:44:54, xunjieli wrote:
> Could you add the part about receiving notifications from the UI thread?
> 
> something like:
> Lives on the calling thread, receives broadcast notifications on the UI thread
> ,and forwards the notifications to the calling thread.

Done.

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:1027:
if (!mRegistered) {
On 2017/04/13 18:44:54, xunjieli wrote:
> I think this needs a comment to note why this check is needed. It's not
obvious
> from a glance that this receiver might be unregistered when the task runs.

Done.

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right):

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/ProxyChangeListener.java:194: if
(!sEnabled || proxyReceiver != mProxyReceiver) {
On 2017/04/13 18:44:54, xunjieli wrote:
> Same here. Maybe add a comment on we need this extra "proxyReceived !=
> mProxyReceiver" check ?

Done.

https://codereview.chromium.org/2812963002/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/ProxyChangeListener.java:238: "Must be
called on NetworkChangeNotifierAutoDetect thread.");
On 2017/04/13 18:44:54, xunjieli wrote:
> nit: s/NetworkChangeNotifierAutoDetect/ProxyChangeListener

Done.

https://codereview.chromium.org/2812963002/diff/100001/net/proxy/proxy_config...
File net/proxy/proxy_config_service_android_unittest.cc (right):

https://codereview.chromium.org/2812963002/diff/100001/net/proxy/proxy_config...
net/proxy/proxy_config_service_android_unittest.cc:71: java_looper_preparer_(),
On 2017/04/13 18:44:54, xunjieli wrote:
> hmm.. I guess this isn't safe to omit? I'd assume the compiler initializes
> |java_looper_preparer_| before |service_| because that's the order specified
> below.

You're right, I think I put it here so the comment would precede the |service_|
initializer call.  I've now moved the comment to java_looper_preparer_
declaration.

[xunjieli, 2017-05-02 14:59:16]

lgtm! one question below.

https://codereview.chromium.org/2812963002/diff/140001/components/cronet/andr...
File components/cronet/android/cronet_library_loader.h (right):

https://codereview.chromium.org/2812963002/diff/140001/components/cronet/andr...
components/cronet/android/cronet_library_loader.h:16: // Only callable after
initiailization thread is started.
nit: typo in initialization.

https://codereview.chromium.org/2812963002/diff/140001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/2812963002/diff/140001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java:34:
// Has ensureInitThreadInitialized() completed?  Only accessed on init thread.
nit: "Only accessed on init thread" is not true. The variable is also accessed
in ensureInitialized() which can be called from any thread. Sine you are here,
could you update this comment?


Not related to this CL, but I do not remember why we had these two booleans as
volatile. Do you know why? Why didn't we synchronize the access with sLoadLock
in ensureInitializedOnInitThread()?

[pauljensen, 2017-05-02 20:35:55]

https://codereview.chromium.org/2812963002/diff/140001/components/cronet/andr...
File components/cronet/android/cronet_library_loader.h (right):

https://codereview.chromium.org/2812963002/diff/140001/components/cronet/andr...
components/cronet/android/cronet_library_loader.h:16: // Only callable after
initiailization thread is started.
On 2017/05/02 14:59:16, xunjieli wrote:
> nit: typo in initialization.

Done.

https://codereview.chromium.org/2812963002/diff/140001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/2812963002/diff/140001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java:34:
// Has ensureInitThreadInitialized() completed?  Only accessed on init thread.
On 2017/05/02 14:59:16, xunjieli wrote:
> nit: "Only accessed on init thread" is not true. The variable is also accessed
> in ensureInitialized() which can be called from any thread. Sine you are here,
> could you update this comment?

Done.

> 
> 
> Not related to this CL, but I do not remember why we had these two booleans as
> volatile. Do you know why? Why didn't we synchronize the access with sLoadLock
> in ensureInitializedOnInitThread()?

The read is synchronized but the write is not.  It should be volatile as it can
be read and written on different threads.

[pauljensen, 2017-05-03 15:29:55]

So I imagined this might slow down initialization because:
1. I'm introducing another thread that must be started up
2. I'm introducing another thread hop (when compared against initializing Cronet
on the UI thread)

So I wrote a little script to test Cronet initialization time using a release
build of the sample app:

while true ; do
  adb shell am force-stop org.chromium.cronet_sample_apk
  adb logcat -c
  sleep 1
  adb shell am start --activity-no-animation -d http://localhost
org.chromium.cronet_sample_apk/.CronetSampleActivity
  sleep 1
  adb logcat -d -v time | grep "Start proc\|onFailed"  | cut -b 13-18 | xargs
echo | sed 's/ /-/' | bc -l
done

I let the script run a bunch of times with and without this CL and was happy to
find out that with and without my change the best initialization time was still
116ms so there didn't appear to be any regression.  I was testing on a Nexus 4
with CPU frequency scaling disabled.

[pauljensen, 2017-05-03 15:32:41]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2017-05-03 15:32:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/2812963002/#ps160001
(title: "address Helen's comments")

[commit-bot: I haz the power, 2017-05-03 15:33: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, 2017-05-03 17:00:34]

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

[commit-bot: I haz the power, 2017-05-03 17:00:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pauljensen, 2017-05-03 17:02:05]

The CQ bit was checked by pauljensen@chromium.org

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

CQ is trying da patch.

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

[pauljensen, 2017-05-03 18:40:53]

The CQ bit was unchecked by pauljensen@chromium.org

[pauljensen, 2017-05-04 14:14:03]

Helen, PTAL @
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
I had to adjust the tests that send synthetic onReceived() updates to first call
NetworkChangeNotifierAutoDetect.register() (which doesn't actually register
because of the mocked out ConnectivityManagerDelegate) so that
NetworkChangeNotifierAutoDetect.mRegistered is true.  I think this is a good
change because it puts the NetworkChangeNotifier into a more realistic state
(getting notifications should only happen when registered).

[xunjieli, 2017-05-05 00:04:45]

On 2017/05/04 14:14:03, pauljensen wrote:
> Helen, PTAL @
> net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
> I had to adjust the tests that send synthetic onReceived() updates to first
call
> NetworkChangeNotifierAutoDetect.register() (which doesn't actually register
> because of the mocked out ConnectivityManagerDelegate) so that
> NetworkChangeNotifierAutoDetect.mRegistered is true.  I think this is a good
> change because it puts the NetworkChangeNotifier into a more realistic state
> (getting notifications should only happen when registered).

lgtm.

[pauljensen, 2017-05-05 01:16:52]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2017-05-05 01:17:28]

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-05-05 03:00:10]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493947012212650,
"parent_rev": "6bd1fc8a43faee33e11dafd0b9e62e0533ee3324", "commit_rev":
"6815aefeae88a9e62065ca8084075e3e98dec267"}

[commit-bot: I haz the power, 2017-05-05 03:00:49]

Description was changed from

==========
[Cronet] Move initialization to a new thread rather than the UI thread.

The UI thread is generally overbooked (esp at app startup time) so
avoiding it can greatly improve Cronet startup time.  The UI thread
was used for initialization previously because it simplified the
NetworkChangeNotifierAutoDetect and ProxyChangeListener logic because
BroadcastReceiver onReceived callbacks always happen on the UI thread
so those classes could be completely single-threaded.  This change
leaves those classes single-threaded except for their onRecieved()
methods which now immediately post to the new initialization thread
and check whether their BroadcastReceivers are currently registered.

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

to

==========
[Cronet] Move initialization to a new thread rather than the UI thread.

The UI thread is generally overbooked (esp at app startup time) so
avoiding it can greatly improve Cronet startup time.  The UI thread
was used for initialization previously because it simplified the
NetworkChangeNotifierAutoDetect and ProxyChangeListener logic because
BroadcastReceiver onReceived callbacks always happen on the UI thread
so those classes could be completely single-threaded.  This change
leaves those classes single-threaded except for their onRecieved()
methods which now immediately post to the new initialization thread
and check whether their BroadcastReceivers are currently registered.

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

Review-Url: https://codereview.chromium.org/2812963002
Cr-Commit-Position: refs/heads/master@{#469578}
Committed:
https://chromium.googlesource.com/chromium/src/+/6815aefeae88a9e62065ca808407...
==========

[commit-bot: I haz the power, 2017-05-05 03:00:50]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/6815aefeae88a9e62065ca808407...