IssueTracker

With the new API(withVariantReselection), We suspect it is not GC ed after the resolution. GC seems to do a better job for artifactResolutionQuery (https://cs.android.com/android-studio/platform/tools/adt/idea/+/mirror-goog-studio-main:gradle-tooling/studio-gradle-tooling-impl/src/com/android/ide/gradle/model/artifacts/builder/AdditionalClassifierArtifactsModelBuilder.kt;l=101?q=AdditionalClassifierArtifactsModelBuilder) and detached configuration (https://cs.android.com/android-studio/platform/tools/adt/idea/+/mirror-goog-studio-main:gradle-tooling/studio-gradle-tooling-impl/src/com/android/ide/gradle/model/artifacts/builder/AdditionalClassifierArtifactsModelBuilder.kt;l=123?q=AdditionalClassifierArtifactsModelBuilder)

I managed to produce some consistent numbers and here is what I did.

To get some numbers of the old way(artifactResolutionQuery & detachedConfiguration) of resolving javadoc/source/samples

1. open the android-extra-large-benchmark project(the 200 module version) with dev versions of AGP and Android Studio, set up the studio flag GRADLE_HEAP_ANALYSIS_OUTPUT_DIRECTORY to the path of hprof output directory
2. sync the project and close android studio
3. reopen android studio, kill all daemon with ./gradlew --stop
4. click the sync button
5. open the two hprof files(one before sync and one after sync)

I got 458MB for the retained size of strong reachable before sync and 912MB after sync. Repeat step 2-5 again and I got 463MB before sync and 917 after sync.

To get data of using the new way(withVariantReselection) of resolving javadoc/source/samples

6. enable the new feature with AGP flag(android.experimental.additionalArtifactsInModel=true) and comment out the old resolution code for building the AdditionalClassifierArtifactsModelBuilder
7. re-build android studio
8. open the same benchmark project and repeart step 2-5

I got 458MB before sync and 948MB after sync for the first iteration, and 458MB before sync vs 948 after sync for the second iteration.

So basically we are able to reproduce the memory regression 910MB->948MB Ivan, Ahmet, does the above approach sound good?

A few things worth mentioning

1. re-open studio is to make sure no studio cache for javadoc/source is used
2. As we capture gradle daemon memory before and after sync, killing daemon before the sync seems to be a good way to have consistent result
3. With 1 and 2, it is safe to say only gradle persistent cache could be used for javadoc/source/sample resolution.

To allow Gradle folks to reproduce like the way I did, I think we need to land Bradley's CL so they can disable the old approach.

The initial motivation of switching to this new API is 1) support resolving javadoc/source/sample published with multiple variants(e.g. debug/release) 2) to have a persistent cache for javadoc/source/sample instead of relying on studio cache. The second point is actually addressed by Gradle regardless of using this API or not. Although artifactResolutionQuery could not let us add attributes, but in theory detachedConfiguration allows that.

So my question to Grade folks is, do you think we could reduce the memory usage when using withVariantReselection api? If not, what is the advantage of using withVariantReselection over detachedConfiguration?

Some updates: as Bradley's change is checked in, Gradle team should be able to reproduce and get some numbers without making changes to source code.

Can you please file an internal bug and use Bugjuggler to block on the Gradle issue?

I don't think there is an Gradle issue filed for that. I need to sync with Ahmet to capture the conversations between AGP and Gradle.

Recently I was investigating a memory regression b/303789366 that was caused by some changes that migrates the javadoc/source fetching to artifactView that is quite related with the mentioned bug. However, during those tests we spotted improvement around artifactview on Gradle 8.4-rc-2 that can be appreciated on the bellow results:

• Gradle 8.3:
  • android.experimental.additionalArtifactsInModel=true (~12.73 GB sync memory heap)
    • https://fusion2.corp.google.com/invocations/bdc73629-3af8-49e0-9ef4-8f5ccc5aa036
• Gradle 8.4:
  • android.experimental.additionalArtifactsInModel=true (~11.15 GB sync memory heap)
    • https://fusion2.corp.google.com/invocations/ac0a2060-63f1-42f8-99de-6074bbee5b15
  • android.experimental.additionalArtifactsInModel=false (~11.15 GB sync memory heap)
    • https://fusion2.corp.google.com/invocations/2408c39d-fda9-44da-8fd6-718ee3532ee5

Those tests have been done using our android-extra-large-benchmark sample project with 2K modules. Probably with all of this we can consider closing the ticket or if you find necessary testing again following the described steps with gradle 8.4-rc-2

Thanks Madalin for the help and sorry for the late reply. This week, I am going to test it out using the step mentioned above and share the result.

Running bazel test //tools/adt/idea/sync-memory-tests:intellij.android.sync-memory-tests_tests__Benchmark2000Memory --config=remote with latest AGP and Gradle 8.4

Current set up:

android.experimental.additionalArtifactsInModel=false and GradleExperimentalSettings#USE_MULTI_VARIANT_EXTRA_ARTIFACTS=false

2000Modules_Sync_Finished retained heap size about 8.1G

https://fusion2.corp.google.com/invocations/032b67be-0572-40d1-a30b-353d4c846b7d/targets/%2F%2Ftools%2Fadt%2Fidea%2Fsync-memory-tests:intellij.android.sync-memory-tests_tests__Benchmark2000Memory/artifacts

Switch to new pipeline:

android.experimental.additionalArtifactsInModel=true and GradleExperimentalSettings#USE_MULTI_VARIANT_EXTRA_ARTIFACTS=true (changing default value of those two properties)

2000Modules_Sync_Finished retained heap size about 8.7G

https://fusion2.corp.google.com/invocations/454a8567-d365-4ea4-a757-bbe3300445c7/targets/%2F%2Ftools%2Fadt%2Fidea%2Fsync-memory-tests:intellij.android.sync-memory-tests_tests__Benchmark2000Memory/tests

Therefore, I don't think the regression is fixed on Gradle side.

Note GradleExperimentalSettings#USE_MULTI_VARIANT_EXTRA_ARTIFACTS controls the behavior on the sync side, setting it to true means we would just use the resolved artifacts provided from the model instead of doing fetching from the sync side. However, I notice setting it to false doesn't increase memory too much on top of setting android.experimental.additionalArtifactsInModel to true.

Based on the discussion in this slack channel https://gradle.slack.com/archives/C10J6STFZ/p1679955439030089, Gradle folks mentioned "When you use detached configurations we should GC resolution results after resolution is performed". Currently, we mostly use detached configuration https://source.corp.google.com/h/googleplex-android/platform/superproject/base/+/studio-main:tools/adt/idea/gradle-tooling/studio-gradle-tooling-impl/src/com/android/ide/gradle/model/artifacts/builder/AdditionalClassifierArtifactsModelBuilder.kt;l=133?q=repo:googleplex-android%2Fplatform%2Fsuperproject%2Fbase%20b:studio-main%20AdditionalClassifierArtifactsModelBuilder

And also they mentioned "We are currently discussing ways to get gradle to release resolved dependency graphs by allowing users to declare dependency resolution as inputs, therefore allowing gradle to know if a given configurations graph is no longer necessary. Though this is still in the works"

I think the memory increase comes from not releasing resolved dependency graph when using withVariantReselection.(This may not apply to previous AGP version when we don't use detachedConfiguration)

The ticket filed to Gradle https://github.com/gradle/gradle/issues/28606

As suggested by Gradle folks, I collect some CPU and memory data using Gradle 8.12 with the feature turned on. I checked with Ahmet and the result is promising. We plan to turn on the multi variant javadoc/source support feature by default after Gradle 8.12 upgrade and carefully monitor the performance impact.