Issue metadata
Sign in to add a comment
|
64kb regression in resource_sizes (MonochromePublic.apk) at 607957:607957 |
||||||||||||||||||
Issue descriptionCaused by “tracing: Add basic mobile tracing preferences” Commit: fa5b7f9870a13aa948fe7de4b5f65b3f91965c93 Link to size graph: https://chromeperf.appspot.com/report?sid=bb23072657e2d7ca892a1c3fa4643b1ee29b3a0a44d0732adda87168e89c0380&num_points=10&rev=607957 Link to trybot result: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-binary-size/94068 Debugging size regressions is documented at: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/apk_size_regressions.md#Debugging-Apk-Size-Increase Looks like this one slipped through the cracks of normal size sheriffing, but I noticed it when prepping for branch review. It a little bit fails the gut-check for how much apk size we should dedicate to a developer-only feature. One easy way to reduce this overhead is move strings from .arsc -> xml/java. It's generally an anti-pattern to use translateable="false" in grd files (I'll follow-up with a presubmit check for this at some point). Here's an example of what I mean: https://chromium-review.googlesource.com/c/chromium/src/+/1381727 Another concern here is java code size / method count. Here's the feature when run with internal proguard: https://storage.googleapis.com/chrome-supersize/viewer.html?load_url=milestones%2Farm%2FMonochrome.apk%2Freport_72.0.3626.7.ndjson&method_count=on&include=%5BtT%5Dracing&exclude=android_webview%7C%2Fbase%7Cwebkit Still pretty big :/. Most ideas will likely make the code uglier, but I think it's worth it in this case. Main idea I came up with when looking at the code: * Use a Handler subclass rather than Runnables / AsyncTasks to combine all nested AsyncTask & Runnables. * Use an interface for callbacks from mNativeController rather than a nested class for each of the three calls (or use the handler for this as well) It's possible that r8 will do better at reducing the overhead of nested classes, so please actually wait on the java part until I can test that out. It'd be great if you could work on the strings first though.
,
Dec 18
Assigning to eseckler@chromium.org because this is the only CL in range: tracing: Add basic mobile tracing preferences Adds a simple preferences menu under the developer options menu with a button to start recording a trace. The recording is controlled via a notification. For now, the recorded trace cannot be configured or shared, we will add these features in follow up patches. See bug for design doc. Bug: 898512 Change-Id: I3a6b4dd24359f8098234ed5fbf5b6db07071a608 Binary-Size: feature implementation, strings won't be translated. Reviewed-on: https://chromium-review.googlesource.com/c/1327208 Commit-Queue: Eric Seckler <eseckler@chromium.org> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org> Reviewed-by: Theresa <twellington@chromium.org> Cr-Commit-Position: refs/heads/master@{#607957}
,
Dec 18
,
Dec 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5b5262277cd56211072a5e2350baf1f5299cf370 commit 5b5262277cd56211072a5e2350baf1f5299cf370 Author: Eric Seckler <eseckler@chromium.org> Date: Tue Dec 18 16:47:32 2018 tracing: Convert mobile tracing strings to literals To reduce binary size of the feature. Based on agrieve's crrev.com/c/1381727. Bug: 916014 Change-Id: I559157f515e2b2490530c4cc05cbb44f3cfbcd5b Reviewed-on: https://chromium-review.googlesource.com/c/1382413 Reviewed-by: Sami Kyöstilä <skyostil@chromium.org> Reviewed-by: Theresa <twellington@chromium.org> Commit-Queue: Eric Seckler <eseckler@chromium.org> Cr-Commit-Position: refs/heads/master@{#617530} [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/java/res/xml/developer_preferences.xml [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/java/res/xml/main_preferences.xml [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/java/res/xml/tracing_preferences.xml [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/java/src/org/chromium/chrome/browser/preferences/AboutChromePreferences.java [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/java/src/org/chromium/chrome/browser/preferences/developer/DeveloperPreferences.java [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/java/src/org/chromium/chrome/browser/preferences/developer/TracingCategoriesPreferences.java [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/java/src/org/chromium/chrome/browser/preferences/developer/TracingPreferences.java [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/java/src/org/chromium/chrome/browser/tracing/TracingController.java [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/java/src/org/chromium/chrome/browser/tracing/TracingNotificationManager.java [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/java/strings/android_chrome_strings.grd [modify] https://crrev.com/5b5262277cd56211072a5e2350baf1f5299cf370/chrome/android/javatests/src/org/chromium/chrome/browser/preferences/developer/TracingPreferencesTest.java
,
Dec 18
From the resource_sizes diff if #4: -27,672 bytes APK size Nice! Thanks for such a quick turn-around! Assigning to me to follow upon the java side once we have r8 available. |
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Dec 18