Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support minification with better consumer proguard rules #2167

Open
ChrisCraik opened this issue Mar 11, 2024 · 3 comments
Open

Support minification with better consumer proguard rules #2167

ChrisCraik opened this issue Mar 11, 2024 · 3 comments
Assignees

Comments

@ChrisCraik
Copy link

Description

Currently, androidx.test doesn't bundle in sufficient proguard rules, which makes testing with minification difficult, especially in self-instrumenting tests which otherwise don't suffer from the test apk / app apk retention issues.

Note that AGP 8.3 recently added support for test minification

In androidx.benchmark we're looking to add these rules on our own for the time being, but it would be preferable and more generally useful to other libraries interested in minification if these could be handled directly by androidx.test.

Steps to Reproduce

Run sample, commenting out the testProguardRules in build.gradle

Note that sample includes simple case (ExampleTest) and more complex case (ExampleBenchmark). Benchmark gradle plugin already sets testBuildType="release"

Expected Results

Self-instrumenting com.android.library module tests don't require additional proguard rules by default

Actual Results

Several failures, requiring several workarounds in proguard file (not including those from b/328649293, which are an AGP issue):

### basic protection against junit/androidx.test reflection, shouldn't affect library/test code
# (fixes `Custom runner class AndroidJUnit4 should have a public constructor with signature AndroidJUnit4(Class testClass)`)
-keepclasseswithmembers class androidx.test.** { *; }
# not needed in this sample, but needed in practice in more complex samples in androidx repo
-keepclasseswithmembers class org.junit.** { *; }
# not needed in this sample, but needed in practice in more complex samples in androidx repo
-keepclasseswithmembers class junit.** { *; }
# (fixes `Missing classes detected while running R8.`)
-dontwarn com.google.errorprone.annotations.MustBeClosed

### keep test classes
# (fixes `Failed loading specified test class 'com.example.benchmark.ExampleTest'`)
-keepclasseswithmembers @org.junit.runner.RunWith class * { *; }

### needed for org.junit.Test annotation to be discoverable by reflection
# (fixes `Invalid test class 'com.example.benchmark.ExampleTest': No test methods found`)
-keepattributes *Annotation*

AndroidX Test and Android OS Versions

  • androidx.test.ext:junit:1.1.5
  • androidx.test:runner:1.5.2
  • Android OS version: N/A

The above rules could likely be made much more minimal, but in microbenchmark, we know androidx.test/junit/kotlin-test are never on the critical path, so we keep aggressively.

Link to a public git repo demonstrating the problem:

See attached repro project: androidxTestR8MicroSample.zip

@TWiStErRob
Copy link
Contributor

@ChrisCraik keeping is not just about speed, more about size. If you keep the whole of androidx.test, the deployment speed goes down significantly, because the DEX (and hence APK size) will be huge.

In some cases androidx.test is so large, that the tests themselves don't even fit in the first dex, so we need other keep rules (can't find the reference 😔) to keep tests and some test framework files in the first dex.

@implementor, please only keep what's absolutely necessary. And let R8 do its job for the rest.


On a related note, please don't use dontwarn on third party code. dontwarn only androidx.test classes. If another testing library (or my code!) uses com.google.errorprone.annotations.MustBeClosed I want to be warned about it, and make a choice myself.

@brettchabot
Copy link
Collaborator

@TWiStErRob are the size concerns from androidx.test itself or its dependencies? I took a quick look at the assembled apks from this repo's gradle-tests, and androidx.test.* itself contributes at most ~ 5K methods. Most androidx.test libraries should intentionally have a relatively lean set of dependencies. The assembled size of our espresso-core unit test apk is 1.6 MB for example. The outliers on dependency size are likely androidx.test.espresso:contrib, accessibility and androidx.test.ext:truth.
I don't have data to back me up but I would hope even with a broad keep rule like '-keep androidx.test.**' minification would still able to significantly reduce code from dependencies, because R8 would only keep code androidx.test actually uses.

That being said I think its reasonable to ask us to put a bit of work into ensuring we have a more targeted minimal set of keep rules.

Also correct me if I'm wrong but the first dex concerns are only relevant if you are targeting android platforms that do not support native multidex (APIS < 21). Currently androidx.test has a min sdk of 19, but bumping this up to 21 is on the not-too-far-off horizon.

@ChrisCraik
Copy link
Author

To clarify, I wasn't proposing an ideal set of rules, just a set that got things working immediately for my use case - microbenchmarking. With those rules, the attached sample comes out to 5K methods, and without them, 21K methods.

I mentioned speed instead of size because size isn't important to the use case of microbenchmarking (hot, compiled codepaths as opposed to e.g. startup), and by supporting R8 we (and users) can expect to see roughly 25% perf boost, so numbers get that much more accurate. It's this speed benefit that's motivating us to ship the suboptimal ruleset above, but note that benchmarks are in self-contained benchmark-only modules due to being compiled differently, so it's unlikely to affect non-benchmark scenarios.

I agree that a more minimal ruleset would be ideal, but those are easier to maintain and keep minimal if they're upstream in androidx.test

Minification disabled:
androidx_test_bench_unminified

Minification enabled (with rules above):
androidx_test_bench_minified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants