Reduce duplicated SimpleRelocator to improve performance (#1271)

* Mark includes and excludes ad MutableSet, so that we can override `equals` and `hashcode`.

* Mark `withDebug` for testing

* Reduce duplicates for `packageRelocators`

* Revert "Mark `withDebug` for testing"

This reverts commit 6a16b57.

* Check relocator count in `autoRelocation`

* Remove `ObjectFactory` from ctor

* Update changelog

* Cleanups

Author: Goooler

api/shadow.api

@@ -130,19 +130,21 @@ public final class com/github/jengelman/gradle/plugins/shadow/relocation/Relocat
 }
 
 public class com/github/jengelman/gradle/plugins/shadow/relocation/SimpleRelocator : com/github/jengelman/gradle/plugins/shadow/relocation/Relocator {
-	public fun <init> (Lorg/gradle/api/model/ObjectFactory;)V
-	public fun <init> (Lorg/gradle/api/model/ObjectFactory;Ljava/lang/String;)V
-	public fun <init> (Lorg/gradle/api/model/ObjectFactory;Ljava/lang/String;Ljava/lang/String;)V
-	public fun <init> (Lorg/gradle/api/model/ObjectFactory;Ljava/lang/String;Ljava/lang/String;Ljava/util/List;)V
-	public fun <init> (Lorg/gradle/api/model/ObjectFactory;Ljava/lang/String;Ljava/lang/String;Ljava/util/List;Ljava/util/List;)V
-	public fun <init> (Lorg/gradle/api/model/ObjectFactory;Ljava/lang/String;Ljava/lang/String;Ljava/util/List;Ljava/util/List;Z)V
-	public synthetic fun <init> (Lorg/gradle/api/model/ObjectFactory;Ljava/lang/String;Ljava/lang/String;Ljava/util/List;Ljava/util/List;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public fun <init> ()V
+	public fun <init> (Ljava/lang/String;)V
+	public fun <init> (Ljava/lang/String;Ljava/lang/String;)V
+	public fun <init> (Ljava/lang/String;Ljava/lang/String;Ljava/util/List;)V
+	public fun <init> (Ljava/lang/String;Ljava/lang/String;Ljava/util/List;Ljava/util/List;)V
+	public fun <init> (Ljava/lang/String;Ljava/lang/String;Ljava/util/List;Ljava/util/List;Z)V
+	public synthetic fun <init> (Ljava/lang/String;Ljava/lang/String;Ljava/util/List;Ljava/util/List;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V
 	public fun applyToSourceContent (Ljava/lang/String;)Ljava/lang/String;
 	public fun canRelocateClass (Ljava/lang/String;)Z
 	public fun canRelocatePath (Ljava/lang/String;)Z
+	public fun equals (Ljava/lang/Object;)Z
 	public fun exclude (Ljava/lang/String;)Lcom/github/jengelman/gradle/plugins/shadow/relocation/SimpleRelocator;
-	public final fun getExcludes ()Lorg/gradle/api/provider/SetProperty;
-	public final fun getIncludes ()Lorg/gradle/api/provider/SetProperty;
+	public final fun getExcludes ()Ljava/util/Set;
+	public final fun getIncludes ()Ljava/util/Set;
+	public fun hashCode ()I
 	public fun include (Ljava/lang/String;)Lcom/github/jengelman/gradle/plugins/shadow/relocation/SimpleRelocator;
 	public fun relocateClass-XBGRxQs (Ljava/lang/String;)Ljava/lang/String;
 	public fun relocatePath-bvWaKNU (Ljava/lang/String;)Ljava/lang/String;

src/docs/changes/README.md

@@ -10,6 +10,7 @@
 **Changed**
 
 - **BREAKING CHANGE:** Move tracking unused classes logic out of `ShadowCopyAction`. ([#1257](https://github.com/GradleUp/shadow/pull/1257))
+- Reduce duplicated `SimpleRelocator` to improve performance. ([#1271](https://github.com/GradleUp/shadow/pull/1271))
 
 **Removed**
 

src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/RelocationTest.kt

@@ -2,6 +2,7 @@ package com.github.jengelman.gradle.plugins.shadow
 
 import assertk.assertFailure
 import assertk.assertThat
+import assertk.assertions.contains
 import assertk.assertions.isEmpty
 import assertk.assertions.isInstanceOf
 import assertk.assertions.isNotEmpty
@@ -15,6 +16,7 @@ import kotlin.io.path.appendText
 import kotlin.io.path.writeText
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.Arguments
 import org.junit.jupiter.params.provider.MethodSource
 import org.opentest4j.AssertionFailedError
 
@@ -36,7 +38,7 @@ class RelocationTest : BasePluginTest() {
     )
     val entryPrefix = relocationPrefix.replace('.', '/')
 
-    run(shadowJarTask)
+    val result = run(shadowJarTask, "--info")
 
     assertThat(outputShadowJar).useAll {
       containsEntries(
@@ -48,6 +50,10 @@ class RelocationTest : BasePluginTest() {
         *junitEntries,
       )
     }
+    // Make sure the relocator count is aligned with the number of unique packages in junit jar.
+    assertThat(result.output).contains(
+      "Relocator count: 6.",
+    )
   }
 
   @Issue(
@@ -345,9 +351,9 @@ class RelocationTest : BasePluginTest() {
     @JvmStatic
     fun prefixProvider() = listOf(
       // The default values.
-      arrayOf(ShadowBasePlugin.SHADOW),
-      arrayOf("new.pkg"),
-      arrayOf("new/path"),
+      Arguments.of(ShadowBasePlugin.SHADOW),
+      Arguments.of("new.pkg"),
+      Arguments.of("new/path"),
     )
   }
 }

src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/relocation/SimpleRelocator.kt

@@ -1,10 +1,7 @@
 package com.github.jengelman.gradle.plugins.shadow.relocation
 
-import com.github.jengelman.gradle.plugins.shadow.internal.setProperty
 import java.util.regex.Pattern
 import org.codehaus.plexus.util.SelectorUtils
-import org.gradle.api.model.ObjectFactory
-import org.gradle.api.provider.SetProperty
 import org.gradle.api.tasks.Input
 
 /**
@@ -16,7 +13,6 @@ import org.gradle.api.tasks.Input
  */
 @CacheableRelocator
 public open class SimpleRelocator @JvmOverloads constructor(
-  objectFactory: ObjectFactory,
   pattern: String? = null,
   shadedPattern: String? = null,
   includes: List<String>? = null,
@@ -31,10 +27,10 @@ public open class SimpleRelocator @JvmOverloads constructor(
   private val sourcePathExcludes = mutableSetOf<String>()
 
   @get:Input
-  public val includes: SetProperty<String> = objectFactory.setProperty()
+  public val includes: MutableSet<String> = mutableSetOf()
 
   @get:Input
-  public val excludes: SetProperty<String> = objectFactory.setProperty()
+  public val excludes: MutableSet<String> = mutableSetOf()
 
   init {
     if (rawString) {
@@ -71,7 +67,7 @@ public open class SimpleRelocator @JvmOverloads constructor(
 
     if (!rawString) {
       // Create exclude pattern sets for sources.
-      for (exclude in this.excludes.get()) {
+      for (exclude in this.excludes) {
         // Excludes should be subpackages of the global pattern.
         if (exclude.startsWith(this.pattern)) {
           sourcePackageExcludes.add(
@@ -136,12 +132,39 @@ public open class SimpleRelocator @JvmOverloads constructor(
     return shadeSourceWithExcludes(content, pathPattern, shadedPathPattern, sourcePathExcludes)
   }
 
+  override fun equals(other: Any?): Boolean {
+    if (this === other) return true
+    if (other !is SimpleRelocator) return false
+    return rawString == other.rawString &&
+      pattern == other.pattern &&
+      pathPattern == other.pathPattern &&
+      shadedPattern == other.shadedPattern &&
+      shadedPathPattern == other.shadedPathPattern &&
+      sourcePackageExcludes == other.sourcePackageExcludes &&
+      sourcePathExcludes == other.sourcePathExcludes &&
+      includes == other.includes &&
+      excludes == other.excludes
+  }
+
+  override fun hashCode(): Int {
+    var result = rawString.hashCode()
+    result = 31 * result + pattern.hashCode()
+    result = 31 * result + pathPattern.hashCode()
+    result = 31 * result + shadedPattern.hashCode()
+    result = 31 * result + shadedPathPattern.hashCode()
+    result = 31 * result + sourcePackageExcludes.hashCode()
+    result = 31 * result + sourcePathExcludes.hashCode()
+    result = 31 * result + includes.hashCode()
+    result = 31 * result + excludes.hashCode()
+    return result
+  }
+
   private fun isIncluded(path: String): Boolean {
-    return includes.get().isEmpty() || includes.get().any { SelectorUtils.matchPath(it, path, "/", true) }
+    return includes.isEmpty() || includes.any { SelectorUtils.matchPath(it, path, "/", true) }
   }
 
   private fun isExcluded(path: String): Boolean {
-    return excludes.get().any { SelectorUtils.matchPath(it, path, "/", true) }
+    return excludes.any { SelectorUtils.matchPath(it, path, "/", true) }
   }
 
   private companion object {

src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/tasks/ShadowCopyAction.kt

@@ -172,6 +172,7 @@ public open class ShadowCopyAction(
     private val visitedFiles = mutableSetOf<String>()
 
     init {
+      logger.info("Relocator count: ${relocators.size}.")
       if (encoding != null) {
         this.zipOutStr.setEncoding(encoding)
       }

src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/tasks/ShadowJar.kt

@@ -30,7 +30,6 @@ import org.gradle.api.Action
 import org.gradle.api.artifacts.Configuration
 import org.gradle.api.file.ConfigurableFileCollection
 import org.gradle.api.file.DuplicatesStrategy
-import org.gradle.api.file.FileCollection
 import org.gradle.api.internal.DocumentationRegistry
 import org.gradle.api.internal.file.FileResolver
 import org.gradle.api.internal.file.copy.CopyAction
@@ -242,7 +241,7 @@ public abstract class ShadowJar :
     destination: String,
     action: Action<SimpleRelocator>?,
   ): ShadowJar = apply {
-    val relocator = SimpleRelocator(objectFactory, pattern, destination)
+    val relocator = SimpleRelocator(pattern, destination)
     addRelocator(relocator, action)
   }
 
@@ -303,17 +302,14 @@ public abstract class ShadowJar :
   private val packageRelocators: List<SimpleRelocator>
     get() {
       if (!enableRelocation.get()) return emptyList()
-
       val prefix = relocationPrefix.get()
-      // Must cast configurations to Set<FileCollection> to fix type mismatch in runtime.
-      return (configurations.get() as Set<FileCollection>).flatMap { configuration ->
-        configuration.files.flatMap { file ->
-          JarFile(file).use { jarFile ->
-            jarFile.entries().toList()
-              .filter { it.name.endsWith(".class") && it.name != "module-info.class" }
-              .map { it.name.substringBeforeLast('/').replace('/', '.') }
-              .map { SimpleRelocator(objectFactory, it, "$prefix.$it") }
-          }
+      return includedDependencies.files.flatMap { file ->
+        JarFile(file).use { jarFile ->
+          jarFile.entries().toList()
+            .filter { it.name.endsWith(".class") && it.name != "module-info.class" }
+            .map { it.name.substringBeforeLast('/').replace('/', '.') }
+            .toSet()
+            .map { SimpleRelocator(it, "$prefix.$it") }
         }
       }
     }

src/test/kotlin/com/github/jengelman/gradle/plugins/shadow/relocation/SimpleRelocatorTest.kt

@@ -4,7 +4,6 @@ import assertk.assertThat
 import assertk.assertions.isEqualTo
 import assertk.assertions.isFalse
 import assertk.assertions.isTrue
-import com.github.jengelman.gradle.plugins.shadow.util.SimpleRelocator
 import org.junit.jupiter.api.Test
 
 /**

src/test/kotlin/com/github/jengelman/gradle/plugins/shadow/transformers/Log4j2PluginsCacheFileTransformerTest.kt

@@ -10,7 +10,7 @@ import assertk.assertions.startsWith
 import assertk.fail
 import com.github.jengelman.gradle.plugins.shadow.internal.requireResourceAsStream
 import com.github.jengelman.gradle.plugins.shadow.relocation.Relocator
-import com.github.jengelman.gradle.plugins.shadow.util.SimpleRelocator
+import com.github.jengelman.gradle.plugins.shadow.relocation.SimpleRelocator
 import com.github.jengelman.gradle.plugins.shadow.util.zipOutputStream
 import java.io.ByteArrayOutputStream
 import java.net.URI

src/test/kotlin/com/github/jengelman/gradle/plugins/shadow/transformers/ServiceFileTransformerTest.kt

@@ -5,7 +5,7 @@ import assertk.assertions.isEqualTo
 import assertk.assertions.isFalse
 import assertk.assertions.isTrue
 import com.github.jengelman.gradle.plugins.shadow.relocation.Relocator
-import com.github.jengelman.gradle.plugins.shadow.util.SimpleRelocator
+import com.github.jengelman.gradle.plugins.shadow.relocation.SimpleRelocator
 import com.github.jengelman.gradle.plugins.shadow.util.zipOutputStream
 import java.io.InputStream
 import java.nio.file.Path

src/test/kotlin/com/github/jengelman/gradle/plugins/shadow/util/Utils.kt

@@ -1,29 +1,12 @@
 package com.github.jengelman.gradle.plugins.shadow.util
 
-import com.github.jengelman.gradle.plugins.shadow.relocation.SimpleRelocator
 import java.io.OutputStream
 import org.apache.tools.zip.ZipOutputStream
 import org.gradle.api.model.ObjectFactory
 import org.gradle.testfixtures.ProjectBuilder
 
 val testObjectFactory: ObjectFactory = ProjectBuilder.builder().build().objects
 
-@Suppress("TestFunctionName")
-fun SimpleRelocator(
-  pattern: String? = null,
-  shadedPattern: String? = null,
-  includes: List<String> = emptyList(),
-  excludes: List<String> = emptyList(),
-  rawString: Boolean = false,
-): SimpleRelocator = SimpleRelocator(
-  objectFactory = testObjectFactory,
-  pattern = pattern,
-  shadedPattern = shadedPattern,
-  includes = includes,
-  excludes = excludes,
-  rawString = rawString,
-)
-
 fun OutputStream.zipOutputStream(): ZipOutputStream {
-  return if (this is ZipOutputStream) this else ZipOutputStream(this)
+  return this as? ZipOutputStream ?: ZipOutputStream(this)
 }