[IC] Fix fallback logic in IncrementalCompilerRunner

The current logic works as follows: - Try either incremental compilation or non-incremental compilation - If the above (or any of its surrounding work) fails, fall back to non-incremental compilation This means we may perform non-incremental compilation twice. This commit will fix that logic so that we fall back to non-incremental compilation only if *incremental compilation* fails. A nice consequence of this change is that it also resolves the critical bugs described at KT-52669 (which occur because the current logic is flawed).
JetBrains · Jun 29, 2022 · 39d59cb · 39d59cb
1 parent aab426c
commit 39d59cb
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 89 deletions.
diff --git a/build-common/src/org/jetbrains/kotlin/build/report/metrics/BuildAttribute.kt b/build-common/src/org/jetbrains/kotlin/build/report/metrics/BuildAttribute.kt
@@ -18,8 +18,9 @@ enum class BuildAttributeKind : Serializable {
 enum class BuildAttribute(val kind: BuildAttributeKind, val readableString: String) : Serializable {
     NO_BUILD_HISTORY(BuildAttributeKind.REBUILD_REASON, "Build history file not found"),
     NO_ABI_SNAPSHOT(BuildAttributeKind.REBUILD_REASON, "ABI snapshot not found"),
+    INTERNAL_ERROR(BuildAttributeKind.REBUILD_REASON, "Internal error during preparation of IC round"),
     CLASSPATH_SNAPSHOT_NOT_FOUND(BuildAttributeKind.REBUILD_REASON, "Classpath snapshot not found"),
-    CACHE_CORRUPTION(BuildAttributeKind.REBUILD_REASON, "Cache corrupted"),
+    INCREMENTAL_COMPILATION_FAILED(BuildAttributeKind.REBUILD_REASON, "Incremental compilation failed"),
     UNKNOWN_CHANGES_IN_GRADLE_INPUTS(BuildAttributeKind.REBUILD_REASON, "Unknown Gradle changes"),
     JAVA_CHANGE_UNTRACKED_FILE_IS_REMOVED(BuildAttributeKind.REBUILD_REASON, "Untracked Java file is removed"),
     JAVA_CHANGE_UNEXPECTED_PSI(BuildAttributeKind.REBUILD_REASON, "Java PSI file is expected"),

diff --git a/...emental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCachesManager.kt b/...emental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCachesManager.kt
@@ -34,6 +34,7 @@ abstract class IncrementalCachesManager<PlatformCache : AbstractIncrementalCache
     private val caches = arrayListOf<BasicMapsOwner>()
 
     var isClosed = false
+    var isSuccessfulyClosed = false
 
     @Synchronized
     protected fun <T : BasicMapsOwner> T.registerCache() {
@@ -52,29 +53,29 @@ abstract class IncrementalCachesManager<PlatformCache : AbstractIncrementalCache
     @Synchronized
     fun close(flush: Boolean = false): Boolean {
         if (isClosed) {
-            return true
+            return isSuccessfulyClosed
         }
-        var successful = true
+        isSuccessfulyClosed = true
         for (cache in caches) {
             if (flush) {
                 try {
                     cache.flush(false)
                 } catch (e: Throwable) {
-                    successful = false
+                    isSuccessfulyClosed = false
                     reporter.report { "Exception when flushing cache ${cache.javaClass}: $e" }
                 }
             }
 
             try {
                 cache.close()
             } catch (e: Throwable) {
-                successful = false
+                isSuccessfulyClosed = false
                 reporter.report { "Exception when closing cache ${cache.javaClass}: $e" }
             }
         }
 
         isClosed = true
-        return successful
+        return isSuccessfulyClosed
     }
 }
 

diff --git a/...mental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCompilerRunner.kt b/...mental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCompilerRunner.kt
@@ -71,7 +71,53 @@ abstract class IncrementalCompilerRunner<
         providedChangedFiles: ChangedFiles?,
         projectDir: File? = null
     ): ExitCode = reporter.measure(BuildTime.INCREMENTAL_COMPILATION_DAEMON) {
-        compileImpl(allSourceFiles, args, messageCollector, providedChangedFiles, projectDir)
+        try {
+            compileImpl(allSourceFiles, args, messageCollector, providedChangedFiles, projectDir)
+        } finally {
+            reporter.measure(BuildTime.CALCULATE_OUTPUT_SIZE) {
+                reporter.addMetric(
+                    BuildPerformanceMetric.SNAPSHOT_SIZE,
+                    buildHistoryFile.length() + lastBuildInfoFile.length() + abiSnapshotFile.length()
+                )
+                if (cacheDirectory.exists() && cacheDirectory.isDirectory()) {
+                    cacheDirectory.walkTopDown().filter { it.isFile }.map { it.length() }.sum().let {
+                        reporter.addMetric(BuildPerformanceMetric.CACHE_DIRECTORY_SIZE, it)
+                    }
+                }
+            }
+        }
+    }
+
+    fun rebuild(
+        reason: BuildAttribute,
+        allSourceFiles: List<File>,
+        args: Args,
+        messageCollector: MessageCollector,
+        providedChangedFiles: ChangedFiles?,
+        projectDir: File? = null,
+        classpathAbiSnapshot: Map<String, AbiSnapshot>
+    ): ExitCode {
+        reporter.report { "Non-incremental compilation will be performed: $reason" }
+        reporter.measure(BuildTime.CLEAR_OUTPUT_ON_REBUILD) {
+            cleanOutputsAndLocalStateOnRebuild(args)
+        }
+        val caches = createCacheManager(args, projectDir)
+        try {
+            if (providedChangedFiles == null) {
+                caches.inputsCache.sourceSnapshotMap.compareAndUpdate(allSourceFiles)
+            }
+            val allKotlinFiles = allSourceFiles.filter { it.isKotlinFile(kotlinSourceFilesExtensions) }
+            return compileIncrementally(
+                args, caches, allKotlinFiles, CompilationMode.Rebuild(reason), messageCollector, withAbiSnapshot,
+                classpathAbiSnapshot = classpathAbiSnapshot
+            ).also {
+                if (it == ExitCode.OK) {
+                    performWorkAfterSuccessfulCompilation(caches)
+                }
+            }
+        } finally {
+            caches.close(true)
+        }
     }
 
     private fun compileImpl(
@@ -82,41 +128,19 @@ abstract class IncrementalCompilerRunner<
         projectDir: File? = null
     ): ExitCode {
         var caches = createCacheManager(args, projectDir)
+        var rebuildReason = BuildAttribute.INTERNAL_ERROR
 
-        if (withAbiSnapshot) {
-            reporter.report { "Incremental compilation with ABI snapshot enabled" }
-        }
-        //TODO if abi-snapshot is corrupted unable to rebuild. Should roll back to withSnapshot = false?
         val classpathAbiSnapshot =
             if (withAbiSnapshot) {
+                reporter.report { "Incremental compilation with ABI snapshot enabled" }
                 reporter.measure(BuildTime.SET_UP_ABI_SNAPSHOTS) {
                     setupJarDependencies(args, withAbiSnapshot, reporter)
                 }
             } else {
                 emptyMap()
             }
 
-        fun rebuild(reason: BuildAttribute): ExitCode {
-            reporter.report { "Non-incremental compilation will be performed: $reason" }
-            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) {
-                cleanOutputsAndLocalStateOnRebuild(args)
-            }
-            caches = createCacheManager(args, projectDir)
-            if (providedChangedFiles == null) {
-                caches.inputsCache.sourceSnapshotMap.compareAndUpdate(allSourceFiles)
-            }
-            val allKotlinFiles = allSourceFiles.filter { it.isKotlinFile(kotlinSourceFilesExtensions) }
-            return compileIncrementally(
-                args, caches, allKotlinFiles, CompilationMode.Rebuild(reason), messageCollector, withAbiSnapshot,
-                classpathAbiSnapshot = classpathAbiSnapshot
-            )
-        }
-
-        // If compilation has crashed or we failed to close caches we have to clear them
-        var cachesMayBeCorrupted = true
-        return try {
+        try {
             val changedFiles = when (providedChangedFiles) {
                 is ChangedFiles.Dependencies -> {
                     val changedSources = caches.inputsCache.sourceSnapshotMap.compareAndUpdate(allSourceFiles)
@@ -129,75 +153,51 @@ abstract class IncrementalCompilerRunner<
                 else -> providedChangedFiles
             }
 
-            @Suppress("MoveVariableDeclarationIntoWhen")
-            val compilationMode = sourcesToCompile(caches, changedFiles, args, messageCollector, classpathAbiSnapshot)
+            var compilationMode = sourcesToCompile(caches, changedFiles, args, messageCollector, classpathAbiSnapshot)
+            val abiSnapshot = if (compilationMode is CompilationMode.Incremental && withAbiSnapshot) {
+                AbiSnapshotImpl.read(abiSnapshotFile, reporter)
+            } else {
+                if (withAbiSnapshot) {
+                    compilationMode = CompilationMode.Rebuild(BuildAttribute.NO_ABI_SNAPSHOT)
+                }
+                null
+            }
 
-            val exitCode = when (compilationMode) {
+            when (compilationMode) {
                 is CompilationMode.Incremental -> {
-                    if (withAbiSnapshot) {
-                        val abiSnapshot = AbiSnapshotImpl.read(abiSnapshotFile, reporter)
-                        if (abiSnapshot != null) {
+                    try {
+                        val exitCode = if (withAbiSnapshot) {
                             compileIncrementally(
-                                args,
-                                caches,
-                                allSourceFiles,
-                                compilationMode,
-                                messageCollector,
-                                withAbiSnapshot,
-                                abiSnapshot,
-                                classpathAbiSnapshot
+                                args, caches, allSourceFiles, compilationMode, messageCollector,
+                                withAbiSnapshot, abiSnapshot!!, classpathAbiSnapshot
                             )
                         } else {
-                            rebuild(BuildAttribute.NO_ABI_SNAPSHOT)
+                            compileIncrementally(args, caches, allSourceFiles, compilationMode, messageCollector, withAbiSnapshot)
                         }
-                    } else {
-                        compileIncrementally(
-                            args,
-                            caches,
-                            allSourceFiles,
-                            compilationMode,
-                            messageCollector,
-                            withAbiSnapshot
-                        )
-                    }
-                }
-                is CompilationMode.Rebuild -> {
-                    rebuild(compilationMode.reason)
-                }
-            }
-
-            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,
-            // but now JsKlib compiler reports invalid exit code.
-            cachesMayBeCorrupted = false
-
-            reporter.measure(BuildTime.CALCULATE_OUTPUT_SIZE) {
-                reporter.addMetric(
-                    BuildPerformanceMetric.SNAPSHOT_SIZE,
-                    buildHistoryFile.length() + lastBuildInfoFile.length() + abiSnapshotFile.length()
-                )
-                if (cacheDirectory.exists() && cacheDirectory.isDirectory()) {
-                    cacheDirectory.walkTopDown().filter { it.isFile }.map { it.length() }.sum().let {
-                        reporter.addMetric(BuildPerformanceMetric.CACHE_DIRECTORY_SIZE, it)
+                        if (exitCode == ExitCode.OK) {
+                            performWorkAfterSuccessfulCompilation(caches)
+                        }
+                        return exitCode
+                    } catch (e: Throwable) {
+                        reporter.report {
+                            "Incremental compilation failed: ${e.stackTraceToString()}.\nFalling back to non-incremental compilation."
+                        }
+                        rebuildReason = BuildAttribute.INCREMENTAL_COMPILATION_FAILED
                     }
                 }
+                is CompilationMode.Rebuild -> rebuildReason = compilationMode.reason
             }
-            return exitCode
-        } catch (e: Exception) { // todo: catch only cache corruption
-            // todo: warn?
-            reporter.report { "Possible caches corruption: $e" }
-            rebuild(BuildAttribute.CACHE_CORRUPTION).also {
-                cachesMayBeCorrupted = false
+        } catch (e: Exception) {
+            reporter.report {
+                "Incremental compilation analysis failed: ${e.stackTraceToString()}.\nFalling back to non-incremental compilation."
             }
         } finally {
-            if (cachesMayBeCorrupted) {
+            if (!caches.close()) {
+                reporter.report { "Unable to close IC caches. Cleaning internal state" }
                 cleanOutputsAndLocalStateOnRebuild(args)
             }
         }
+        return rebuild(rebuildReason, allSourceFiles, args, messageCollector, providedChangedFiles, projectDir, classpathAbiSnapshot)
     }
 
     /**
@@ -411,7 +411,10 @@ abstract class IncrementalCompilerRunner<
                 break
             }
 
-            val (dirtyLookupSymbols, dirtyClassFqNames, forceRecompile) = changesCollector.getDirtyData(listOf(caches.platformCache), reporter)
+            val (dirtyLookupSymbols, dirtyClassFqNames, forceRecompile) = changesCollector.getDirtyData(
+                listOf(caches.platformCache),
+                reporter
+            )
             val compiledInThisIterationSet = sourcesToCompile.toHashSet()
 
             val forceToRecompileFiles = mapClassesFqNamesToFiles(listOf(caches.platformCache), forceRecompile, reporter)

diff --git a/...tests/src/test/kotlin/org/jetbrains/kotlin/gradle/IncrementalCompilationMultiProjectIT.kt b/...tests/src/test/kotlin/org/jetbrains/kotlin/gradle/IncrementalCompilationMultiProjectIT.kt
@@ -675,7 +675,7 @@ abstract class BaseIncrementalCompilationMultiProjectIT : IncrementalCompilation
             }
 
             build("assemble") {
-                assertOutputContains("Non-incremental compilation will be performed: CACHE_CORRUPTION")
+                assertOutputContains("Non-incremental compilation will be performed: INCREMENTAL_COMPILATION_FAILED")
             }
 
             val lookupFile = projectPath.resolve("lib/build/kotlin/${compileKotlinTaskName}/cacheable/${compileCacheFolderName}/lookups/file-to-id.tab")