Cache Gradle properties with a BuildService to improve performance

Currently, the performance overhead when loading a Gradle property consists of: 1. Creating a Provider 2. Resolving the Provider Both steps are performed even when the same property was loaded before. That means this overhead will multiply when there are a large number of property read requests (e.g., in KT-62496, the tested project 400,000 read requests for only 17 properties). To improve performance, we will now cache both steps with a BuildService. With this commit, configuration time for the tested project returns to the same level before commit d6becee where the performance regression happened (that commit can't be reverted because it introduced `Provider`s which are required for proper Gradle usage). Test: Manually verified on the large project in KT-62496 ^KT-62496 Fixed
2023-10-11 21:21:56 +01:00
parent 141f2e3234
commit 5ceebec331
9 changed files with 116 additions and 60 deletions
@@ -0,0 +1,86 @@
+/*
+ * Copyright 2010-2023 JetBrains s.r.o. and Kotlin Programming Language contributors.
+ * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
+ */
+
+package org.jetbrains.kotlin.gradle.plugin
+
+import org.gradle.api.Project
+import org.gradle.api.provider.MapProperty
+import org.gradle.api.provider.Provider
+import org.gradle.api.services.BuildService
+import org.gradle.api.services.BuildServiceParameters
+import org.jetbrains.kotlin.gradle.plugin.internal.ConfigurationTimePropertiesAccessor
+import org.jetbrains.kotlin.gradle.plugin.internal.configurationTimePropertiesAccessor
+import org.jetbrains.kotlin.gradle.plugin.internal.usedAtConfigurationTime
+import org.jetbrains.kotlin.gradle.utils.localProperties
+import org.jetbrains.kotlin.gradle.utils.registerClassLoaderScopedBuildService
+import java.util.concurrent.Callable
+import java.util.concurrent.ConcurrentHashMap
+
+/**
+ * [BuildService] that looks up properties in the following precedence order:
+ *   1. Project's extra properties ([org.gradle.api.plugins.ExtensionContainer.getExtraProperties])
+ *   2. Project's Gradle properties ([org.gradle.api.provider.ProviderFactory.gradleProperty])
+ *   3. Root project's `local.properties` file ([Project.localProperties])
+ *
+ *  Note that extra and Gradle properties may differ across projects, whereas `local.properties` is shared across all projects.
+ */
+internal abstract class PropertiesBuildService : BuildService<PropertiesBuildService.Params> {
+
+    interface Params : BuildServiceParameters {
+        val localProperties: MapProperty<String, String>
+    }
+
+    private val propertiesManager = ConcurrentHashMap<Project, PropertiesManager>()
+
+    /** Returns a [Provider] of the value of the property with the given [propertyName] in the given [project]. */
+    fun property(propertyName: String, project: Project): Provider<String> {
+        return propertiesManager.computeIfAbsent(project) { PropertiesManager(project, parameters.localProperties.get()) }
+            .property(propertyName)
+    }
+
+    /** Returns the value of the property with the given [propertyName] in the given [project]. */
+    fun get(propertyName: String, project: Project): String? {
+        return property(propertyName, project).orNull
+    }
+
+    companion object {
+
+        fun registerIfAbsent(project: Project): Provider<PropertiesBuildService> =
+            project.gradle.registerClassLoaderScopedBuildService(PropertiesBuildService::class) {
+                it.parameters.localProperties.set(project.localProperties)
+            }
+    }
+}
+
+private class PropertiesManager(private val project: Project, private val localProperties: Map<String, String>) {
+
+    private val configurationTimePropertiesAccessor: ConfigurationTimePropertiesAccessor by lazy {
+        project.configurationTimePropertiesAccessor
+    }
+
+    private val properties = ConcurrentHashMap<String, Provider<String>>()
+
+    fun property(propertyName: String): Provider<String> {
+        // Note: The same property may be read many times (KT-62496). Therefore,
+        //   - Use a map to create only one Provider per property.
+        //   - Use MemoizedCallable to resolve the Provider only once
+        return properties.computeIfAbsent(propertyName) {
+            project.provider { computeValue(propertyName) }
+        }
+    }
+
+    private fun computeValue(propertyName: String): String? {
+        return project.extraProperties.getOrNull(propertyName) as String? // We don't memoize extraProperties as they can still change
+            ?: MemoizedCallable {
+                project.providers.gradleProperty(propertyName).usedAtConfigurationTime(configurationTimePropertiesAccessor).orNull
+                    ?: localProperties[propertyName]
+            }.call()
+    }
+}
+
+private class MemoizedCallable<T>(valueResolver: Callable<T>) : Callable<T> {
+    private val value by lazy { valueResolver.call() }
+    override fun call(): T = value
+}
@@ -39,10 +39,10 @@ import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLI
 import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_MPP_HIERARCHICAL_STRUCTURE_SUPPORT
 import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_MPP_IMPORT_ENABLE_KGP_DEPENDENCY_RESOLUTION
 import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_MPP_IMPORT_ENABLE_SLOW_SOURCES_JAR_RESOLVER
-import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_PUBLISH_JVM_ENVIRONMENT_ATTRIBUTE
 import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_NATIVE_IGNORE_DISABLED_TARGETS
 import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_NATIVE_SUPPRESS_EXPERIMENTAL_ARTIFACTS_DSL_WARNING
 import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_NATIVE_USE_XCODE_MESSAGE_STYLE
+import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_PUBLISH_JVM_ENVIRONMENT_ATTRIBUTE
 import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_RUN_COMPILER_VIA_BUILD_TOOLS_API
 import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_STDLIB_DEFAULT_DEPENDENCY
 import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider.PropertyNames.KOTLIN_STDLIB_JDK_VARIANTS_VERSION_ALIGNMENT
@@ -55,7 +55,6 @@ import org.jetbrains.kotlin.gradle.targets.js.ir.KotlinIrJsGeneratedTSValidation
 import org.jetbrains.kotlin.gradle.targets.js.ir.KotlinJsIrOutputGranularity
 import org.jetbrains.kotlin.gradle.tasks.KotlinCompilerExecutionStrategy
 import org.jetbrains.kotlin.gradle.utils.NativeCompilerDownloader
-import org.jetbrains.kotlin.gradle.utils.loadProperty
 import org.jetbrains.kotlin.gradle.utils.localProperties
 import org.jetbrains.kotlin.konan.target.KonanTarget
 import org.jetbrains.kotlin.konan.target.presetName
@@ -529,6 +528,12 @@ internal class PropertiesProvider private constructor(private val project: Proje
    val suppressBuildToolsApiVersionConsistencyChecks: Boolean
        get() = booleanProperty(PropertyNames.KOTLIN_SUPPRESS_BUILD_TOOLS_API_VERSION_CONSISTENCY_CHECKS) ?: false

+    private val propertiesBuildService = PropertiesBuildService.registerIfAbsent(project).get()
+
+    internal fun property(propertyName: String): Provider<String> = propertiesBuildService.property(propertyName, project)
+
+    internal fun get(propertyName: String): String? = propertiesBuildService.get(propertyName, project)
+
    /**
     * Retrieves a comma-separated list of browsers to use when running karma tests for [target]
     * @see KOTLIN_JS_KARMA_BROWSERS
@@ -538,23 +543,22 @@ internal class PropertiesProvider private constructor(private val project: Proje
            ?: property(KOTLIN_JS_KARMA_BROWSERS).orNull

    private fun propertyWithDeprecatedVariant(propName: String, deprecatedPropName: String): String? {
-        val deprecatedProperty = property(deprecatedPropName).orNull
+        val deprecatedProperty = get(deprecatedPropName)
        if (deprecatedProperty != null) {
            project.reportDiagnosticOncePerBuild(KotlinToolingDiagnostics.DeprecatedPropertyWithReplacement(deprecatedPropName, propName))
        }
-        return property(propName).orNull ?: deprecatedProperty
+        return get(propName) ?: deprecatedProperty
    }

    private fun booleanProperty(propName: String): Boolean? =
-        property(propName).orNull?.toBoolean()
+        get(propName)?.toBoolean()

    private inline fun <reified T : Enum<T>> enumProperty(
        propName: String,
        defaultValue: T,
-    ): T = this.property(propName).orNull?.let { enumValueOf<T>(it.toUpperCaseAsciiOnly()) } ?: defaultValue
+    ): T = get(propName)?.let { enumValueOf<T>(it.toUpperCaseAsciiOnly()) } ?: defaultValue

-    private val localProperties = project.localProperties
-    internal fun property(propName: String): Provider<String> = project.loadProperty(propName, localProperties)
+    private val localProperties: Map<String, String> by lazy { project.localProperties.get() }

    private fun propertiesWithPrefix(prefix: String): Map<String, String> {
        val result: MutableMap<String, String> = mutableMapOf()
@@ -563,7 +567,7 @@ internal class PropertiesProvider private constructor(private val project: Proje
                result[name] = value
            }
        }
-        localProperties.orNull?.forEach { (name, value) ->
+        localProperties.forEach { (name, value) ->
            if (name.startsWith(prefix)) {
                // Project properties have higher priority.
                result.putIfAbsent(name, value)
@@ -31,8 +31,8 @@ internal inline fun <reified T : Any> Any.getExtension(name: String): T? =
 internal inline fun <reified T : Any> Any.findExtension(name: String): T? =
    (this as ExtensionAware).extensions.findByName(name)?.let { it as T? }

-inline val Any.extraProperties: ExtraPropertiesExtension
-    get() = (this as ExtensionAware).extensions.extraProperties
+inline val ExtensionAware.extraProperties: ExtraPropertiesExtension
+    get() = extensions.extraProperties

@JvmName("getOrNullTyped")
 internal inline fun <reified T : Any> ExtraPropertiesExtension.getOrNull(name: String): T? {
@@ -18,7 +18,7 @@ import java.util.*
 * Returned type is `Map<String, String>` due to the following [bug in Gradle](https://github.com/gradle/gradle/pull/24846) which
 * prevents proper serialization of [Properties] type.
 *
- * If the file does not exist - returned provider will be empty.
+ * If the file does not exist - returned map will be empty.
 *
 * Usage:
 * ```
@@ -38,7 +38,7 @@ internal abstract class CustomPropertiesFileValueSource : ValueSource<Map<String

    override fun getDisplayName(): String = "properties file ${parameters.propertiesFile.get().asFile.absolutePath}"

-    override fun obtain(): Map<String, String>? {
+    override fun obtain(): Map<String, String> {
        val customFile = parameters.propertiesFile.get().asFile
        return if (customFile.exists()) {
            customFile.bufferedReader().use {
@@ -46,7 +46,7 @@ internal abstract class CustomPropertiesFileValueSource : ValueSource<Map<String
                Properties().apply { load(it) }.toMap() as Map<String, String>
            }
        } else {
-            null
+            emptyMap()
        }
    }
 }
@@ -14,12 +14,11 @@ import org.gradle.invocation.DefaultGradle
 import org.gradle.tooling.events.OperationCompletionListener
 import org.gradle.tooling.events.task.TaskFinishEvent
 import org.jetbrains.kotlin.gradle.plugin.BuildEventsListenerRegistryHolder
+import org.jetbrains.kotlin.gradle.plugin.PropertiesBuildService
 import org.jetbrains.kotlin.gradle.plugin.internal.ConfigurationTimePropertiesAccessor
 import org.jetbrains.kotlin.gradle.plugin.internal.configurationTimePropertiesAccessor
 import org.jetbrains.kotlin.gradle.plugin.internal.usedAtConfigurationTime
 import org.jetbrains.kotlin.gradle.plugin.statistics.KotlinBuildStatHandler.Companion.runSafe
-import org.jetbrains.kotlin.gradle.utils.loadProperty
-import org.jetbrains.kotlin.gradle.utils.localProperties
 import org.jetbrains.kotlin.statistics.BuildSessionLogger
 import org.jetbrains.kotlin.statistics.BuildSessionLogger.Companion.STATISTICS_FOLDER_NAME
 import org.jetbrains.kotlin.statistics.metrics.BooleanMetrics
@@ -246,16 +245,11 @@ internal abstract class AbstractKotlinBuildStatsService(
    private val customSessionLoggerRootPath: String?

    init {
-        val localProperties = project.localProperties
-        forcePropertiesValidation = project
-            .loadProperty(FORCE_VALUES_VALIDATION, localProperties)
-            .orNull?.toBoolean() ?: false
-        customSessionLoggerRootPath = project
-            .loadProperty(CUSTOM_LOGGER_ROOT_PATH, localProperties)
-            .orNull
-            ?.also {
-                logger.warn("$CUSTOM_LOGGER_ROOT_PATH property for test purpose only")
-            }
+        val propertiesBuildService = PropertiesBuildService.registerIfAbsent(project).get()
+        forcePropertiesValidation = propertiesBuildService.get(FORCE_VALUES_VALIDATION, project)?.toBoolean() ?: false
+        customSessionLoggerRootPath = propertiesBuildService.get(CUSTOM_LOGGER_ROOT_PATH, project)?.also {
+            logger.warn("$CUSTOM_LOGGER_ROOT_PATH property for test purpose only")
+        }
    }

    private val sessionLoggerRootPath =
@@ -9,8 +9,6 @@ import org.gradle.api.Project
 import org.gradle.api.file.Directory
 import org.gradle.api.file.RegularFile
 import org.gradle.api.provider.Provider
-import org.jetbrains.kotlin.gradle.plugin.extraProperties
-import org.jetbrains.kotlin.gradle.plugin.getOrNull
 import org.jetbrains.kotlin.gradle.plugin.internal.CustomPropertiesFileValueSource
 import org.jetbrains.kotlin.gradle.plugin.internal.configurationTimePropertiesAccessor
 import org.jetbrains.kotlin.gradle.plugin.internal.usedAtConfigurationTime
@@ -133,31 +131,6 @@ internal fun File.existsCompat(): Boolean =
        exists()
    }

-/**
- * Looks up the property in the following sources with decreasing priority:
- * 1. Current project extra properties
- * 2. Project Gradle properties (-P, gradle.properties, etc...)
- * 3. `local.properties` file located in the rootDir
- *
- * If multiple properties are loaded from a same caller, it is better to cache `localProperties` into local variable.
- */
-internal fun Project.loadProperty(
-    propName: String,
-    localPropertiesProvider: Provider<Map<String, String>> = localProperties
-): Provider<String> = providers
-    .provider<String> {
-        extraProperties.getOrNull(propName) as? String
-    }
-    .orElse(
-        project.providers
-            .gradleProperty(propName)
-            .usedAtConfigurationTime(configurationTimePropertiesAccessor)
-    )
-    .orElse(
-        @Suppress("TYPE_MISMATCH")
-        localPropertiesProvider.map { it[propName] }
-    )
-
 /**
 * Loads 'local.properties' file content as [Properties].
 *
@@ -31,7 +31,6 @@ class ExternalKotlinTargetApiUtilsTest {
    @Test
    fun `test - publishJvmEnvironmentAttribute`() {
        val project = buildProjectWithMPP()
-
        project.externalKotlinTargetApiUtils.publishJvmEnvironmentAttribute(true)
        assertTrue(project.kotlinPropertiesProvider.publishJvmEnvironmentAttribute)

@@ -9,14 +9,13 @@ import org.gradle.api.Project
 import org.gradle.api.internal.project.ProjectInternal
 import org.jetbrains.kotlin.gradle.plugin.PropertiesProvider
 import org.jetbrains.kotlin.gradle.plugin.diagnostics.*
-import org.jetbrains.kotlin.gradle.util.applyKotlinJvmPlugin
-import org.jetbrains.kotlin.gradle.util.buildProject
-import org.jetbrains.kotlin.gradle.util.checkDiagnostics
-import org.jetbrains.kotlin.gradle.plugin.diagnostics.ToolingDiagnostic
 import org.jetbrains.kotlin.gradle.plugin.diagnostics.ToolingDiagnostic.Severity
 import org.jetbrains.kotlin.gradle.plugin.diagnostics.ToolingDiagnostic.Severity.ERROR
 import org.jetbrains.kotlin.gradle.plugin.diagnostics.ToolingDiagnostic.Severity.WARNING
 import org.jetbrains.kotlin.gradle.plugin.extraProperties
+import org.jetbrains.kotlin.gradle.util.applyKotlinJvmPlugin
+import org.jetbrains.kotlin.gradle.util.buildProject
+import org.jetbrains.kotlin.gradle.util.checkDiagnostics
 import org.jetbrains.kotlin.gradle.util.set
 import org.junit.Test

@@ -120,6 +119,7 @@ class DiagnosticsReportingFunctionalTest {
    fun testSuppressedWarnings() {
        buildProject().run {
            applyKotlinJvmPlugin()
+
            extraProperties.set(PropertiesProvider.PropertyNames.KOTLIN_SUPPRESS_GRADLE_PLUGIN_WARNINGS, "TEST_DIAGNOSTIC")
            reportTestDiagnostic()
            evaluate()
@@ -25,6 +25,7 @@ class KotlinAndroidSourceSetLayoutExtensionTest {
    fun `single platform plugin`() {
        val project = ProjectBuilder.builder().build()
        project.plugins.apply(KotlinAndroidPluginWrapper::class.java)
+
        assertEquals(singleTargetAndroidSourceSetLayout, project.kotlinAndroidSourceSetLayout)

        project.setMultiplatformAndroidSourceSetLayoutVersion(1)
@@ -36,8 +37,7 @@ class KotlinAndroidSourceSetLayoutExtensionTest {

    @Test
    fun `test multiplatform plugin`() {
-        val project = buildProjectWithMPP { }
-
+        val project = buildProjectWithMPP()
        assertEquals(
            multiplatformAndroidSourceSetLayoutV2, project.kotlinAndroidSourceSetLayout,
            "Expected v2 being set as default"