KT-45777: Don't include classpath snapshot dir in task output backup

for performance reasons: (1) the snapshots are too big, and (2) they are usually updated only at the end of the task execution--in a failed task run, they are usually unchanged and therefore don't need to be restored.
2021-11-19 12:46:11 +03:00
parent 062a8fe56f
commit 6ba1b2cc08
12 changed files with 137 additions and 74 deletions
@@ -17,6 +17,7 @@
 package org.jetbrains.kotlin.incremental

 import java.io.File
+import java.io.IOException

 fun File.isJavaFile() =
        extension.equals("java", ignoreCase = true)
@@ -26,3 +27,43 @@ fun File.isKotlinFile(sourceFilesExtensions: List<String>): Boolean =

 fun File.isClassFile(): Boolean =
        extension.equals("class", ignoreCase = true)
+
+/**
+ * Deletes the contents of this directory (not the directory itself) if it exists, or creates the directory if it does not yet exist.
+ *
+ * If this is a regular file, this method will throw an exception.
+ */
+fun File.cleanDirectoryContents() {
+    when {
+        isDirectory -> listFiles()!!.forEach { it.forceDeleteRecursively() }
+        isFile -> error("File.cleanDirectoryContents does not accept a regular file: $path")
+        else -> forceMkdirs()
+    }
+}
+
+/** Deletes this file or directory recursively (if it exists). */
+fun File.forceDeleteRecursively() {
+    if (!deleteRecursively()) {
+        throw IOException("Could not delete '$path'")
+    }
+}
+
+/**
+ * Creates this directory (if it does not yet exist).
+ *
+ * If this is a regular file, this method will throw an exception.
+ */
+@Suppress("SpellCheckingInspection")
+fun File.forceMkdirs() {
+    when {
+        this.isDirectory -> { /* Do nothing */ }
+        this.isFile -> error("File.forceMkdirs does not accept a regular file: $path")
+        else -> {
+            // Note that if the directory already exists, mkdirs() will return `false`, but here we ensure that the directory does not exist
+            // before calling mkdirs(), so it's safe to check the returned result of mkdirs() below.
+            if (!mkdirs()) {
+                throw IOException("Could not create directory '$path'")
+            }
+        }
+    }
+}
@@ -38,7 +38,6 @@ import org.jetbrains.kotlin.incremental.util.BufferingMessageCollector
 import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.progress.CompilationCanceledStatus
 import java.io.File
-import java.io.IOException

 abstract class IncrementalCompilerRunner<
        Args : CommonCompilerArguments,
@@ -50,7 +49,7 @@ abstract class IncrementalCompilerRunner<
    private val buildHistoryFile: File,
    // there might be some additional output directories (e.g. for generated java in kapt)
    // to remove them correctly on rebuild, we pass them as additional argument
-    private val outputFiles: Collection<File> = emptyList()
+    private val additionalOutputFiles: Collection<File> = emptyList()
 ) {

    protected val cacheDirectory = File(workingDir, cacheDirName)
@@ -106,7 +105,7 @@ abstract class IncrementalCompilerRunner<
            caches.close(false)
            // todo: we can recompile all files incrementally (not cleaning caches), so rebuild won't propagate
            reporter.measure(BuildTime.CLEAR_OUTPUT_ON_REBUILD) {
-                clearOutputsOnRebuild(args)
+                cleanOutputsAndLocalStateOnRebuild(args)
            }
            caches = createCacheManager(args, projectDir)
            if (providedChangedFiles == null) {
@@ -168,7 +167,9 @@ abstract class IncrementalCompilerRunner<
                }
            }

-            performWorkAfterCompilation(caches)
+            if (exitCode == ExitCode.OK) {
+                performWorkAfterSuccessfulCompilation(caches)
+            }

            if (!caches.close(flush = true)) throw RuntimeException("Could not flush caches")
            // Here we should analyze exit code of compiler. E.g. compiler failure should lead to caches rebuild,
