Reuse InputConnectionHandlerThread

Reuse InputConnectionHandlerThread

This can prevent the number of threads from growing when an app creates
and destroys webviews.

BUG=668692
Committed: https://crrev.com/43fae8979739f1ef5620996a9ac978e100c0ec7c
Cr-Commit-Position: refs/heads/master@{#435557}

[Changwan Ryu, 2016-12-01 02:19:47]

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

[commit-bot: I haz the power, 2016-12-01 02:20:05]

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

[Changwan Ryu, 2016-12-01 02:21:20]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org, tobiasjs@chromium.org, torne@chromium.org,
yabinh@chromium.org

[Changwan Ryu, 2016-12-01 02:21:21]

yabinh@, could you take a quick look at this CL?

[yabinh, 2016-12-01 02:40:12]

non-owner lgtm

[commit-bot: I haz the power, 2016-12-01 03:48:22]

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

[commit-bot: I haz the power, 2016-12-01 03:48:23]

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

[Changwan Ryu, 2016-12-01 04:28:58]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2016-12-01 04:28:58]

The patchset sent to the CQ was uploaded after l-g-t-m from yabinh@chromium.org
Link to the patchset: https://codereview.chromium.org/2543893002/#ps20001
(title: "fixed findit failure")

[commit-bot: I haz the power, 2016-12-01 04:29:10]

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-12-01 05:07:22]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480566538047340,
"parent_rev": "b1de0fda100c5be08c188bf0734d2bbaff373948", "commit_rev":
"9f5dfe514f40c9cd816f1a3c47d8728ae6b3654f"}

[commit-bot: I haz the power, 2016-12-01 05:08:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-01 05:12:09]

Description was changed from

==========
Reuse InputConnectionHandlerThread

This can prevent the number of threads from growing when an app creates
and destroys webviews.

BUG=668692
==========

to

==========
Reuse InputConnectionHandlerThread

This can prevent the number of threads from growing when an app creates
and destroys webviews.

BUG=668692

Committed: https://crrev.com/43fae8979739f1ef5620996a9ac978e100c0ec7c
Cr-Commit-Position: refs/heads/master@{#435557}
==========

[commit-bot: I haz the power, 2016-12-01 05:12:10]

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

[Torne, 2016-12-01 11:01:59]

Thanks for the simpler fix. Some post-commit minor comments that are fine to
address in a followup CL and don't need to be cherrypicked to release branches:

https://codereview.chromium.org/2543893002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/2543893002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:34:
// Reused for multiple WebView instances. TODO(changwan): check if we need to
quit the loop
No, you don't; none of our other global threads exit, and it just complicates
the code by meaning we have to handle its lifetime correctly and thread-safely
again. It's much safer to just keep it forever.

https://codereview.chromium.org/2543893002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:69:
@SuppressFBWarnings("LI_LAZY_INIT_UPDATE_STATIC")
If you suppress a thread safety warning you should probably at least comment (or
ideally add calling-thread assertions) explaining why it's safe to do so. I
assume this is always called on the UI thread?

However, even better than explaining why you suppressed the warning is not
triggering it in the first place. I'm not sure if the test usage is a problem
here or not, but in general there's two good options here:

1) Don't initialise it lazily at all, just do it in a static initialiser. This
is always safe as long as the initialisation can't fail, but it means the thread
would get started up the first time ThreadedInputConnectionFactory is referenced
(probably fine?)

2) Initialise it using
https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom instead.
This is likewise always safe as long as the initialisation can't fail, and it
will defer the thread startup until the first time the method is called; the
only real downside being that it takes a couple of lines more code to write.

If one of those is reasonable and doesn't create problems for the test, then
that's generally better than suppressing the warning - it guarantees the method
never has a thread safety problem even if the usage of the class changes, and it
serves as an example so that other people don't suppress the warning in cases
where it may be a real problem.

The VM is already paying the cost of ensuring that classes are initialised in a
threadsafe way whether you depend on it or not, so the runtime cost of using
option 1 is literally nothing; there's technically a very small runtime cost to
option 2 because it introduces an additional class, but the VM is very efficient
at this because it's all handled with atomics in native code - it's far, far
cheaper than Java synchronisation.

