Extend the multidex logic to only kick in for the main browser application.

Extend the multidex logic to only kick in for the main browser application.

For each Application instances that falls into the Multidex path,
there is a regression in startup time as it must bootstrap the
classloader to be able to access the additional sources.

During a normal run of Chrome on Android, there are three Application
instances created (Main Browser, Renderer, and GPU).

The renderer process is an isolated process, which was being skipped
with the existing check in this file.

The problem is with the GPU process.  We had the assumption that all
non-isolated processes would only be hit once by the Multidex startup
regression, but it turns out that any Application in a different
process is hit by the same slowdown.  This means that the first time
the page attempts to paint anything, you see a hang as the GPU process
is attempting to initialize.

This results in a bunch of tests (mainly on K builds on slow devices)
to start flaking as they timeout trying to load the first non
about:blank page.

This change extends the existing check for isolated process to also
skip initializing multi dex for anything running in a separate
process.  As a result, anything running in a different process will
need to ensure all code is included in the main dex or force the
additional dex files to be loaded (not as of yet needed), but we
no longer see the slow down on basic operations in Chrome.

BUG=560600
Committed: https://crrev.com/ae7c11660f7b39f2d5c4dc28baf404a45aa0fc2d
Cr-Commit-Position: refs/heads/master@{#361738}

[Ted C, 2015-11-24 23:45:25]

tedchoc@chromium.org changed reviewers:
+ nyquist@chromium.org

[Ted C, 2015-11-24 23:45:25]

PTAL

cc yfriedman@ to be amazed/disgusted by the glory

[nyquist, 2015-11-25 00:12:52]

Thanks for putting this hack in the multidex code instead of as a generic tool
for others to use...

One question though, could you expand the CL description a bit given the...
interestingness of the CL...?

These questions would be helpful to answer in the description for future
readers:
- Why do we need to do this? (tests are horrifyingly slow and flake/timeout...)
- What happened before? (multidex kicked in...)
- How does this change make it better?
- Why is the code put in ChromiumMultiDex instead of as a utility in
//base/android?

https://codereview.chromium.org/1469803007/diff/1/base/android/java/templates...
File base/android/java/templates/ChromiumMultiDex.template (right):

https://codereview.chromium.org/1469803007/diff/1/base/android/java/templates...
base/android/java/templates/ChromiumMultiDex.template:69: try {
Nit: Could you extract most of this block out to a private static
getCurrentProcessName() (except the return check...)?

https://codereview.chromium.org/1469803007/diff/1/base/android/java/templates...
base/android/java/templates/ChromiumMultiDex.template:82: return
currentProcessName != null && !currentProcessName.contains(":");
Could you add a comment explaining the significance of a colon being part of the
process name?

[Ted C, 2015-11-25 00:33:50]

Description was changed from

==========
Extend the multidex logic to only kick in for the main browser application.

BUG=560600
==========

to

==========
Extend the multidex logic to only kick in for the main browser application.

For each Application instances that falls into the Multidex path,
there is a regression in startup time as it must bootstrap the
classloader to be able to access the additional sources.

During a normal run of Chrome on Android, there are three Application
instances created (Main Browser, Renderer, and GPU).

The renderer process is an isolated process, which was being skipped
with the existing check in this file.

The problem is with the GPU process.  We had the assumption that all
non-isolated processes would only be hit once by the Multidex startup
regression, but it turns out that any Application in a different 
process is hit by the same slowdown.  This means that the first time
the page attempts to paint anything, you see a hang as the GPU process
is attempting to initialize.

This results in a bunch of tests (mainly on K builds on slow devices)
to start flaking as they timeout trying to load the first non 
about:blank page.

This change extends the existing check for isolated process to also
skip initializing multi dex for anything running in a separate 
process.  As a result, anything running in a different process will
need to ensure all code is included in the main dex or force the
additional dex files to be loaded (not as of yet needed), but we
no longer see the slow down on basic operations in Chrome.

BUG=560600
==========

[Ted C, 2015-11-25 00:49:44]

On 2015/11/25 00:12:52, nyquist wrote:
> Thanks for putting this hack in the multidex code instead of as a generic tool
> for others to use...
> 
> One question though, could you expand the CL description a bit given the...
> interestingness of the CL...?
> 
> These questions would be helpful to answer in the description for future
> readers:
> - Why do we need to do this? (tests are horrifyingly slow and
flake/timeout...)
> - What happened before? (multidex kicked in...)
> - How does this change make it better?
> - Why is the code put in ChromiumMultiDex instead of as a utility in
> //base/android?
> 

Expanded the comment.

Sadly, I think this change isn't sufficient.  We have things that run in
separate
processes that we need to make sure load all of the dex files.  I think I'll
need to twiddle with this a bit more.

I still want to avoid it for the gpu process as loading a page is the common
behavior pattern, but I might have to make this grosser for other places
(i.e. the BrowserRestartActivity)

>
https://codereview.chromium.org/1469803007/diff/1/base/android/java/templates...
> File base/android/java/templates/ChromiumMultiDex.template (right):
> 
>
https://codereview.chromium.org/1469803007/diff/1/base/android/java/templates...
> base/android/java/templates/ChromiumMultiDex.template:69: try {
> Nit: Could you extract most of this block out to a private static
> getCurrentProcessName() (except the return check...)?
> 
>
https://codereview.chromium.org/1469803007/diff/1/base/android/java/templates...
> base/android/java/templates/ChromiumMultiDex.template:82: return
> currentProcessName != null && !currentProcessName.contains(":");
> Could you add a comment explaining the significance of a colon being part of
the
> process name?

[Ted C, 2015-11-25 01:47:23]

Updated to check for privileged processes explicitly.  This is a gross
check to be in base, but I think the "easiest" way to unhose the bots
that are currently timing out.

Thoughts?  Pitchforks?

[Yaron, 2015-11-25 01:48:10]

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

[Yaron, 2015-11-25 01:48:10]

Can't really think of a good answer. If you're already getting the process name,
you could have a requirement that it reads meta data in the manifest. So
basically you'd have something like:

<meta-data android:name="<process_name>.load_multidex" android:value="1">

In the absence of it, that process won't do it. I can't really think of another
way of doing it given that you only have the app context but I think that could
work

[Ted C, 2015-11-25 02:36:56]

On 2015/11/25 01:48:10, Yaron wrote:
> Can't really think of a good answer. If you're already getting the process
name,
> you could have a requirement that it reads meta data in the manifest. So
> basically you'd have something like:
> 
> <meta-data android:name="<process_name>.load_multidex" android:value="1">
> 
> In the absence of it, that process won't do it. I can't really think of
another
> way of doing it given that you only have the app context but I think that
could
> work

Hmm...that would be somewhat sad to have to set meta data at the application
level in addition to defining the <activity> or whatever.

And after doing the initial experiment, I think a blacklist for multidex is
probably
the approach I'd use.

So really the change would be to update the privileges service definition to be:

        {% for i in range(num_privileged_services) %}
        <meta-data android:name="privileged_process{{ i }}.ignore_multidex"
android:value="1" />

         .... existing definition
        <service
android:name="org.chromium.content.app.PrivilegedProcessService{{ i }}"
            android:process=":privileged_process{{ i }}"
            android:permission="{{ manifest_package }}.permission.CHILD_SERVICE"
            android:isolatedProcess="false"
            android:exported="false" />
        {% endfor %}

I don't really care either way as I hope this to be a short term solution until
we figure out why the processes that aren't isolated are behaving badly.

[jbudorick, 2015-11-25 15:16:15]

On 2015/11/25 02:36:56, Ted C wrote:
> On 2015/11/25 01:48:10, Yaron wrote:
> > Can't really think of a good answer. If you're already getting the process
> name,
> > you could have a requirement that it reads meta data in the manifest. So
> > basically you'd have something like:
> > 
> > <meta-data android:name="<process_name>.load_multidex" android:value="1">
> > 
> > In the absence of it, that process won't do it. I can't really think of
> another
> > way of doing it given that you only have the app context but I think that
> could
> > work
> 
> Hmm...that would be somewhat sad to have to set meta data at the application
> level in addition to defining the <activity> or whatever.
> 
> And after doing the initial experiment, I think a blacklist for multidex is
> probably
> the approach I'd use.
> 
> So really the change would be to update the privileges service definition to
be:
> 
>         {% for i in range(num_privileged_services) %}
>         <meta-data android:name="privileged_process{{ i }}.ignore_multidex"
> android:value="1" />
> 
>          .... existing definition
>         <service
> android:name="org.chromium.content.app.PrivilegedProcessService{{ i }}"
>             android:process=":privileged_process{{ i }}"
>             android:permission="{{ manifest_package
}}.permission.CHILD_SERVICE"
>             android:isolatedProcess="false"
>             android:exported="false" />
>         {% endfor %}
> 
> I don't really care either way as I hope this to be a short term solution
until
> we figure out why the processes that aren't isolated are behaving badly.

The multidex support library uses the multidex.version shared preferences file
to store the timestamp, the CRC of the APK, and the number of dexes. It uses the
timestamp and the CRC to check whether or not it needs to extract the secondary
dexes. However, the privileged processes (despite the name) don't have
permission to read that shared preferences file, so they always think they need
to extract the secondary dexes.

This is the logcat from a run with a fresh installation with a few tweaks to
ChromiumMultiDex:

$ adb logcat -c && adb logcat -v threadtime | grep multidex -i

11-25 05:03:34.229 10018 10018 I cr_base_multidex: timestamp: -1
11-25 05:03:34.229 10018 10018 I cr_base_multidex: crc: -1
11-25 05:03:34.229 10018 10018 I cr_base_multidex: dex.number: 1
11-25 05:03:34.229 10018 10018 I cr_base_multidex: dataDir:
/data/data/org.chromium.chrome
11-25 05:03:34.229 10018 10018 I cr_base_multidex: shared preferences file:
/data/data/org.chromium.chrome/shared_prefs/multidex.version.xml
11-25 05:03:34.229 10018 10018 I cr_base_multidex: readable: false
11-25 05:03:34.229 10018 10018 I MultiDex: VM with version 1.6.0 does not have
multidex support
11-25 05:03:34.229 10018 10018 I MultiDex: install
11-25 05:03:34.229 10018 10018 I MultiDex:
MultiDexExtractor.load(/data/app/org.chromium.chrome-1.apk, false)
11-25 05:03:34.239 10018 10018 I MultiDex: Detected that extraction must be
performed.
11-25 05:03:34.289 10018 10018 I MultiDex: Extraction is needed for file
/data/data/org.chromium.chrome/code_cache/secondary-dexes/org.chromium.chrome-1.apk.classes2.zip
11-25 05:03:34.289 10018 10018 I MultiDex: Extracting
/data/data/org.chromium.chrome/code_cache/secondary-dexes/org.chromium.chrome-1.apk.classes-734512617.zip
11-25 05:03:35.559 10018 10018 I MultiDex: Renaming to
/data/data/org.chromium.chrome/code_cache/secondary-dexes/org.chromium.chrome-1.apk.classes2.zip
11-25 05:03:35.559 10018 10018 I MultiDex: Extraction success - length
/data/data/org.chromium.chrome/code_cache/secondary-dexes/org.chromium.chrome-1.apk.classes2.zip:
2298991
11-25 05:03:35.559 10018 10018 I MultiDex: load found 1 secondary dex files
11-25 05:03:36.639 10018 10018 I MultiDex: install done
11-25 05:03:36.639 10018 10018 I cr_base_multidex: Completed multidex
installation.
11-25 05:03:36.639 10018 10018 I cr_base_multidex: timestamp: 1448427726000
11-25 05:03:36.639 10018 10018 I cr_base_multidex: crc: 750363190
11-25 05:03:36.649 10018 10018 I cr_base_multidex: dex.number: 2
11-25 05:03:36.649 10018 10018 I cr_base_multidex: dataDir:
/data/data/org.chromium.chrome
11-25 05:03:36.649 10018 10018 I cr_base_multidex: shared preferences file:
/data/data/org.chromium.chrome/shared_prefs/multidex.version.xml
11-25 05:03:36.649 10018 10018 I cr_base_multidex: readable: true
11-25 05:03:36.649 10018 10018 I MultiDex: install
11-25 05:03:36.649 10018 10018 I cr_base_multidex: Completed multidex
installation.
11-25 05:03:37.329 10065 10065 I cr_base_multidex: Skipping multidex
installation: inside isolated process.
11-25 05:03:37.599 10102 10102 I cr_base_multidex: timestamp: -1
11-25 05:03:37.599 10102 10102 I cr_base_multidex: crc: -1
11-25 05:03:37.599 10102 10102 I cr_base_multidex: dex.number: 1
11-25 05:03:37.599 10102 10102 I cr_base_multidex: dataDir:
/data/data/org.chromium.chrome
11-25 05:03:37.599 10102 10102 I cr_base_multidex: shared preferences file:
/data/data/org.chromium.chrome/shared_prefs/multidex.version.xml
11-25 05:03:37.599 10102 10102 I cr_base_multidex: readable: false
11-25 05:03:37.609 10102 10102 I MultiDex: VM with version 1.6.0 does not have
multidex support
11-25 05:03:37.609 10102 10102 I MultiDex: install
11-25 05:03:37.609 10102 10102 I MultiDex:
MultiDexExtractor.load(/data/app/org.chromium.chrome-1.apk, false)
11-25 05:03:37.609 10102 10102 I MultiDex: Detected that extraction must be
performed.
11-25 05:03:37.699 10102 10102 I MultiDex: Extraction is needed for file
/data/data/org.chromium.chrome/code_cache/secondary-dexes/org.chromium.chrome-1.apk.classes2.zip
11-25 05:03:37.699 10102 10102 I MultiDex: Extracting
/data/data/org.chromium.chrome/code_cache/secondary-dexes/org.chromium.chrome-1.apk.classes-734512617.zip
11-25 05:03:39.109 10102 10102 I MultiDex: Renaming to
/data/data/org.chromium.chrome/code_cache/secondary-dexes/org.chromium.chrome-1.apk.classes2.zip
11-25 05:03:39.109 10102 10102 I MultiDex: Extraction success - length
/data/data/org.chromium.chrome/code_cache/secondary-dexes/org.chromium.chrome-1.apk.classes2.zip:
2298991
11-25 05:03:39.109 10102 10102 I MultiDex: load found 1 secondary dex files
11-25 05:03:40.249 10102 10102 I MultiDex: install done
11-25 05:03:40.249 10102 10102 I cr_base_multidex: Completed multidex
installation.

10018, the browser process, extracts the secondary dex. 10065, the renderer
process, is isolated and skips multidex installation entirely. 10102, the
privileged process, doesn't have permission to read the shared preferences but
does have permission to read and extract the secondary dex.

I'm looking a little into why a service process wouldn't be able to open a
shared preferences file, but I haven't found anything yet.

[Ted C, 2015-11-25 17:26:15]

Just uploaded a version that uses the manifest meta-data approach.  How does
that look?

[jbudorick, 2015-11-25 18:13:35]

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

[jbudorick, 2015-11-25 18:13:36]

https://codereview.chromium.org/1469803007/diff/40001/base/android/java/templ...
File base/android/java/templates/ChromiumMultiDex.template (right):

https://codereview.chromium.org/1469803007/diff/40001/base/android/java/templ...
base/android/java/templates/ChromiumMultiDex.template:51: &&
!shouldInstallMultiDex(context)) {
One other thing to consider: we could potentially only skip multidex extraction
rather than skipping installation entirely. This would involve reflecting into
the multidex support library, but it would let privileged processes use code
from secondary dexes without having to pay the extraction penalty every time.

[Ted C, 2015-11-25 18:17:15]

https://codereview.chromium.org/1469803007/diff/40001/base/android/java/templ...
File base/android/java/templates/ChromiumMultiDex.template (right):

https://codereview.chromium.org/1469803007/diff/40001/base/android/java/templ...
base/android/java/templates/ChromiumMultiDex.template:51: &&
!shouldInstallMultiDex(context)) {
On 2015/11/25 18:13:35, jbudorick wrote:
> One other thing to consider: we could potentially only skip multidex
extraction
> rather than skipping installation entirely. This would involve reflecting into
> the multidex support library, but it would let privileged processes use code
> from secondary dexes without having to pay the extraction penalty every time.

Maybe, but right now it doesn't seem we need anything from the secondary dexes,
so this seems like even more complication for stuff that is unneeded.

[nyquist, 2015-11-25 18:57:20]

lgtm

https://codereview.chromium.org/1469803007/diff/40001/base/android/java/templ...
File base/android/java/templates/ChromiumMultiDex.template (right):

https://codereview.chromium.org/1469803007/diff/40001/base/android/java/templ...
base/android/java/templates/ChromiumMultiDex.template:29: private static final
String IGNORE_MULTIDEX_KEY = ".ignore_multidex";
Could you add a comment here referring to the value in the manifest?

[Yaron, 2015-11-25 19:02:59]

lgtm

https://codereview.chromium.org/1469803007/diff/40001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1469803007/diff/40001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:608: <meta-data android:name="{{
manifest_package }}{{ privileged_process_name }}.ignore_multidex"
nit: omit this for release builds (ok as follow-up)

