New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 634998 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Use base_proguard_config.flags in Cronet's proguarded targets

Project Member Reported by xunji...@chromium.org, Aug 5 2016

Issue description

It might be beneficial to use base_proguard_config.flags instead of writing our flags in cronet/android/proguard.cfg. That way we can take advantage of the new proguard changes in base and do not need to keep everything in sync ourselves.

Ideally cronet/android/proguard.cfg will only contain proguard flags specific to Cronet and net.

 
Status: Started (was: Assigned)
Before patch (https://codereview.chromium.org/2214013002/):

CronetSample.apk dex count:
<root>: 1877
    a: 33
        a: 33
            a: 33
    android: 211
        accounts: 5
        animation: 6
        app: 15
        content: 49
            pm: 4
            res: 6
        net: 35
            http: 2
            wifi: 3
        os: 56
        preference: 1
        security: 1
        telephony: 17
        text: 1
        util: 17
        view: 5
        widget: 3
    dalvik: 1
        system: 1
    java: 358
        io: 57
        lang: 102
            reflect: 12
        math: 1
        net: 25
        nio: 32
            channels: 7
            charset: 6
        security: 24
            cert: 8
            interfaces: 2
            spec: 1
        text: 1
        util: 116
            concurrent: 26
                atomic: 16
            regex: 5
            zip: 16
    javax: 8
        net: 6
            ssl: 5
        security: 2
            auth: 2
                x500: 2
    org: 1266
        chromium: 1259
            base: 487
                library_loader: 48
                metrics: 22
                multidex: 4
            cronet_sample_apk: 32
            net: 740
                impl: 253
        json: 7
Overall method count: 1877

After patch:
Read in 1399 method IDs.
<root>: 1399
    android: 170
        accounts: 5
        app: 8
        content: 37
            pm: 4
            res: 5
        net: 35
            http: 2
            wifi: 3
        os: 50
        security: 1
        telephony: 17
        text: 1
        util: 11
        view: 2
        widget: 3
    java: 265
        io: 23
        lang: 75
            reflect: 3
        math: 1
        net: 25
        nio: 32
            channels: 7
            charset: 6
        security: 24
            cert: 8
            interfaces: 2
            spec: 1
        text: 1
        util: 84
            concurrent: 23
                atomic: 16
            regex: 4
    javax: 8
        net: 6
            ssl: 5
        security: 2
            auth: 2
                x500: 2
    org: 956
        chromium: 956
            base: 268
                library_loader: 39
                metrics: 16
            cronet_sample_apk: 32
            net: 656
                impl: 247
Overall method count: 1399


xunjieli@xunjieli:~/chrome/src$ stat out/r/apks/CronetSample.apk
  File: ‘out/r/apks/CronetSample.apk’
  Size: 1856591   	Blocks: 3632       IO Block: 4096   regular file

xunjieli@xunjieli:~/chrome/src$ stat out/r_proguard/apks/CronetSample.apk
  File: ‘out/r_proguard/apks/CronetSample.apk’
  Size: 1846551   	Blocks: 3608       IO Block: 4096   regular file


This will save us 1877 -1399 = 478 dex method counts and 1856591 - 1846551 = 10kB from CronetSample.apk.

Comment 2 by mef@chromium.org, Aug 8 2016

If we use base_proguard_config.flags then we need to ensure to include them into build package, so applications that depend on Cronet would use them as well.
Yep, totally agree. The CL (https://codereview.chromium.org/2214013002/) combines cronet proguard and base proguard flags into one file which is then included in cronet package.
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/761ef674d6498bad0f6c1bbdbbfcec59ffe5bafb

commit 761ef674d6498bad0f6c1bbdbbfcec59ffe5bafb
Author: xunjieli <xunjieli@chromium.org>
Date: Thu Aug 18 13:51:58 2016

Use keepclassmembers for android.webkit.JavascriptInterface in proguard

This CL uses -keepclassmembers for
android.webkit.JavascriptInterface instead of
-keepclasseswithmembers in proguard.

The reason is because keeping methods is enough for files
annotated with for android.webkit.JavascriptInterface.

BUG= 634998 

Review-Url: https://codereview.chromium.org/2250963004
Cr-Commit-Position: refs/heads/master@{#412818}

[modify] https://crrev.com/761ef674d6498bad0f6c1bbdbbfcec59ffe5bafb/base/android/base_proguard_config.flags

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eff981189bb608fb2125f3b514ab8a5634bc2c0a

commit eff981189bb608fb2125f3b514ab8a5634bc2c0a
Author: xunjieli <xunjieli@chromium.org>
Date: Wed Sep 07 21:51:18 2016

[Cronet] add base proguard flags

This CL pulls in base's proguard flags when compiling
cronet_sample_apk and cronet_package.

Cronet will not need to duplicate proguard flags and can
benefit from base proguard file changes.

This saves about 1882 - 1471 = 411 methods from our dex
count for CronetSample.apk and 10kB(4.35%).

BUG= 634998 

Review-Url: https://codereview.chromium.org/2214013002
Cr-Commit-Position: refs/heads/master@{#417060}

[modify] https://crrev.com/eff981189bb608fb2125f3b514ab8a5634bc2c0a/components/cronet/android/BUILD.gn
[modify] https://crrev.com/eff981189bb608fb2125f3b514ab8a5634bc2c0a/components/cronet/android/proguard.cfg
[modify] https://crrev.com/eff981189bb608fb2125f3b514ab8a5634bc2c0a/components/cronet/android/sample/javatests/proguard.cfg
[add] https://crrev.com/eff981189bb608fb2125f3b514ab8a5634bc2c0a/components/cronet/tools/generate_proguard_file.py

Cc: agrieve@chromium.org
Status: Fixed (was: Started)
I checked in the CL. However, there is still some work needed to move some app-level proguard flags out of base_proguard_config.flags.

The graphs tracking our Dex size is on perf dashboard logged under test suite "resource_sizes (CronetSample.apk)." Thanks to all the work done by agrieve@ and other Android folks.

Using base flags gives us a 20% saving on our Dex according to the perf dashboard.
https://chromeperf.appspot.com/report?sid=26f804155d6ac6e65a4ab5032217d9835ae87b93608abebb67839fef9e5d62f6&rev=417266
Woo! That's great!

Sign in to add a comment