Issue metadata
Sign in to add a comment
|
26kb regression in resource_sizes (MonochromePublic.apk) at 547004:547004 |
||||||||||||||||||||
Issue descriptionCaused by “[WebAuthn] Initial add of AuthenticatorImpl” Commit: 3d33e449af12861c2ca6ab5c28e947ef681371b4 Link to size graph: https://chromeperf.appspot.com/report?sid=a097e74b1aa288511afb4cb616efe0f95ba4d347ad61d5e835072f23450938ba&num_points=10&rev=547004 Debugging size regressions is documented at: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/apk_size_regressions.md#Debugging-Apk-Size-Increase Based on the graph, increase is entirely from dex growth
,
Apr 3 2018
Assigning to kpaulhamus@chromium.org because this is the only CL in range: [WebAuthn] Initial add of AuthenticatorImpl This change registers the Java implementation of authenticator.mojom behind a feature flag. The implementation currently returns "Not implemented" errors for both get() and create() calls. Bug: 678885 Change-Id: Ieebff9c70b1c227695b90ed0d045f96af1adf23d Reviewed-on: https://chromium-review.googlesource.com/927722 Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org> Cr-Commit-Position: refs/heads/master@{#547004}
,
Apr 3 2018
Here's the output of: tools/binary_size/diagnose_bloat.py --cloud 3d33e449af12861c2ca6ab5c28e947ef681371b4 .text=336 bytes .rodata=0 bytes .data.rel.ro=32 bytes .data=0 bytes .bss=0 bytes .dex=18.1kb .pak.translations=0 bytes .pak.nontranslated=0 bytes .other=367 bytes total=18.8kb Number of unique paths: 112 Section Legend: t=.text, r=.rodata, R=.data.rel.ro, d=.data, b=.bss, x=.dex, p=.pak.translations, P=.pak.nontranslated, o=.other Index | Running Total | Section@Address | Δ PSS (Δ size_without_padding) | Path ------------------------------------------------------------ ~ 0) -1400 (-7.3%) x@0x0 -1400 (125987->124587) MonochromePublic.apk/prebuilt * Unattributed Dex ~ 1) -694 (-3.6%) o@0x0 706 (0->0) {no path} Overhead: APK file + 2) -76 (-0.4%) x@0x0 618 (0->618) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialCreationOptions org.chromium.webauth.mojom.PublicKeyCredentialCreationOptions decode(org.chromium.mojo.bindings.Decoder) + 3) 320 (1.7%) x@0x0 396 (0->396) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialRequestOptions org.chromium.webauth.mojom.PublicKeyCredentialRequestOptions decode(org.chromium.mojo.bindings.Decoder) + 4) 701 (3.6%) x@0x0 381 (0->381) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.Authenticator_Internal$Stub boolean acceptWithResponder(org.chromium.mojo.bindings.Message,org.chromium.mojo.bindings.MessageReceiver) ~ 5) 362 (1.9%) o@0x0 -339 (0->0) {no path} Overhead: ELF file + 6) 699 (3.6%) x@0x0 337 (0->337) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialDescriptor org.chromium.webauth.mojom.PublicKeyCredentialDescriptor decode(org.chromium.mojo.bindings.Decoder) + 7) 1027 (5.3%) x@0x0 328 (0->328) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialCreationOptions void encode(org.chromium.mojo.bindings.Encoder) + 8) 1338 (6.9%) x@0x0 311 (0->311) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.AuthenticatorSelectionCriteria org.chromium.webauth.mojom.AuthenticatorSelectionCriteria decode(org.chromium.mojo.bindings.Decoder) + 9) 1649 (8.6%) x@0x0 311 (0->311) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialCreationOptions boolean equals(java.lang.Object) + 10) 1935 (10.0%) x@0x0 286 (0->286) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.GetAssertionAuthenticatorResponse org.chromium.webauth.mojom.GetAssertionAuthenticatorResponse decode(org.chromium.mojo.bindings.Decoder) + 11) 2194 (11.4%) x@0x0 259 (0->259) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialRequestOptions boolean equals(java.lang.Object) + 12) 2446 (12.7%) x@0x0 252 (0->252) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialUserEntity org.chromium.webauth.mojom.PublicKeyCredentialUserEntity decode(org.chromium.mojo.bindings.Decoder) + 13) 2678 (13.9%) x@0x0 232 (0->232) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.GetAssertionAuthenticatorResponse boolean equals(java.lang.Object) + 14) 2910 (15.1%) x@0x0 232 (0->232) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialRequestOptions void encode(org.chromium.mojo.bindings.Encoder) + 15) 3133 (16.3%) x@0x0 223 (0->223) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialCreationOptions int hashCode() + 16) 3355 (17.4%) x@0x0 222 (0->222) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialRpEntity org.chromium.webauth.mojom.PublicKeyCredentialRpEntity decode(org.chromium.mojo.bindings.Decoder) + 17) 3572 (18.6%) x@0x0 217 (0->217) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.CommonCredentialInfo org.chromium.webauth.mojom.CommonCredentialInfo decode(org.chromium.mojo.bindings.Decoder) + 18) 3786 (19.7%) x@0x0 214 (0->214) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialUserEntity boolean equals(java.lang.Object) + 19) 3992 (20.7%) x@0x0 206 (0->206) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.Authenticator_Internal$AuthenticatorGetAssertionResponseParams org.chromium.webauth.mojom.Authenticator_Internal$AuthenticatorGetAssertionResponseParams decode(org.chromium.mojo.bindings.Decoder) + 20) 4198 (21.8%) x@0x0 206 (0->206) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.Authenticator_Internal$AuthenticatorMakeCredentialResponseParams org.chromium.webauth.mojom.Authenticator_Internal$AuthenticatorMakeCredentialResponseParams decode(org.chromium.mojo.bindings.Decoder) + 21) 4394 (22.8%) x@0x0 196 (0->196) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.MakeCredentialAuthenticatorResponse org.chromium.webauth.mojom.MakeCredentialAuthenticatorResponse decode(org.chromium.mojo.bindings.Decoder) + 22) 4586 (23.8%) x@0x0 192 (0->192) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialParameters org.chromium.webauth.mojom.PublicKeyCredentialParameters decode(org.chromium.mojo.bindings.Decoder) + 23) 4775 (24.8%) x@0x0 189 (0->189) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.Authenticator_Internal$Stub boolean accept(org.chromium.mojo.bindings.Message) + 24) 4963 (25.8%) x@0x0 188 (0->188) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.CommonCredentialInfo boolean equals(java.lang.Object) + 25) 5151 (26.8%) x@0x0 188 (0->188) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialRpEntity boolean equals(java.lang.Object) + 26) 5336 (27.7%) x@0x0 185 (0->185) third_party/WebKit/public/android_mojo_bindings_java_sources.srcjar org.chromium.webauth.mojom.PublicKeyCredentialRequestOptions int hashCode() Looks like just a lot of generated mojo code. Note that the actual dex increase is ~18kb, but the "normalized size" counts dex as ~1.5x since it gets extracted on-device. This somewhat fails the gut-check for me. It's a lot of code for something that doesn't do very much. Some ideas for reducing the overhead: 1. Don't use mojo's in Java (write C++ interfaces, then use JNI to send only the information you need to the Java side) 2. Work with mojo team to reduce the overhead of mojo's java bindings (e.g. do you really need hashCode() and equals() for every object?)
,
Apr 3 2018
The mojom is unfortunately quite sizeable. It doesn't do very much *yet*, but all of the structs will eventually be used in the Java implementation. Re #1, are there examples that you can point me to? #2 seems reasonable as well; I don't see a need for all of the extra bloat either. This is rather new to me - what's the best way to proceed?
,
Apr 4 2018
There are some docs on JNI at: https://chromium.googlesource.com/chromium/src.git/+/master/base/android/jni_generator/README.md It's also not too hard to get by just copy & pasting from other examples in the codebase. For 2), Maybe email https://groups.google.com/a/chromium.org/forum/#!forum/chromium-mojo, or attempt to dig into mojo code generation and send a patch?
,
Apr 11 2018
I've looked at this a bit closer now and believe we can remove equals() and hashCode() to save lots of space. WIP change here: https://chromium-review.googlesource.com/c/chromium/src/+/1005031 It shows moving the two methods to the base class, but for Unions it doesn't always work. Instead, I think we should just remove them. For tests, we can write custom isEquals(), and we can just advize devs not to use mojom objects directly as keys in a hashmap, or set (are than any docs for java mojo that could be updated to mention this?)
,
Apr 11 2018
Forbidding structs and unions as map keys is not acceptable IMHO. We already have legitimate uses of this.
,
Apr 11 2018
What are the existing usages? Note: you can still do it by wrapping them, or by using .serialize() as a key.
,
Apr 11 2018
That said I think we can address the code size issue for unions too. I'll give that a bit more thought. Either way thanks for looking at this!
,
Apr 11 2018
Sure, you can serialize and deserialize your own keys. I'd like to see if we can preserve the fact that you don't have to without bloating code size. On second glance, I guess there are no longer any uses outside of tests.
,
Apr 11 2018
sg - I'll pause on this until you've sufficiently thought about it. Was pretty easy to maintain isEqual/hashCode() for structs (via serialize()), but for unions they require a parameter (which I just hardcoded to null), which is causing the one test to fail. It's pretty normal in java for objects to not support isEqual() / hashCode(), so I wouldn't expect anyone to be surprised by their absence. It's also generally true the mutable types don't provide hashCode(), since mutating a field will change the hashCode and cause bugs if the object was part of a hashmap when mutated. Here is the current list of options as I see it: * Don't provide isEqual() / hashCode() * Provide them via base class, with the caveat that they will fail for Unions that contain a handle (what I've currently implemented). * Provide them only when a newly added mojom flag is present (generate_java_hashCode = true)
,
Apr 11 2018
Since they aren't in use today in any Java code, I would be OK with deleting them for now to get code size wins. I just don't want to commit to these things never being supported long-term. We should be OK with re-introducing them later assuming we can do so with reasonably little code size impact.
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b41b27df3be8bcececacb1f5637d75b394f079e5 commit b41b27df3be8bcececacb1f5637d75b394f079e5 Author: Andrew Grieve <agrieve@chromium.org> Date: Thu Apr 12 20:59:42 2018 Android: Optimize Java mojom classes for binary size * Removes .equals() and .hashCode() (unused in non-test code) * Coalesce version checks in decode() * Move mTag field from generated to Union base class. diagnose_bloat.py --enable-chrome-android-internal shows: -262 entries methods -33,740 bytes main dex size bytes main dex size * Largest single symbol savings from equals(): -428 * Largest single symbol savings from hashCode(): -299 * Largest single symbol savings from decode(): -112 Bug: 828403 Change-Id: Iad41ec6baf0101fd5080375f3a0e65b561d90395 Reviewed-on: https://chromium-review.googlesource.com/1005031 Commit-Queue: agrieve <agrieve@chromium.org> Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#550354} [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/chrome/android/javatests/src/org/chromium/chrome/browser/CopylessPasteTest.java [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTestUtils.java [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsVersioningTest.java [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/mojo/public/java/bindings/src/org/chromium/mojo/bindings/Union.java [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/mojo/public/tools/bindings/generators/java_templates/data_types_definition.tmpl
,
Apr 12 2018
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b41b27df3be8bcececacb1f5637d75b394f079e5 commit b41b27df3be8bcececacb1f5637d75b394f079e5 Author: Andrew Grieve <agrieve@chromium.org> Date: Thu Apr 12 20:59:42 2018 Android: Optimize Java mojom classes for binary size * Removes .equals() and .hashCode() (unused in non-test code) * Coalesce version checks in decode() * Move mTag field from generated to Union base class. diagnose_bloat.py --enable-chrome-android-internal shows: -262 entries methods -33,740 bytes main dex size bytes main dex size * Largest single symbol savings from equals(): -428 * Largest single symbol savings from hashCode(): -299 * Largest single symbol savings from decode(): -112 Bug: 828403 Change-Id: Iad41ec6baf0101fd5080375f3a0e65b561d90395 Reviewed-on: https://chromium-review.googlesource.com/1005031 Commit-Queue: agrieve <agrieve@chromium.org> Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#550354} [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/chrome/android/javatests/src/org/chromium/chrome/browser/CopylessPasteTest.java [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTestUtils.java [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsVersioningTest.java [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/mojo/public/java/bindings/src/org/chromium/mojo/bindings/Union.java [modify] https://crrev.com/b41b27df3be8bcececacb1f5637d75b394f079e5/mojo/public/tools/bindings/generators/java_templates/data_types_definition.tmpl |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Apr 3 2018