[Ted C, 2015-11-25 19:14:11]

https://codereview.chromium.org/1469803007/diff/40001/base/android/java/templ...
File base/android/java/templates/ChromiumMultiDex.template (right):

https://codereview.chromium.org/1469803007/diff/40001/base/android/java/templ...
base/android/java/templates/ChromiumMultiDex.template:29: private static final
String IGNORE_MULTIDEX_KEY = ".ignore_multidex";
On 2015/11/25 18:57:20, nyquist wrote:
> Could you add a comment here referring to the value in the manifest?

Done.

https://codereview.chromium.org/1469803007/diff/40001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1469803007/diff/40001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:608: <meta-data android:name="{{
manifest_package }}{{ privileged_process_name }}.ignore_multidex"
On 2015/11/25 19:02:59, Yaron wrote:
> nit: omit this for release builds (ok as follow-up)

Added a TODO, this looks like it will require adding a new jinja variable as
build type is not exposed as of yet.  So a follow up will be better.

[Ted C, 2015-11-25 19:21:44]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2015-11-25 19:21:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1469803007/#ps60001
(title: "Address review comments")

[commit-bot: I haz the power, 2015-11-25 19:22:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1469803007/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1469803007/60001

[commit-bot: I haz the power, 2015-11-25 21:16:17]

Description was changed from

==========
Extend the multidex logic to only kick in for the main browser application.

For each Application instances that falls into the Multidex path,
there is a regression in startup time as it must bootstrap the
classloader to be able to access the additional sources.

During a normal run of Chrome on Android, there are three Application
instances created (Main Browser, Renderer, and GPU).

The renderer process is an isolated process, which was being skipped
with the existing check in this file.

The problem is with the GPU process.  We had the assumption that all
non-isolated processes would only be hit once by the Multidex startup
regression, but it turns out that any Application in a different 
process is hit by the same slowdown.  This means that the first time
the page attempts to paint anything, you see a hang as the GPU process
is attempting to initialize.

This results in a bunch of tests (mainly on K builds on slow devices)
to start flaking as they timeout trying to load the first non 
about:blank page.

This change extends the existing check for isolated process to also
skip initializing multi dex for anything running in a separate 
process.  As a result, anything running in a different process will
need to ensure all code is included in the main dex or force the
additional dex files to be loaded (not as of yet needed), but we
no longer see the slow down on basic operations in Chrome.

BUG=560600
==========

to

==========
Extend the multidex logic to only kick in for the main browser application.

For each Application instances that falls into the Multidex path,
there is a regression in startup time as it must bootstrap the
classloader to be able to access the additional sources.

During a normal run of Chrome on Android, there are three Application
instances created (Main Browser, Renderer, and GPU).

The renderer process is an isolated process, which was being skipped
with the existing check in this file.

The problem is with the GPU process.  We had the assumption that all
non-isolated processes would only be hit once by the Multidex startup
regression, but it turns out that any Application in a different 
process is hit by the same slowdown.  This means that the first time
the page attempts to paint anything, you see a hang as the GPU process
is attempting to initialize.

This results in a bunch of tests (mainly on K builds on slow devices)
to start flaking as they timeout trying to load the first non 
about:blank page.

This change extends the existing check for isolated process to also
skip initializing multi dex for anything running in a separate 
process.  As a result, anything running in a different process will
need to ensure all code is included in the main dex or force the
additional dex files to be loaded (not as of yet needed), but we
no longer see the slow down on basic operations in Chrome.

BUG=560600
==========

[commit-bot: I haz the power, 2015-11-25 21:16:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-25 21:17:34]

Description was changed from

==========
Extend the multidex logic to only kick in for the main browser application.

For each Application instances that falls into the Multidex path,
there is a regression in startup time as it must bootstrap the
classloader to be able to access the additional sources.

During a normal run of Chrome on Android, there are three Application
instances created (Main Browser, Renderer, and GPU).

The renderer process is an isolated process, which was being skipped
with the existing check in this file.

The problem is with the GPU process.  We had the assumption that all
non-isolated processes would only be hit once by the Multidex startup
regression, but it turns out that any Application in a different 
process is hit by the same slowdown.  This means that the first time
the page attempts to paint anything, you see a hang as the GPU process
is attempting to initialize.

This results in a bunch of tests (mainly on K builds on slow devices)
to start flaking as they timeout trying to load the first non 
about:blank page.

This change extends the existing check for isolated process to also
skip initializing multi dex for anything running in a separate 
process.  As a result, anything running in a different process will
need to ensure all code is included in the main dex or force the
additional dex files to be loaded (not as of yet needed), but we
no longer see the slow down on basic operations in Chrome.

BUG=560600
==========

to

==========
Extend the multidex logic to only kick in for the main browser application.

For each Application instances that falls into the Multidex path,
there is a regression in startup time as it must bootstrap the
classloader to be able to access the additional sources.

During a normal run of Chrome on Android, there are three Application
instances created (Main Browser, Renderer, and GPU).

The renderer process is an isolated process, which was being skipped
with the existing check in this file.

The problem is with the GPU process.  We had the assumption that all
non-isolated processes would only be hit once by the Multidex startup
regression, but it turns out that any Application in a different
process is hit by the same slowdown.  This means that the first time
the page attempts to paint anything, you see a hang as the GPU process
is attempting to initialize.

This results in a bunch of tests (mainly on K builds on slow devices)
to start flaking as they timeout trying to load the first non
about:blank page.

This change extends the existing check for isolated process to also
skip initializing multi dex for anything running in a separate
process.  As a result, anything running in a different process will
need to ensure all code is included in the main dex or force the
additional dex files to be loaded (not as of yet needed), but we
no longer see the slow down on basic operations in Chrome.

BUG=560600

Committed: https://crrev.com/ae7c11660f7b39f2d5c4dc28baf404a45aa0fc2d
Cr-Commit-Position: refs/heads/master@{#361738}
==========

[commit-bot: I haz the power, 2015-11-25 21:17:35]

Patchset 4 (id:??) landed as
https://crrev.com/ae7c11660f7b39f2d5c4dc28baf404a45aa0fc2d
Cr-Commit-Position: refs/heads/master@{#361738}