Deduplicate strings stored in the configuration cache #29950

bamboo · 2024-07-18T03:53:01Z

With this change, the configuration cache entry for assembleDebug on perf-android-large-2 is 74% smaller (356M against 1.3G) and it runs ~30% faster on a cache hit:

Gradle performance tests also show improvement.

Reviewing cheatsheet

Before merging the PR, comments starting with

❌ ❓must be fixed
🤔 💅 should be fixed
💭 may be fixed
🎉 celebrate happy things

bamboo · 2024-07-18T03:53:22Z

@bot-gradle test ACC without PTS

bot-gradle · 2024-07-18T03:53:25Z

I've triggered the following builds with parameters: -DenablePredictiveTestSelection=false for you. Click here to see all build failures.

AllConfigCacheTestsPullRequestFeedback build

mlopatkin

Awesome results! I've left a few questions before approving.

mlopatkin · 2024-07-18T18:42:10Z

...n/src/main/java/org/gradle/internal/serialize/kryo/StringDeduplicatingKryoBackedEncoder.java

❓ This is also used for Dependency Engine cache data. It looks like this isn't cross-version, but can you confirm, just in case?

Yes, it seems to be only used for temporary caches here but I'm confirming with bt-dependency-management just in case.

mlopatkin · 2024-07-18T18:51:16Z

...n/src/main/java/org/gradle/internal/serialize/kryo/StringDeduplicatingKryoBackedEncoder.java

+         */
+        Integer newIndex = strings.size() + 2;
+        strings.put(key, newIndex);
+        writeStringIndex(1);


❌ Can we have a constants for 0 and 1 instead of these magic numbers? E.g. NULL_STRING_INDEX = 0 and NEW_STRING = 1.

mlopatkin · 2024-07-18T19:05:15Z

...n/src/main/java/org/gradle/internal/serialize/kryo/StringDeduplicatingKryoBackedDecoder.java