@@ -193,41 +194,32 @@ abstract class IncrementalCompilerRunner<
            rebuild(BuildAttribute.CACHE_CORRUPTION)
        } finally {
            if (cachesMayBeCorrupted) {
-                clearOutputsOnRebuild(args)
+                cleanOutputsAndLocalStateOnRebuild(args)
            }
        }
    }

    /**
-     * Deletes output files and contents of output directories on rebuild (including `@LocalState` files/directories).
+     * Deletes output files and contents of output directories on rebuild, including `@LocalState` files/directories.
     *
-     * If the output directories do not yet exist, they will be created.
+     * If the directories do not yet exist, they will be created.
     */
-    private fun clearOutputsOnRebuild(args: Args) {
-        val destinationDir = destinationDir(args)
-        val (outputDirsThatExist, regularOrNonExistentOutputFiles) = outputFiles.partition { it.isDirectory }
-        val regularOutputFiles = regularOrNonExistentOutputFiles.filter { it.exists() }
+    private fun cleanOutputsAndLocalStateOnRebuild(args: Args) {
+        // Use Set as additionalOutputFiles may already contain destinationDir and workingDir
+        val outputFiles = setOf(destinationDir(args), workingDir) + additionalOutputFiles

-        // outputDirsThatExist may or may not contain destinationDir and workingDir.
-        // Collect all of them so that we don't miss any output directories.
-        // Use Set to avoid duplication.
-        val allOutputDirs = setOf(destinationDir, workingDir) + outputDirsThatExist
-
-        reporter.reportVerbose { "Clearing outputs on rebuild" }
-        allOutputDirs.forEach { dir ->
-            reporter.reportVerbose { "  Deleting contents of directory '${dir.path}'" }
-            dir.listFiles()?.forEach {
-                it.deleteRecursively()
-                if (it.exists()) throw IOException("Could not delete '${it.path}'")
+        reporter.reportVerbose { "Cleaning outputs on rebuild" }
+        outputFiles.forEach {
+            when {
+                it.isDirectory -> {
+                    reporter.reportVerbose { "  Deleting contents of directory '${it.path}'" }
+                    it.cleanDirectoryContents()
+                }
+                it.isFile -> {
+                    reporter.reportVerbose { "  Deleting file '${it.path}'" }
+                    it.forceDeleteRecursively()
+                }
            }
-
-            dir.mkdirs()
-            if (!dir.exists()) throw IOException("Could not create directory '${dir.path}'")
-        }
-        regularOutputFiles.forEach { file ->
-            reporter.reportVerbose { "  Deleting file '${file.path}'" }
-            file.delete()
-            if (file.exists()) throw IOException("Could not delete file '${file.path}'")
        }
    }

@@ -502,7 +494,12 @@ abstract class IncrementalCompilerRunner<
        BuildDiffsStorage.writeToFile(buildHistoryFile, BuildDiffsStorage(prevDiffs + newDiff), reporter)
    }

-    protected open fun performWorkAfterCompilation(caches: CacheManager) {}
+    /**
+     * Performs some work after a compilation if the compilation completed successfully.
+     *
+     * This method MUST NOT be called when the compilation failed because the results produced by the work here would be incorrect.
+     */
+    protected open fun performWorkAfterSuccessfulCompilation(caches: CacheManager) {}

    companion object {
        const val DIRTY_SOURCES_FILE_NAME = "dirty-sources.txt"
@@ -129,7 +129,7 @@ class IncrementalJvmCompilerRunner(
    workingDir,
    "caches-jvm",
    reporter,
-    outputFiles = outputFiles,
+    additionalOutputFiles = outputFiles,
    buildHistoryFile = buildHistoryFile
 ) {
    override fun isICEnabled(): Boolean =
@@ -454,7 +454,7 @@ class IncrementalJvmCompilerRunner(
        return exitCode
    }

-    override fun performWorkAfterCompilation(caches: IncrementalJvmCachesManager) {
+    override fun performWorkAfterSuccessfulCompilation(caches: IncrementalJvmCachesManager) {
        if (classpathChanges is ClasspathChanges.ClasspathSnapshotEnabled) {
            reporter.measure(BuildTime.SAVE_SHRUNK_CURRENT_CLASSPATH_SNAPSHOT_AFTER_COMPILATION) {
                shrinkAndSaveClasspathSnapshot(classpathChanges, caches.lookupCache)
@@ -5,7 +5,6 @@

 package org.jetbrains.kotlin.compilerRunner

-import org.gradle.api.file.FileCollection
 import org.jetbrains.kotlin.config.Services
 import org.jetbrains.kotlin.gradle.logging.GradlePrintingMessageCollector
 import org.jetbrains.kotlin.gradle.report.ReportingSettings
@@ -15,7 +14,7 @@ internal class GradleCompilerEnvironment(
    val compilerClasspath: Iterable<File>,
    messageCollector: GradlePrintingMessageCollector,
    outputItemsCollector: OutputItemsCollector,
-    val outputFiles: FileCollection,
+    val outputFiles: List<File>,
    val reportingSettings: ReportingSettings,
    val incrementalCompilationEnvironment: IncrementalCompilationEnvironment? = null,
    val kotlinScriptExtensions: Array<String> = emptyArray()
@@ -6,8 +6,11 @@
 package org.jetbrains.kotlin.compilerRunner

 import org.gradle.api.GradleException
-import org.gradle.api.file.*
+import org.gradle.api.file.ConfigurableFileCollection
+import org.gradle.api.file.DirectoryProperty
+import org.gradle.api.file.FileSystemOperations
 import org.gradle.api.logging.Logging
+import org.gradle.api.provider.ListProperty
 import org.gradle.api.provider.Property
 import org.gradle.workers.WorkAction
 import org.gradle.workers.WorkParameters
@@ -63,7 +66,8 @@ internal class GradleCompilerRunnerWithWorkers(
                    fileSystemOperations,
                    parameters.buildDir,
                    parameters.snapshotsDir,
-                    parameters.taskOutputs,
+                    parameters.taskOutputs.files.toList(),
+                    emptyList(),
                    logger
                )
            } else {
@@ -22,7 +22,7 @@ import org.jetbrains.kotlin.gradle.report.TaskExecutionInfo
 import org.jetbrains.kotlin.gradle.report.TaskExecutionProperties.ABI_SNAPSHOT
 import org.jetbrains.kotlin.gradle.report.TaskExecutionResult
 import org.jetbrains.kotlin.gradle.tasks.KotlinCompilerExecutionStrategy
-import org.jetbrains.kotlin.gradle.tasks.clearLocalState
+import org.jetbrains.kotlin.gradle.tasks.cleanOutputsAndLocalState
 import org.jetbrains.kotlin.gradle.tasks.throwGradleExceptionIfError
 import org.jetbrains.kotlin.gradle.utils.stackTraceAsString
 import org.jetbrains.kotlin.incremental.ChangedFiles
@@ -313,7 +313,7 @@ internal class GradleKotlinCompilerWork @Inject constructor(

    private fun compileOutOfProcess(): ExitCode {
        metrics.addAttribute(BuildAttribute.OUT_OF_PROCESS_EXECUTION)
-        clearLocalState(outputFiles, log, metrics, reason = "out-of-process execution strategy is non-incremental")
+        cleanOutputsAndLocalState(outputFiles, log, metrics, reason = "out-of-process execution strategy is non-incremental")

        return metrics.measure(BuildTime.NON_INCREMENTAL_COMPILATION_OUT_OF_PROCESS) {
            runToolInSeparateProcess(compilerArgs, compilerClassName, compilerFullClasspath, log, buildDir)
@@ -322,7 +322,7 @@ internal class GradleKotlinCompilerWork @Inject constructor(

    private fun compileInProcess(messageCollector: MessageCollector): ExitCode {
        metrics.addAttribute(BuildAttribute.IN_PROCESS_EXECUTION)
-        clearLocalState(outputFiles, log, metrics, reason = "in-process execution strategy is non-incremental")
+        cleanOutputsAndLocalState(outputFiles, log, metrics, reason = "in-process execution strategy is non-incremental")

        metrics.startMeasure(BuildTime.NON_INCREMENTAL_COMPILATION_IN_PROCESS, System.nanoTime())
        // in-process compiler should always be run in a different thread
@@ -21,13 +21,7 @@ import org.jetbrains.kotlin.gradle.internal.kapt.incremental.UnknownSnapshot
 import org.jetbrains.kotlin.gradle.internal.tasks.TaskConfigurator
 import org.jetbrains.kotlin.gradle.internal.tasks.TaskWithLocalState
 import org.jetbrains.kotlin.gradle.tasks.*
-import org.jetbrains.kotlin.gradle.tasks.cacheOnlyIfEnabledForKotlin
-import org.jetbrains.kotlin.gradle.tasks.clearLocalState
 import org.jetbrains.kotlin.gradle.utils.*
-import org.jetbrains.kotlin.gradle.utils.isConfigurationCacheAvailable
-import org.jetbrains.kotlin.gradle.utils.property
-import org.jetbrains.kotlin.gradle.utils.propertyWithConvention
-import org.jetbrains.kotlin.gradle.utils.propertyWithNewInstance
 import org.jetbrains.kotlin.utils.addToStdlib.cast
 import java.io.File
 import java.util.concurrent.Callable
@@ -251,7 +245,7 @@ abstract class KaptTask @Inject constructor(
        return if (isIncremental) {
            findClasspathChanges(inputChanges)
        } else {
-            clearLocalState()
+            cleanOutputsAndLocalState()
            KaptIncrementalChanges.Unknown
        }
    }
@@ -286,7 +280,7 @@ abstract class KaptTask @Inject constructor(
            val classpathChanges = currentSnapshot.diff(previousSnapshot, changedFiles)
            if (classpathChanges == KaptClasspathChanges.Unknown) {
                // We are unable to determine classpath changes, so clean the local state as we will run non-incrementally
-                clearLocalState()
+                cleanOutputsAndLocalState()
            }
            currentSnapshot.writeToCache()

@@ -311,7 +305,7 @@ abstract class KaptTask @Inject constructor(
                )
            }
        } else {
-            clearLocalState("Kapt is running non-incrementally")
+            cleanOutputsAndLocalState("Kapt is running non-incrementally")

            ClasspathSnapshot.ClasspathSnapshotFactory
                .createCurrent(
@@ -7,10 +7,10 @@ package org.jetbrains.kotlin.gradle.internal.tasks

 import org.gradle.api.Task
 import org.gradle.api.file.ConfigurableFileCollection
-import org.gradle.api.file.FileCollection
 import org.gradle.api.provider.Property
 import org.gradle.api.tasks.Internal
 import org.jetbrains.kotlin.build.report.metrics.BuildMetricsReporter
+import java.io.File

 internal interface TaskWithLocalState : Task {
    @get:Internal
@@ -20,5 +20,5 @@ internal interface TaskWithLocalState : Task {
    val metrics: Property<BuildMetricsReporter>
 }

-internal fun TaskWithLocalState.allOutputFiles(): FileCollection =
-    outputs.files + localStateDirectories
+internal fun TaskWithLocalState.allOutputFiles(): List<File> =
+    (outputs.files.files + localStateDirectories.files).toList()
@@ -348,7 +348,7 @@ internal class CacheBuilder(
            computedCompilerClasspath,
            messageCollector,
            outputItemCollector,
-            outputFiles = objectFiles,
+            outputFiles = objectFiles.files.toList(),
            reportingSettings = reportingSettings
        )

@@ -60,7 +60,6 @@ import org.jetbrains.kotlin.incremental.ClasspathChanges.ClasspathSnapshotEnable
 import org.jetbrains.kotlin.incremental.ClasspathSnapshotFiles
 import org.jetbrains.kotlin.incremental.IncrementalCompilerRunner
 import org.jetbrains.kotlin.library.impl.isKotlinLibrary
-import org.jetbrains.kotlin.statistics.BuildSessionLogger
 import org.jetbrains.kotlin.statistics.metrics.BooleanMetrics
 import org.jetbrains.kotlin.utils.JsLibraryUtils
 import org.jetbrains.kotlin.utils.addToStdlib.cast
@@ -333,6 +332,10 @@ abstract class AbstractKotlinCompile<T : CommonCompilerArguments> : AbstractKotl

    private val systemPropertiesService = CompilerSystemPropertiesService.registerIfAbsent(project.gradle)

+    /** Task outputs that we don't want to include in [TaskOutputsBackup] (see [TaskOutputsBackup]'s kdoc for more info). */
+    @get:Internal
+    protected open val taskOutputsBackupExcludes: List<File> = emptyList()
+
    @TaskAction
    fun execute(inputChanges: InputChanges) {
        KotlinBuildStatsService.applyIfInitialised {
@@ -357,6 +360,7 @@ abstract class AbstractKotlinCompile<T : CommonCompilerArguments> : AbstractKotl
                            layout.buildDirectory,
                            layout.buildDirectory.dir("snapshot/kotlin/$name"),
                            allOutputFiles(),
+                            taskOutputsBackupExcludes,
                            logger
                        ).also {
                            it.createSnapshot()
@@ -365,9 +369,9 @@ abstract class AbstractKotlinCompile<T : CommonCompilerArguments> : AbstractKotl
                else null

            if (!isIncrementalCompilationEnabled()) {
-                clearLocalState("IC is disabled")
+                cleanOutputsAndLocalState("IC is disabled")
            } else if (!inputChanges.isIncremental) {
-                clearLocalState("Task cannot run incrementally")
+                cleanOutputsAndLocalState("Task cannot run incrementally")
            }

            executeImpl(inputChanges, outputsBackup)
@@ -648,6 +652,13 @@ abstract class KotlinCompile @Inject constructor(
        abstract val classpathSnapshotDir: DirectoryProperty
    }

+    override val incrementalProps: List<FileCollection>
+        get() = listOf(stableSources, commonSourceSet, classpathSnapshotProperties.classpath, classpathSnapshotProperties.classpathSnapshot)
+
+    // Exclude classpathSnapshotDir from TaskOutputsBackup (see TaskOutputsBackup's kdoc for more info). */
+    override val taskOutputsBackupExcludes: List<File>
+        get() = classpathSnapshotProperties.classpathSnapshotDir.orNull?.asFile?.let { listOf(it) } ?: emptyList()
+
    @get:Internal
    internal val defaultKotlinJavaToolchain: Provider<DefaultKotlinJavaToolchain> = objects
        .propertyWithNewInstance(
@@ -712,9 +723,6 @@ abstract class KotlinCompile @Inject constructor(
        KotlinJvmCompilerArgumentsContributor(KotlinJvmCompilerArgumentsProvider(this))
    }

-    override val incrementalProps: List<FileCollection>
-        get() = listOf(stableSources, commonSourceSet, classpathSnapshotProperties.classpath, classpathSnapshotProperties.classpathSnapshot)
-
    override fun getSourceRoots(): SourceRoots.ForJvm = jvmSourceRoots

    override fun validateCompilerArguments(args: K2JVMCompilerArguments) {
@@ -745,9 +753,14 @@ abstract class KotlinCompile @Inject constructor(
            )
        } else null

+        @Suppress("ConvertArgumentToSet")
        val environment = GradleCompilerEnvironment(
            defaultCompilerClasspath, messageCollector, outputItemCollector,
-            outputFiles = allOutputFiles(),
+            // In the incremental compiler, outputFiles will be cleaned on rebuild. However, because classpathSnapshotDir is not included in
+            // TaskOutputsBackup, we don't want classpathSnapshotDir to be cleaned immediately on rebuild, and therefore we exclude it from
+            // outputFiles here. (See TaskOutputsBackup's kdoc for more info.)
+            outputFiles = allOutputFiles()
+                    - (classpathSnapshotProperties.classpathSnapshotDir.orNull?.asFile?.let { setOf(it) } ?: emptySet()),
            reportingSettings = reportingSettings,
            incrementalCompilationEnvironment = icEnv,
            kotlinScriptExtensions = sourceFilesExtensions.get().toTypedArray()
@@ -1149,4 +1162,3 @@ data class KotlinCompilerPluginData(
        val outputFiles: Set<File>
    )
 }
-
@@ -20,13 +20,29 @@ internal class TaskOutputsBackup(
    val fileSystemOperations: FileSystemOperations,
    val buildDirectory: DirectoryProperty,
    val snapshotsDir: Provider<Directory>,
-    val outputs: FileCollection,
+
+    allOutputs: List<File>,
+
+    /**
+     * Task outputs that we don't want to back up for performance reasons (e.g., if (1) they are too big, and (2) they are usually updated
+     * only at the end of the task execution--in a failed task run, they are usually unchanged and therefore don't need to be restored).
+     *
+     * NOTE: In `IncrementalCompilerRunner`, if incremental compilation fails, it will try again by cleaning all the outputs and perform
+     * non-incremental compilation. It is important that `IncrementalCompilerRunner` do not clean [outputsToExclude] immediately but only
+     * right before [outputsToExclude] are updated (which is usually at the end of the task execution). This is so that if the fallback
+     * compilation fails, [outputsToExclude] will remain unchanged and the other outputs will be restored, and the next task run can be
+     * incremental.
+     */
+    outputsToExclude: List<File> = emptyList(),
    val logger: Logger
 ) {
+    /** The outputs to back up and restore. Note that this may be a subset of all the outputs of a task (see `outputsToExclude`). */
+    val outputs: List<File> = allOutputs - outputsToExclude.toSet()
+
    fun createSnapshot() {
        // Kotlin JS compilation task declares one file from 'destinationDirectory' output as task `@OutputFile'
        // property. To avoid snapshot sync collisions, each snapshot output directory has also 'index' as prefix.
-        outputs.files.toSortedSet().forEachIndexed { index, outputPath ->
+        outputs.toSortedSet().forEachIndexed { index, outputPath ->
            val pathInSnapshot = "$index${File.separator}${outputPath.pathRelativeToBuildDirectory}"
            if (outputPath.isDirectory) {
                snapshotsDir
@@ -53,7 +69,7 @@ internal class TaskOutputsBackup(
            it.delete(outputs)
        }

-        outputs.files.toSortedSet().forEachIndexed { index, outputPath ->
+        outputs.toSortedSet().forEachIndexed { index, outputPath ->
            val pathInSnapshot = "$index${File.separator}${outputPath.pathRelativeToBuildDirectory}"
            val fileInSnapshot = snapshotsDir.get().file(pathInSnapshot).asFile
            if (fileInSnapshot.isDirectory) {
@@ -6,10 +6,12 @@ import org.jetbrains.kotlin.build.report.metrics.BuildTime
 import org.jetbrains.kotlin.build.report.metrics.measure
 import org.jetbrains.kotlin.cli.common.ExitCode
 import org.jetbrains.kotlin.compilerRunner.KotlinLogger
-import org.jetbrains.kotlin.gradle.logging.GradleKotlinLogger
 import org.jetbrains.kotlin.gradle.internal.tasks.TaskWithLocalState
 import org.jetbrains.kotlin.gradle.internal.tasks.allOutputFiles
+import org.jetbrains.kotlin.gradle.logging.GradleKotlinLogger
 import org.jetbrains.kotlin.gradle.logging.kotlinDebug
+import org.jetbrains.kotlin.incremental.cleanDirectoryContents
+import org.jetbrains.kotlin.incremental.forceDeleteRecursively
 import java.io.File

 fun throwGradleExceptionIfError(exitCode: ExitCode) {
@@ -23,12 +25,12 @@ fun throwGradleExceptionIfError(exitCode: ExitCode) {
    }
 }

-internal fun TaskWithLocalState.clearLocalState(reason: String? = null) {
+internal fun TaskWithLocalState.cleanOutputsAndLocalState(reason: String? = null) {
    val log = GradleKotlinLogger(logger)
-    clearLocalState(allOutputFiles(), log, metrics.get(), reason)
+    cleanOutputsAndLocalState(allOutputFiles(), log, metrics.get(), reason)
 }

-internal fun clearLocalState(
+internal fun cleanOutputsAndLocalState(
    outputFiles: Iterable<File>,
    log: KotlinLogger,
    metrics: BuildMetricsReporter,
@@ -36,21 +38,19 @@ internal fun clearLocalState(
 ) {
    log.kotlinDebug {
        val suffix = reason?.let { " ($it)" }.orEmpty()
-        "Clearing output$suffix:"
+        "Cleaning output$suffix:"
    }

    metrics.measure(BuildTime.CLEAR_OUTPUT) {
        for (file in outputFiles) {
-            if (!file.exists()) continue
            when {
                file.isDirectory -> {
-                    log.debug("Deleting output directory: $file")
-                    file.deleteRecursively()
-                    file.mkdirs()
+                    log.debug("Deleting contents of output directory: $file")
+                    file.cleanDirectoryContents()
                }
                file.isFile -> {
                    log.debug("Deleting output file: $file")
-                    file.delete()
+                    file.forceDeleteRecursively()
                }
            }
        }