-
Notifications
You must be signed in to change notification settings - Fork 5k
Apply snappy compression to Configuration Cache entries #29643
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
Conversation
@bot-gradle test APT |
I've triggered the following builds for you. Click here to see all build failures. |
* Determines the maximum memory working set: [bufferCount] * [bufferCapacity]. | ||
* The default maximum working set is `32MB`. | ||
*/ | ||
val bufferCount = System.getProperty("org.gradle.configuration-cache.internal.buffer-count", null)?.toInt() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? 1 million buffers that are 32 bit long each? Normally, I'd expect the other way around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? 1 million buffers that are 32 bit long each? Normally, I'd expect the other way around.
Yes, perhaps the term buffer is not the best choice here. A better analogy might be of a packet
(like in UDP). These are used to quickly send the encoded information from the producer to the writer thread for compression >> encryption >> disk IO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abstratt Please take another look, I changed the code to the packet
analogy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
32 bit long
bytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now see that the graph walking in smaller builds is probably stuck on waiting for the writer thread to finish compression. We still may win something back by tweaking compression ratio, but I don't have better ideas.
...n/configuration-cache/src/main/kotlin/org/gradle/internal/cc/impl/io/ParallelOutputStream.kt
Outdated
Show resolved
Hide resolved
...n/configuration-cache/src/main/kotlin/org/gradle/internal/cc/impl/io/ParallelOutputStream.kt
Show resolved
Hide resolved
...n/configuration-cache/src/main/kotlin/org/gradle/internal/cc/impl/io/ParallelOutputStream.kt
Outdated
Show resolved
Hide resolved
} finally { | ||
// in case of failure, this releases some memory until the | ||
// producer realizes there was a failure | ||
ready.clear() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 neat!
...n/configuration-cache/src/main/kotlin/org/gradle/internal/cc/impl/io/ParallelOutputStream.kt
Outdated
Show resolved
Hide resolved
...n/configuration-cache/src/main/kotlin/org/gradle/internal/cc/impl/io/ParallelOutputStream.kt
Outdated
Show resolved
Hide resolved
...n/configuration-cache/src/main/kotlin/org/gradle/internal/cc/impl/io/ParallelOutputStream.kt
Show resolved
Hide resolved
...tion/configuration-cache/src/main/kotlin/org/gradle/internal/cc/impl/ConfigurationCacheIO.kt
Outdated
Show resolved
Hide resolved
...tion/configuration-cache/src/main/kotlin/org/gradle/internal/cc/impl/ConfigurationCacheIO.kt
Outdated
Show resolved
Hide resolved
3827678
to
9ac5192
Compare
} | ||
|
||
def "configuration cache encryption enablement is #enabled if kind=#kind"() { | ||
Assume.assumeTrue("Compression no longer exposes the raw data", enabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I'm not sure what to do with this test. @abstratt, do you think we still need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we are losing here is the ability of detecting a regression where a bug introduced to CC effectively disabled encryption.
We would need to make compression optional (even if behind an internal option), so we could disable it for this test case. May be a good thing to have (for debugging compression performance in the field), and seems more or less trivial to implement, but I will not push for it.
@bot-gradle test ACC |
I've triggered the following builds for you. Click here to see all build failures. |
61e4c1a
to
cb8df0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits
|
||
private | ||
fun flip(packet: Buffer) { | ||
// packet.flip() is not available in Java 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 I've just remembered the whole story here: Buffer flip()
is in Java since NIO's inception, and therefore available for ByteBuffer
as well. A covariant override ByteBuffer flip()
is a new addition of Java 9, and it is what trips the compiler over. I guess, with the typing you have here packet.flip()
should already work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can see that now.
|
||
|
||
private | ||
typealias Packet = java.nio.ByteBuffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 TBH, I find the code with this alias less readable. As a Java developer I know what a ByteBuffer
can do, but now have to jump an extra hop to connect the Packet
name to it. I don't insist on changing this.
💅 I wonder if chunk
is a better name for the pieces we're tossing around here, because a packet somewhat implies having metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunk
seems like a better name indeed, thanks. I also agree about the alias.
6991ec1
to
d4385f1
Compare
d4385f1
to
8305de5
Compare
- Make it easier to run a specific set of benchmarks
Based on benchmark results.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. If you don't want the stale bot to close it, then set a milestone for it. |
Apply snappy compression to Configuration Cache entries
Compression and encryption are offloaded to a separate thread, utilizing a pool of byte buffers capped at a maximum memory working set of 16MB.
The resulting files are 1/5th of their original size. Storing might be marginally slower in some cases, but loading is as fast or faster in all tested scenarios.
Reviewing cheatsheet
Before merging the PR, comments starting with