-                string = strings[idx];
+            int index = readStringIndex();
+            switch (index) {
+                case 0:


🤔 One potential trick: ATM we're wasting two first elements of the array. We can avoid this and replace switch with a single if, though I'm not sure if it affects performance much:

Let's use 0 for new string and 1 for null

Have null in the initial array: INITIAL_CAPACITY_MARKER = new String [] { null }.

Now we can change the switch into

if (index == 0) return readNewString(); return strings[index - 1];

But I guess this goes to microbenchmarking territory.

Interesting idea! I went with a variation that avoids the subtraction on every read.

On a second thought, using the two constants on both encoder and decoder makes the connection clearer and it doesn't seem to be a clear performance advantage either way.

mlopatkin · 2024-07-18T19:07:16Z

...n/src/main/java/org/gradle/internal/serialize/kryo/StringDeduplicatingKryoBackedDecoder.java

 * stream.
 */
 public class StringDeduplicatingKryoBackedDecoder extends AbstractDecoder implements Decoder, Closeable {
    public static final int INITIAL_CAPACITY = 32;


❔ Looks like it can be private too.

mlopatkin · 2024-07-18T19:17:12Z

...egTest/groovy/org/gradle/internal/cc/impl/ConfigurationCacheEncryptionIntegrationTest.groovy

+     * @see com.esotericsoftware.kryo.io.Output#writeString(java.lang.String)
+     */
+    @SuppressWarnings('GrDeprecatedAPIUsage')
+    private static byte[] encodedBytesOf(String string) {


🤔 TBH, I think this test now knows too much of the encoding details and has become fragile.

Indeed. It can still be useful and less fragile by having the asserts ignoring the last character but I'd deal with it later once we introduce compression.

mlopatkin · 2024-07-18T19:23:19Z

...oovy/org/gradle/internal/cc/impl/ConfigurationCacheStringDeduplicationIntegrationTest.groovy

+            }
+
+            def service = gradle.sharedServices.registerIfAbsent('stringChecker', StringDedupCheckerService) {}
+            allprojects {


❓ Is this test IP-incompatible? It can be a settings script that uses the new lifecycle API.
❓ With parallel project saving, will string deduplication work across project? Or is the goal to have it across projects?

Is this test IP-incompatible? It can be a settings script that uses the new lifecycle API.

Right, good point.

With parallel project saving, will string deduplication work across project? Or is the goal to have it across projects?

Yes, that would be the idea. We discussed having all the strings handled by a separate String Pool worker, let's see if we can make that fly.

mlopatkin · 2024-07-18T19:25:21Z

...oovy/org/gradle/internal/cc/impl/ConfigurationCacheStringDeduplicationIntegrationTest.groovy

+                    if (string === null) {
+                        string = s
+                    } else {
+                        assert string === s


bamboo · 2024-07-18T19:47:51Z

@bot-gradle test RFN without PTS

bot-gradle · 2024-07-18T19:47:58Z

I've triggered the following builds with parameters: -DenablePredictiveTestSelection=false for you. Click here to see all build failures.

ReadyForNightly build

bamboo · 2024-07-18T23:37:19Z

Thanks for the review, @mlopatkin, please take another look.

pablobaxter · 2024-07-19T06:51:20Z

I only did a size check of the config cache on the 6200+ project I am working on. Results are as follows:
The config cache with 8.5 (current version) is ~759.2MB.
The config cache with the 8.10 nightly is ~776.9MB.
The config cache with the local build (8.10) is ~232.2MB.

For ~70% smaller configuration cache entries, faster loading times and potentially reduced memory usage on cache hits.

- write string indexes as small ints for reduced file sizes (~10% smaller) - avoid `null` check on every `readNullableString()` operation

So `getWritePosition` can be polymorphically exposed from `KryoBackedEncoder` and `StringDeduplicatingKryoBackedEncoder`. ./gradlew :architecture-test:test -ParchunitRefreeze

And use it to restore `ConfigurationCacheEncryptionIntegrationTest` to its former behavior.

bot-gradle · 2024-07-19T17:35:49Z

WARN: Based on labels, this pull request addresses notable issue but no changes to release note found.

bot-gradle · 2024-07-19T20:01:52Z

The merge queue build has failed. Click here to see all failures.

joshfriend · 2024-08-13T11:34:56Z

I did a configuration phase benchmark in our build to test the performance against 8.6:

configure_only {
    warm-ups = 3
    iterations = 20
    versions = ["8.6", "8.10-rc-1"]
    tasks = [
        ":apps:some-app:app:assembleDebug"
    ]
    gradle-args = [
        "--dry-run",
    ]
    clear-configuration-cache-state-before = BUILD
}

Then a CC load benchmark (same scenario but with clear-configuration-cache-state-before = BUILD removed:

CC size for this 3700 project build was 718MB in 8.6, and in 8.10-rc-1 it is 212MB!

bamboo · 2024-08-19T11:42:09Z

Thanks for sharing the data, @joshfriend! That's great to see 👍

bamboo self-assigned this Jul 18, 2024

This comment has been minimized.

Sign in to view

bamboo force-pushed the bamboo/cc/dedupe-strings branch from 52c9ece to 2f372c4 Compare July 18, 2024 17:40

bamboo changed the title ~~WIP~~ Deduplicate strings stored to the configuration cache Jul 18, 2024

bamboo changed the title ~~Deduplicate strings stored to the configuration cache~~ Deduplicate strings stored in the configuration cache Jul 18, 2024

bamboo marked this pull request as ready for review July 18, 2024 17:41

bamboo requested a review from a team July 18, 2024 17:41

bamboo requested a review from a team as a code owner July 18, 2024 17:41

bamboo requested review from abstratt and alllex and removed request for a team July 18, 2024 17:41

bamboo added in:configuration-cache Configuration Caching a:performance-improvement Performance improvement potential labels Jul 18, 2024

bamboo added this to the 8.10 RC1 milestone Jul 18, 2024

bamboo mentioned this pull request Jul 18, 2024

Memory Improvements based on findings in >6k project build #29947

Closed

9 tasks

mlopatkin requested changes Jul 18, 2024

View reviewed changes

bamboo force-pushed the bamboo/cc/dedupe-strings branch from 62d1ba8 to 2000402 Compare July 18, 2024 23:36

mlopatkin approved these changes Jul 19, 2024

View reviewed changes

bamboo requested a review from a team as a code owner July 19, 2024 17:21

bamboo added 4 commits July 19, 2024 14:32

Deduplicate strings written to the configuration cache

9bf6fad

For ~70% smaller configuration cache entries, faster loading times and potentially reduced memory usage on cache hits.

Optimize StringDeduplicatingKryoBacked(En|De)coder

9f995d4

- write string indexes as small ints for reduced file sizes (~10% smaller) - avoid `null` check on every `readNullableString()` operation

Make ConfigurationCacheDeduplicationIntegrationTest IP compatible

409edd9

Introduce constants for NULL_STRING and NEW_STRING encodings

f0bc896

bamboo added 2 commits July 19, 2024 14:32

Introduce PositionAwareEncoder

e035e37

So `getWritePosition` can be polymorphically exposed from `KryoBackedEncoder` and `StringDeduplicatingKryoBackedEncoder`. ./gradlew :architecture-test:test -ParchunitRefreeze

Introduce internal flag to allow disabling string deduplication

7688645

And use it to restore `ConfigurationCacheEncryptionIntegrationTest` to its former behavior.

bamboo force-pushed the bamboo/cc/dedupe-strings branch from c37d777 to 7688645 Compare July 19, 2024 17:32

bamboo added this pull request to the merge queue Jul 19, 2024

Merged via the queue into master with commit 22b1905 Jul 19, 2024

bamboo deleted the bamboo/cc/dedupe-strings branch July 19, 2024 20:03

ov7a added the pending:release-notes Indicates that the issue requires a release notes entry label Jul 24, 2024

ov7a removed the pending:release-notes Indicates that the issue requires a release notes entry label Sep 30, 2024

Deduplicate strings stored in the configuration cache #29950

Deduplicate strings stored in the configuration cache #29950

Uh oh!

Conversation

bamboo commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewing cheatsheet

Uh oh!

This comment has been minimized.

This comment has been minimized.

bamboo commented Jul 18, 2024

Uh oh!

bot-gradle commented Jul 18, 2024

Uh oh!

mlopatkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bamboo Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bamboo commented Jul 18, 2024

Uh oh!

bot-gradle commented Jul 18, 2024

Uh oh!

bamboo commented Jul 18, 2024

Uh oh!

pablobaxter commented Jul 19, 2024

Uh oh!

bot-gradle commented Jul 19, 2024

Uh oh!

bot-gradle commented Jul 19, 2024

Uh oh!

joshfriend commented Aug 13, 2024

Uh oh!

bamboo commented Aug 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bamboo commented Jul 18, 2024 •

edited

Loading

bamboo Jul 18, 2024 •

edited

Loading