[Cronet] Add ExperimentalCronetEngine.Builder.setThreadPriority() API

[Cronet] Add ExperimentalCronetEngine.Builder.setThreadPriority() API

This API allows setting the thread priority of Cronet's internal main thread.

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

Review-Url: https://codereview.chromium.org/2876253002
Cr-Commit-Position: refs/heads/master@{#472449}
Committed: https://chromium.googlesource.com/chromium/src/+/60fae58468c10337e6538d8334034d590bbc86da

[pauljensen, 2017-05-12 19:20:27]

Description was changed from

==========
[Cronet] Add ExperimentalCronetEngine.Builder.setThreadPriority() API

This API allows setting the thread priority of Cronet's internal main thread.

BUG=721519
==========

to

==========
[Cronet] Add ExperimentalCronetEngine.Builder.setThreadPriority() API

This API allows setting the thread priority of Cronet's internal main thread.

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

[pauljensen, 2017-05-13 01:51:05]

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

[pauljensen, 2017-05-13 01:51:06]

Misha, PTAL, thank you!

[mef, 2017-05-15 17:20:11]

Looks reasonable, mostly question whether we really need to use different
default priorities for java and native Cronet Engines.

WDYT?

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java
(right):

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:408:
return mThreadPriority == INVALID_THREAD_PRIORITY ? defaultThreadPriority :
mThreadPriority;
can we just default |mThreadPriority| to sensible defaultThreadPriority?

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:153:
mNetworkThreadPriority =
builder.threadPriority(Process.THREAD_PRIORITY_BACKGROUND);
It is unclear why passing default priority as argument is better than just
specifying it as default in builder.

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java
(right):

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java:48:
builder.threadPriority(THREAD_PRIORITY_BACKGROUND +
THREAD_PRIORITY_MORE_FAVORABLE);
should native thread also default to THREAD_PRIORITY_BACKGROUND +
THREAD_PRIORITY_MORE_FAVORABLE ?

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1341:
assertEquals(threadPriority, getThreadPriority(builder.build()));
This loop creates and leaks 40*2 CronetEngines. 
Is that expected? Should we shutdown those?

[pauljensen, 2017-05-15 19:03:25]

WDYT about first experimenting with the different native thread priorities?

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java
(right):

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:408:
return mThreadPriority == INVALID_THREAD_PRIORITY ? defaultThreadPriority :
mThreadPriority;
On 2017/05/15 17:20:11, mef wrote:
> can we just default |mThreadPriority| to sensible defaultThreadPriority?

I feel that could have significant impact on performance.  I think we should
have an embedder experiment with the background+1 priority, and if that looks
good, then use it for the default.

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:153:
mNetworkThreadPriority =
builder.threadPriority(Process.THREAD_PRIORITY_BACKGROUND);
On 2017/05/15 17:20:11, mef wrote:
> It is unclear why passing default priority as argument is better than just
> specifying it as default in builder.

Because there are different defaults for different callers that share one
builder.

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java
(right):

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java:48:
builder.threadPriority(THREAD_PRIORITY_BACKGROUND +
THREAD_PRIORITY_MORE_FAVORABLE);
On 2017/05/15 17:20:11, mef wrote:
> should native thread also default to THREAD_PRIORITY_BACKGROUND +
> THREAD_PRIORITY_MORE_FAVORABLE ?

Probably, but I'd like to run this as an experiment first.

[pauljensen, 2017-05-16 13:19:45]

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1341:
assertEquals(threadPriority, getThreadPriority(builder.build()));
On 2017/05/15 17:20:11, mef wrote:
> This loop creates and leaks 40*2 CronetEngines. 
> Is that expected? Should we shutdown those?

Done.

[mef, 2017-05-16 15:51:29]

lgtm

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java
(right):

https://codereview.chromium.org/2876253002/diff/10009/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBuilderImpl.java:408:
return mThreadPriority == INVALID_THREAD_PRIORITY ? defaultThreadPriority :
mThreadPriority;
On 2017/05/15 19:03:25, pauljensen wrote:
> On 2017/05/15 17:20:11, mef wrote:
> > can we just default |mThreadPriority| to sensible defaultThreadPriority?
> 
> I feel that could have significant impact on performance.  I think we should
> have an embedder experiment with the background+1 priority, and if that looks
> good, then use it for the default.

Sounds good, it makes sense to experiment.

[pauljensen, 2017-05-17 14:05:22]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2017-05-17 14:05:49]

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-17 14:37:38]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1495029922585680,
"parent_rev": "c313139af54d7870871413bf4697f1969569b6d5", "commit_rev":
"60fae58468c10337e6538d8334034d590bbc86da"}

[commit-bot: I haz the power, 2017-05-17 14:37:49]

Description was changed from

==========
[Cronet] Add ExperimentalCronetEngine.Builder.setThreadPriority() API

This API allows setting the thread priority of Cronet's internal main thread.

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

to

==========
[Cronet] Add ExperimentalCronetEngine.Builder.setThreadPriority() API

This API allows setting the thread priority of Cronet's internal main thread.

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

Review-Url: https://codereview.chromium.org/2876253002
Cr-Commit-Position: refs/heads/master@{#472449}
Committed:
https://chromium.googlesource.com/chromium/src/+/60fae58468c10337e6538d833403...
==========

[commit-bot: I haz the power, 2017-05-17 14:37:50]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/60fae58468c10337e6538d833403...