New issue
Advanced search Search tips

Issue 711840 link

Starred by 1 user
Status: WontFix
Owner: ----
Closed: Apr 2017
Cc:
torne@chromium.org
agrieve@chromium.org
tobiasjs@chromium.org
ntfschr@chromium.org
estevenson@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Feature



Sign in to add a comment

consider lazy jni registration everywhere

Project Member Reported by boliu@chromium.org, Apr 14 2017 
History as I discerned..
* chrome registers all jni methods on start up
* webview added and switched lazy jni init (to speed up start iirc?  crbug.com/442327  doesn't say)
* monochrome tried to maintain existing behavior, register on start up if it's running chrome, and lazy register if it's running webview. looks like this originated in private code, so I didn't dig very far
* monochrome changed to always lazy register because that the code to correctly detect webview vs chrome needs jni int he first place [1]
* chrome switches to registering only some classes on start up in child process and other classes lazily (or I guess not at all), because child doesn't load the whole dex file [2]

[1] https://codereview.chromium.org/2593653002/diff/20001/chrome/browser/android/monochrome_entry_point.cc
[2] https://codereview.chromium.org/2501193003


Right now it's super complicated. If one forgets a call to register a class, an only tests webview and monochrome, then the issue is missed entirely. Also I was kinda horrified that non-trivial ShouldSkipJniRegistration is inlined in every generated class.

Can we just do lazy registration everywhere and simplify all of this? Monochrome shipping on N+ already does lazy registration, so whatever perf impact is already there. I'm sure removing complexity will be a net benefit for binary size..

Comment 1 by torne@chromium.org, Apr 17 2017

Status: WontFix (was: Untriaged)
It has to be the way it is, because lazy registration doesn't work when using the crazy linker. The JVM calls dlsym to look up the symbols when they're first called, and libraries loaded by the crazy linker aren't searched.

And yes, the reason we moved to doing it this way in webview was startup latency.

My expectation (though I haven't tested this very thoroughly) is that lazy registration is an all-around perf win with no actual downside; it's not about perf impact, it's just the incompatibility with the crazy linker.

Comment 2 by estevenson@chromium.org, Apr 17 2017 

Also see  issue 683256  for binary size reduction & jni registration simplification.

Comment 3 by estevenson@chromium.org, Apr 17 2017 

FYI: removing inline from ShouldSkipJniRegistration surprisingly actually increases the size by 60 bytes (still probably shouldn't be inlined):

tools/binary_size/supersize archive before.size --elf-file out/Release/lib.unstripped/libmonochrome.so --apk-file out/Release/apks/MonochromePublic.apk
tools/binary_size/supersize diff before.size after.size

Section Sizes (Total=60 bytes):
    .bss: 0 bytes (not included in totals)
    .data: 0 bytes (0.0%)
    .data.rel.ro: 0 bytes (0.0%)
    .data.rel.ro.local: 0 bytes (0.0%)
    .rodata: 0 bytes (0.0%)
    .text: 60 bytes (100.0%)

9 symbols added (+), 7 changed (~), 9 removed (-), 356879 unchanged (not shown)
0 object files added, 0 removed

Comment 4 by agrieve@chromium.org, Apr 18 2017 

Certainly falls under the realm "crazy" linker: is it possible to patch dlsym()? Found what looks to be code that tries to do it here:
https://github.com/asLody/ElfHook/blob/master/src/elf_module.cpp

Only thing I'm not sure about from inspection is whether the PLT would be read-only.

Comment 5 by torne@chromium.org, Apr 18 2017 

We discussed adding the loaded library into the system linker's list, or trying to redirect dlsym in various ways, and came to the conclusion that this was too scary/dangerous due to different android platform versions and vendor customisations, as well as being highly discouraged by the bionic team. I don't think we want to go down that path. :)
►

Sign in to add a comment