https://codereview.chromium.org/2543893002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:76:
return new Handler(sHandlerThread.getLooper());
Is it actually necessary to make a new handler every time? They're all the same,
no? It seems like it would be sufficient to just store the Handler as a static
variable and not even need to keep a reference to the thread. (and remove
mHandler entirely since it always points to the same place).

[Changwan Ryu, 2016-12-02 09:06:49]

Uploaded https://codereview.chromium.org/2544163003/ as a clean-up / follow-up.

https://codereview.chromium.org/2543893002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
(right):

https://codereview.chromium.org/2543893002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:34:
// Reused for multiple WebView instances. TODO(changwan): check if we need to
quit the loop
On 2016/12/01 11:01:59, Torne wrote:
> No, you don't; none of our other global threads exit, and it just complicates
> the code by meaning we have to handle its lifetime correctly and thread-safely
> again. It's much safer to just keep it forever.

Thanks for the explanation. Comment is removed.

https://codereview.chromium.org/2543893002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:69:
@SuppressFBWarnings("LI_LAZY_INIT_UPDATE_STATIC")
On 2016/12/01 11:01:58, Torne wrote:
> If you suppress a thread safety warning you should probably at least comment
(or
> ideally add calling-thread assertions) explaining why it's safe to do so. I
> assume this is always called on the UI thread?
> 
> However, even better than explaining why you suppressed the warning is not
> triggering it in the first place. I'm not sure if the test usage is a problem
> here or not, but in general there's two good options here:
> 
> 1) Don't initialise it lazily at all, just do it in a static initialiser. This
> is always safe as long as the initialisation can't fail, but it means the
thread
> would get started up the first time ThreadedInputConnectionFactory is
referenced
> (probably fine?)
> 
> 2) Initialise it using
> https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom instead.
> This is likewise always safe as long as the initialisation can't fail, and it
> will defer the thread startup until the first time the method is called; the
> only real downside being that it takes a couple of lines more code to write.
> 
> If one of those is reasonable and doesn't create problems for the test, then
> that's generally better than suppressing the warning - it guarantees the
method
> never has a thread safety problem even if the usage of the class changes, and
it
> serves as an example so that other people don't suppress the warning in cases
> where it may be a real problem.
> 
> The VM is already paying the cost of ensuring that classes are initialised in
a
> threadsafe way whether you depend on it or not, so the runtime cost of using
> option 1 is literally nothing; there's technically a very small runtime cost
to
> option 2 because it introduces an additional class, but the VM is very
efficient
> at this because it's all handled with atomics in native code - it's far, far
> cheaper than Java synchronisation.

I'll go ahead with option 2). I didn't realize that I can use static initializer
inside the initialization-on-demand holder, and was worried that structure may
be too complicated. Now I've uploaded a follow-up CL.

https://codereview.chromium.org/2543893002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:76:
return new Handler(sHandlerThread.getLooper());
On 2016/12/01 11:01:59, Torne wrote:
> Is it actually necessary to make a new handler every time? They're all the
same,
> no? It seems like it would be sufficient to just store the Handler as a static
> variable and not even need to keep a reference to the thread. (and remove
> mHandler entirely since it always points to the same place).

Not at all. I just tried very hard to minimize the code change and ended up like
this. :'( Partially reviving what I initially had for ImeThread feature:

https://codereview.chromium.org/1278593004/diff/1/content/public/android/java...

[Torne, 2016-12-02 12:13:09]

On 2016/12/02 09:06:49, Changwan Ryu wrote:
> I'll go ahead with option 2). I didn't realize that I can use static
initializer
> inside the initialization-on-demand holder, and was worried that structure may
> be too complicated. Now I've uploaded a follow-up CL.

Yup. Java just compiles static initialiser blocks and any initialiser
expressions on static field declarations into one big class-init method, and
calls that class-init method in a magically threadsafe way for you when the
class is referenced :)