diff --git a/CHANGELOG.md b/CHANGELOG.md index e1329602f..0fc6c858b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,9 @@ - `XmlAppendingTransformer` - Append terminating newline in `ServiceFileTransformer`. ([#2202](https://github.com/GradleUp/shadow/pull/2202)) - Remove redundant JAR normalization for R8 output. ([#2236](https://github.com/GradleUp/shadow/pull/2236)) +- Parallelize bytecode remapping. ([#2302](https://github.com/GradleUp/shadow/pull/2302)) + Custom `Relocator` implementations must now be thread-safe. + Significantly improves `shadowJar` execution performance when relocating classes (up to ~4.6x faster on large dependencies). ### Deprecated diff --git a/build.gradle.kts b/build.gradle.kts index ae2dcc51a..50bf1a68d 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -131,6 +131,7 @@ dependencies { compileOnly(libs.develocity) compileOnly(libs.kotlin.gradlePlugin) compileOnly(libs.kotlin.reflect) + compileOnly(libs.kotlinx.coroutines) api(libs.apache.ant) // Types from Ant are exposed in the public API. implementation(libs.apache.log4j) implementation(libs.jdependency) @@ -152,7 +153,10 @@ dependencies { testing.suites { named("test") { - dependencies { implementation(libs.xmlunit) } + dependencies { + implementation(libs.kotlinx.coroutines) + implementation(libs.xmlunit) + } } register("documentTest") { targets.configureEach { diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 1b9adc80c..485a40ced 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -12,6 +12,7 @@ jdependency = "org.vafer:jdependency:2.16" jdom2 = "org.jdom:jdom2:2.0.6.1" kotlin-metadata = { module = "org.jetbrains.kotlin:kotlin-metadata-jvm", version.ref = "kotlin" } kotlin-reflect = { module = "org.jetbrains.kotlin:kotlin-reflect", version.ref = "kotlin" } +kotlinx-coroutines = "org.jetbrains.kotlinx:kotlinx-coroutines-core:1.10.1" plexus-utils = "org.codehaus.plexus:plexus-utils:4.1.0" plexus-xml = "org.codehaus.plexus:plexus-xml:4.2.0" xmlunit = "org.xmlunit:xmlunit-legacy:2.13.0" diff --git a/src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/ParallelRelocationTest.kt b/src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/ParallelRelocationTest.kt new file mode 100644 index 000000000..57563cdb1 --- /dev/null +++ b/src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/ParallelRelocationTest.kt @@ -0,0 +1,157 @@ +package com.github.jengelman.gradle.plugins.shadow + +import assertk.assertThat +import assertk.assertions.contains +import assertk.assertions.isEqualTo +import com.github.jengelman.gradle.plugins.shadow.testkit.classLoader +import com.github.jengelman.gradle.plugins.shadow.testkit.containsExactly +import com.github.jengelman.gradle.plugins.shadow.testkit.getContent +import com.github.jengelman.gradle.plugins.shadow.testkit.loadClass +import kotlin.io.path.appendText +import kotlin.io.path.readBytes +import org.junit.jupiter.api.Test + +class ParallelRelocationTest : BasePluginTest() { + @Test + fun largeNumberOfClassesWithRelocation() { + val count = 500 + val classNames = (1..count).map { "Class%03d".format(it) } + val largeJar = + buildJar("many-classes.jar") { + for (name in classNames) { + insert( + "com/example/pkg/$name.class", + createEmptyClassBytes("com/example/pkg/$name"), + ) + } + } + + projectScript.appendText( + """ + |dependencies { + | ${implementationFiles(largeJar)} + |} + |$shadowJarTask { + | relocate 'com.example.pkg', 'relocated.pkg' + |} + """ + .trimMargin() + ) + + runWithSuccess(shadowJarPath) + + assertThat(outputShadowedJar).useAll { + val relocatedEntries = classNames.map { "relocated/pkg/$it.class" }.toTypedArray() + containsExactly( + "META-INF/MANIFEST.MF", + *relocatedEntries, + "META-INF/", + "relocated/pkg/", + "relocated/", + ) + classLoader { + loadClass("relocated.pkg.${classNames.first()}") + loadClass("relocated.pkg.${classNames.last()}") + } + } + } + + @Test + fun deterministicZipEntryOrderAcrossMultipleBuilds() { + val count = 150 + val testJar = + buildJar("deterministic-test.jar") { + for (i in 1..count) { + insert( + "com/example/test/TestClass$i.class", + createEmptyClassBytes("com/example/test/TestClass$i"), + ) + insert("resources/res_$i.txt", "content $i") + } + } + + projectScript.appendText( + """ + |dependencies { + | ${implementationFiles(testJar)} + |} + |$shadowJarTask { + | relocate 'com.example.test', 'shadowed.example.test' + |} + """ + .trimMargin() + ) + + runWithSuccess(shadowJarPath) + val firstBytes = path("build/libs/my-1.0-all.jar").readBytes() + + runWithSuccess(shadowJarPath, "--rerun-tasks") + val secondBytes = path("build/libs/my-1.0-all.jar").readBytes() + + assertThat(firstBytes).isEqualTo(secondBytes) + } + + @Test + fun resourcesAndUnrelocatedClassesPreserved() { + val resourceContent = "A".repeat(10_000) + val testJar = + buildJar("mixed-entries.jar") { + insert( + "com/example/relocated/RelocatedClass.class", + createEmptyClassBytes("com/example/relocated/RelocatedClass"), + ) + insert( + "com/example/untouched/UntouchedClass.class", + createEmptyClassBytes("com/example/untouched/UntouchedClass"), + ) + insert("assets/large-resource.txt", resourceContent) + } + + projectScript.appendText( + """ + |dependencies { + | ${implementationFiles(testJar)} + |} + |$shadowJarTask { + | relocate 'com.example.relocated', 'shadowed.example.relocated' + |} + """ + .trimMargin() + ) + + runWithSuccess(shadowJarPath) + + assertThat(outputShadowedJar).useAll { + getContent("assets/large-resource.txt").isEqualTo(resourceContent) + classLoader { + loadClass("shadowed.example.relocated.RelocatedClass") + loadClass("com.example.untouched.UntouchedClass") + } + } + } + + @Test + fun errorPropagationWhenClassIsCorrupted() { + val badClassEntry = "corrupt/BadClass.class" + val corruptJar = + buildJar("corrupt.jar") { + insert(badClassEntry, byteArrayOf(0xCA.toByte(), 0xFE.toByte(), 0xBA.toByte())) + } + + projectScript.appendText( + """ + |dependencies { + | ${implementationFiles(corruptJar)} + |} + |$shadowJarTask { + | relocate 'corrupt', 'relocated.corrupt' + |} + """ + .trimMargin() + ) + + val result = runWithFailure(shadowJarPath) + + assertThat(result.output).contains("Error in ASM processing class $badClassEntry") + } +} diff --git a/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/GradleCompat.kt b/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/GradleCompat.kt index a565aa0da..f6c317801 100644 --- a/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/GradleCompat.kt +++ b/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/GradleCompat.kt @@ -74,6 +74,8 @@ internal fun FileTreeElement.inputStream(): InputStream = file.inputStream() } +internal fun FileTreeElement.readBytes(): ByteArray = inputStream().use(InputStream::readBytes) + internal inline fun ObjectFactory.property( defaultValue: Any? = null ): Property = diff --git a/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/RelocatorRemapper.kt b/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/RelocatorRemapper.kt index 0c33dbb28..2e93f03d4 100644 --- a/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/RelocatorRemapper.kt +++ b/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/RelocatorRemapper.kt @@ -13,28 +13,29 @@ import org.vafer.jdeb.shaded.objectweb.asm.commons.Remapper * (possibly) remapped class bytes. If no remapping is required, the original bytes are returned. */ internal fun FileCopyDetails.remapClass(relocators: Set): ByteArray = - inputStream() - .use { it.readBytes() } - .let { bytes -> - var modified = false - val remapper = RelocatorRemapper(relocators) { modified = true } + readBytes().remapClass(relocators = relocators, path = path) - // We don't pass the ClassReader here. This forces the ClassWriter to rebuild the constant - // pool. Copying the original constant pool should be avoided because it would keep references - // to the original class names. This is not a problem at runtime (because these entries in the - // constant pool are never used), but confuses some tools such as Felix's maven-bundle-plugin - // that use the constant pool to determine the dependencies of a class. - try { - val cw = ClassWriter(0) - val cr = ClassReader(bytes) - val cv = ClassRemapper(cw, remapper) - cr.accept(cv, ClassReader.EXPAND_FRAMES) - // If we didn't need to change anything, keep the original bytes as-is. - if (modified) cw.toByteArray() else bytes - } catch (t: Throwable) { - gradleError("Error in ASM processing class $path", t) - } +internal fun ByteArray.remapClass(relocators: Set, path: String): ByteArray = + let { bytes -> + var modified = false + val remapper = RelocatorRemapper(relocators) { modified = true } + + // We don't pass the ClassReader here. This forces the ClassWriter to rebuild the constant + // pool. Copying the original constant pool should be avoided because it would keep references + // to the original class names. This is not a problem at runtime (because these entries in the + // constant pool are never used), but confuses some tools such as Felix's maven-bundle-plugin + // that use the constant pool to determine the dependencies of a class. + try { + val cw = ClassWriter(0) + val cr = ClassReader(bytes) + val cv = ClassRemapper(cw, remapper) + cr.accept(cv, ClassReader.EXPAND_FRAMES) + // If we didn't need to change anything, keep the original bytes as-is. + if (modified) cw.toByteArray() else bytes + } catch (t: Throwable) { + gradleError("Error in ASM processing class $path", t) } + } private class RelocatorRemapper( private val relocators: Set, diff --git a/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/relocation/Relocator.kt b/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/relocation/Relocator.kt index 91d5953b2..cc966785a 100644 --- a/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/relocation/Relocator.kt +++ b/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/relocation/Relocator.kt @@ -8,6 +8,9 @@ import org.gradle.api.tasks.Input * Modified from * [org.apache.maven.plugins.shade.relocation.Relocator.java](https://github.com/apache/maven-shade-plugin/blob/master/src/main/java/org/apache/maven/plugins/shade/relocation/Relocator.java). * + * Implementations of [Relocator] must be thread-safe, as their methods may be invoked concurrently + * across multiple worker threads during parallel bytecode remapping. + * * @author Jason van Zyl * @author John Engelman */ diff --git a/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/tasks/ShadowCopyAction.kt b/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/tasks/ShadowCopyAction.kt index e3dd181d1..bfcdfb066 100644 --- a/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/tasks/ShadowCopyAction.kt +++ b/src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/tasks/ShadowCopyAction.kt @@ -9,6 +9,7 @@ import com.github.jengelman.gradle.plugins.shadow.internal.entries import com.github.jengelman.gradle.plugins.shadow.internal.gradleError import com.github.jengelman.gradle.plugins.shadow.internal.inputStream import com.github.jengelman.gradle.plugins.shadow.internal.parentDirectoryEntries +import com.github.jengelman.gradle.plugins.shadow.internal.readBytes import com.github.jengelman.gradle.plugins.shadow.internal.remapClass import com.github.jengelman.gradle.plugins.shadow.internal.writeEntry import com.github.jengelman.gradle.plugins.shadow.relocation.Relocator @@ -16,6 +17,11 @@ import com.github.jengelman.gradle.plugins.shadow.relocation.relocatePath import com.github.jengelman.gradle.plugins.shadow.transformers.ResourceTransformer import com.github.jengelman.gradle.plugins.shadow.transformers.TransformerContext import java.io.File +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Deferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.async +import kotlinx.coroutines.runBlocking import org.apache.tools.zip.Zip64RequiredException import org.apache.tools.zip.ZipOutputStream import org.gradle.api.file.FileCopyDetails @@ -73,7 +79,12 @@ internal constructor( override fun execute(stream: CopyActionProcessingStream): WorkResult { try { zipOutStream.use { zos -> - stream.process(StreamAction(zos)) + runBlocking { + val action = StreamAction(zos, this) + stream.process(action) + action.flush() + } + processTransformers(zos) addDirs(zos) checkDuplicateEntries(zos) @@ -150,8 +161,12 @@ internal constructor( } } - private inner class StreamAction(private val zipOutStr: ZipOutputStream) : - CopyActionProcessingStreamAction { + private inner class StreamAction( + private val zipOutStr: ZipOutputStream, + private val scope: CoroutineScope, + ) : CopyActionProcessingStreamAction { + private val pendingEntries = ArrayDeque() + init { logger.info("Relocator count: {}.", relocators.size) } @@ -180,20 +195,40 @@ internal constructor( val multiReleasePrefix = multiReleaseRegex.find(path)?.value.orEmpty() val pathSuffix = path.removePrefix(multiReleasePrefix) val relocatedPath = multiReleasePrefix + relocators.relocatePath(pathSuffix) - fileDetails.writeToZip( - entryName = relocatedPath, - bytes = fileDetails.remapClass(relocators = relocators), + val rawBytes = fileDetails.readBytes() + val deferred = + scope.async(Dispatchers.Default) { + rawBytes.remapClass(relocators = relocators, path = path) + } + pendingEntries.addLast( + PendingEntry( + entryName = relocatedPath, + fileDetails = fileDetails, + deferredBytes = deferred, + ) ) + if (pendingEntries.size >= MAX_PENDING_ENTRIES) { + runBlocking { pendingEntries.removeFirst().writeToZip() } + } } } else -> { val relocated = relocators.relocatePath(path) if (transform(fileDetails, relocated)) return + if (pendingEntries.isNotEmpty()) { + runBlocking { flush() } + } fileDetails.writeToZip(relocated) } } } + suspend fun flush() { + while (pendingEntries.isNotEmpty()) { + pendingEntries.removeFirst().writeToZip() + } + } + private fun isUnused(classPath: String): Boolean { val className = classPath.substringBeforeLast(".").replace('/', '.') return unusedClasses.contains(className).also { @@ -214,6 +249,10 @@ internal constructor( return true } + private suspend fun PendingEntry.writeToZip() { + fileDetails.writeToZip(entryName, deferredBytes.await()) + } + private fun FileCopyDetails.writeToZip(entryName: String, bytes: ByteArray? = null) { zipOutStr.writeEntry( name = entryName, @@ -230,10 +269,22 @@ internal constructor( } } + private class PendingEntry( + val entryName: String, + val fileDetails: FileCopyDetails, + val deferredBytes: Deferred, + ) + public companion object { private val logger = Logging.getLogger(@Suppress("DEPRECATION") ShadowCopyAction::class.java) private val multiReleaseRegex = "^META-INF/versions/\\d+/".toRegex() + /** + * Maximum number of in-flight parallel class remapping tasks in the sliding window. Bounds + * memory consumption on large archives while keeping worker threads saturated. + */ + private const val MAX_PENDING_ENTRIES = 128 + @Deprecated( message = "Use `ShadowJar.CONSTANT_TIME_FOR_ZIP_ENTRIES` constant instead. This will be removed in Shadow 10.", diff --git a/src/test/kotlin/com/github/jengelman/gradle/plugins/shadow/tasks/ShadowCopyActionTest.kt b/src/test/kotlin/com/github/jengelman/gradle/plugins/shadow/tasks/ShadowCopyActionTest.kt index 53d7b5e00..f0cf101e1 100644 --- a/src/test/kotlin/com/github/jengelman/gradle/plugins/shadow/tasks/ShadowCopyActionTest.kt +++ b/src/test/kotlin/com/github/jengelman/gradle/plugins/shadow/tasks/ShadowCopyActionTest.kt @@ -5,14 +5,26 @@ import assertk.assertThat import assertk.assertions.hasMessage import assertk.assertions.isEqualTo import assertk.assertions.isInstanceOf +import assertk.assertions.isLessThan import assertk.assertions.isTrue import com.github.jengelman.gradle.plugins.shadow.internal.createZipOutputStream -import com.github.jengelman.gradle.plugins.shadow.internal.useZip +import com.github.jengelman.gradle.plugins.shadow.internal.remapClass +import com.github.jengelman.gradle.plugins.shadow.relocation.Relocator +import com.github.jengelman.gradle.plugins.shadow.relocation.SimpleRelocator +import com.github.jengelman.gradle.plugins.shadow.testkit.JarPath +import com.github.jengelman.gradle.plugins.shadow.testkit.containsExactly +import com.github.jengelman.gradle.plugins.shadow.testkit.requireResourceAsPath import com.github.jengelman.gradle.plugins.shadow.util.noOpDelegate -import java.io.ByteArrayInputStream import java.io.File import java.io.InputStream import java.io.OutputStream +import kotlin.io.path.readBytes +import kotlin.time.Duration +import kotlin.time.measureTime +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.runBlocking import org.apache.tools.zip.UnixStat import org.apache.tools.zip.Zip64RequiredException import org.gradle.api.file.FilePermissions @@ -28,6 +40,8 @@ import org.junit.jupiter.api.io.TempDir @Suppress("DEPRECATION") class ShadowCopyActionTest { + private val classEntry = "${ShadowCopyActionTest::class.java.name.replace('.', '/')}.class" + @TempDir lateinit var tempDir: File @DisabledOnOs(OS.WINDOWS) // TODO: The output jar can't be deleted due to stream closing. @@ -96,12 +110,80 @@ class ShadowCopyActionTest { val result = action.execute(stream) assertThat(result.didWork).isTrue() - zipFile.useZip { assertThat(size()).isEqualTo(65536) } + JarPath(zipFile.toPath()).use { + assertThat(it.size()).isEqualTo(65536) + } + } + + @Test + fun remapsManyClassesInParallel() { + val zipFile = tempDir.resolve("output.jar") + val action = + ShadowCopyAction( + zipFile = zipFile, + relocators = setOf(SimpleRelocator("com.example", "relocated.example")), + ) + + val rawBytes = requireResourceAsPath(classEntry).readBytes() + + val count = 500 + val stream = CopyActionProcessingStream { streamAction -> + for (i in 1..count) { + streamAction.processFile(dummyDetails("com/example/Class$i.class", rawBytes)) + } + } + + val result = action.execute(stream) + assertThat(result.didWork).isTrue() + val expectedEntries = + (1..count).map { "relocated/example/Class$it.class" } + + listOf("relocated/example/", "relocated/") + JarPath(zipFile.toPath()).use { + assertThat(it).containsExactly(*expectedEntries.toTypedArray()) + } + } + + @Test + fun parallelRemappingFasterThanSequential() { + val relocators = setOf(SimpleRelocator("com.example", "relocated.example")) + val rawBytes = requireResourceAsPath(classEntry).readBytes() + val classes = (1..1000).map { "com/example/Class$it.class" to rawBytes } + + fun remapSequential() = classes.map { (path, bytes) -> + bytes.remapClass(relocators = relocators, path = path) + } + + fun remapParallel() = runBlocking { + classes + .map { (path, bytes) -> + async(Dispatchers.Default) { + bytes.remapClass(relocators = relocators, path = path) + } + } + .awaitAll() + } + + // Warm up JIT and coroutines thread pool + repeat(3) { + remapSequential() + remapParallel() + } + + var sequentialDuration = Duration.ZERO + var parallelDuration = Duration.ZERO + repeat(5) { + sequentialDuration += measureTime { remapSequential() } + parallelDuration += measureTime { remapParallel() } + } + + val ratio = if (Runtime.getRuntime().availableProcessors() == 1) 1.0 else 0.8 + assertThat(parallelDuration).isLessThan(sequentialDuration * ratio) } private fun ShadowCopyAction( zipFile: File = tempDir.resolve("output.jar"), isZip64: Boolean = false, + relocators: Set = emptySet(), ) = ShadowCopyAction( zipFile = zipFile, @@ -112,14 +194,17 @@ class ShadowCopyActionTest { encoding = null, ), transformers = emptySet(), - relocators = emptySet(), + relocators = relocators, unusedClasses = emptySet(), isPreserveFileTimestamps = true, failOnDuplicateEntries = false, ) } -private fun dummyDetails(path: String): FileCopyDetailsInternal = +private fun dummyDetails( + path: String, + bytes: ByteArray = ByteArray(0), +): FileCopyDetailsInternal = object : FileCopyDetailsInternal by noOpDelegate() { private val _relativePath = RelativePath.parse(true, path) @@ -129,9 +214,9 @@ private fun dummyDetails(path: String): FileCopyDetailsInternal = override fun getRelativePath(): RelativePath = _relativePath - override fun open(): InputStream = ByteArrayInputStream(ByteArray(0)) + override fun open(): InputStream = bytes.inputStream() - override fun copyTo(target: OutputStream) = Unit + override fun copyTo(target: OutputStream) = target.write(bytes) override fun getLastModified(): Long = 0L