From 5a2ff8669132c9fedd99762f0d5fac5f4b7f7ea8 Mon Sep 17 00:00:00 2001 From: Ivan Gavrilovic Date: Tue, 16 Jul 2019 14:23:52 +0100 Subject: [PATCH] Handle classpath snapshot changes better Because Kapt Gradle task uses PathSensitivity.NONE for the input representing the classpath structure, there are cases when paths of the files may change, but because content is the same, there will be no incremental changes. Classpath snapshot comparison did not handle this case correctly, but this commit fixes that. In more details: - classpath entries with the same path and that are not reported as changed will have their information loaded from the previous snapshot - any other entry (changed path or changed content) will be reloaded Test: ClasspathSnapshotTest --- .../kapt/incremental/ClasspathSnapshot.kt | 41 ++++++++----------- .../kapt/incremental/ClasspathSnapshotTest.kt | 16 ++++++++ 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshot.kt b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshot.kt index 5e177539fd7..0f02ea2dc56 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshot.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshot.kt @@ -54,36 +54,20 @@ open class ClasspathSnapshot protected constructor( return KaptClasspathChanges.Unknown } - loadEntriesFor(changedFiles) + val unchangedBetweenCompilations = dataForFiles.keys.intersect(previousSnapshot.dataForFiles.keys).filter { it !in changedFiles } + val currentToLoad = dataForFiles.keys.filter { it !in unchangedBetweenCompilations }.also { loadEntriesFor(it) } + val previousToLoad = previousSnapshot.dataForFiles.keys.filter { it !in unchangedBetweenCompilations } - val currentHashAbiSize = changedFiles.sumBy { dataForFiles[it]!!.classAbiHash.size } - val currentHashesToAnalyze = - HashMap(currentHashAbiSize).also { hashes -> - changedFiles.forEach { - hashes.putAll(dataForFiles[it]!!.classAbiHash) - } - } - - val currentUnchanged = dataForFiles.keys.filter { it !in changedFiles } - val previousChanged = previousSnapshot.dataForFiles.keys.filter { it !in currentUnchanged } - - check(changedFiles.size == previousChanged.size) { + check(currentToLoad.size == previousToLoad.size) { """ - Number of changed files in snapshots differs. Reported changed files: $changedFiles + Number of loaded files in snapshots differs. Reported changed files: $changedFiles Current snapshot data files: ${dataForFiles.keys} Previous snapshot data files: ${previousSnapshot.dataForFiles.keys} """.trimIndent() } - val previousHashAbiSize = previousChanged.sumBy { previousSnapshot.dataForFiles.get(it)?.classAbiHash?.size ?: 0 } - val previousHashesToAnalyze = - HashMap(previousHashAbiSize).also { hashes -> - for (c in previousChanged) { - previousSnapshot.dataForFiles[c]?.let { - hashes.putAll(it.classAbiHash) - } - } - } + val currentHashesToAnalyze = getHashesToAnalyze(currentToLoad) + val previousHashesToAnalyze = previousSnapshot.getHashesToAnalyze(previousToLoad) val changedClasses = mutableSetOf() for (key in previousHashesToAnalyze.keys + currentHashesToAnalyze.keys) { @@ -104,7 +88,7 @@ open class ClasspathSnapshot protected constructor( // We do not compute structural data for unchanged files of the current snapshot for performance reasons. // That is why we reuse the previous snapshot as that one contains all unchanged entries. - for (unchanged in currentUnchanged) { + for (unchanged in unchangedBetweenCompilations) { dataForFiles[unchanged] = previousSnapshot.dataForFiles[unchanged]!! } @@ -113,6 +97,15 @@ open class ClasspathSnapshot protected constructor( return KaptClasspathChanges.Known(allImpactedClasses) } + private fun getHashesToAnalyze(filesToLoad: List): HashMap { + val hashAbiSize = filesToLoad.sumBy { dataForFiles[it]!!.classAbiHash.size } + return HashMap(hashAbiSize).also { hashes -> + filesToLoad.forEach { + hashes.putAll(dataForFiles[it]!!.classAbiHash) + } + } + } + private fun loadEntriesFor(file: Iterable) { for (f in file) { if (dataForFiles[f] == null) { diff --git a/libraries/tools/kotlin-gradle-plugin/src/test/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshotTest.kt b/libraries/tools/kotlin-gradle-plugin/src/test/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshotTest.kt index 9df969dfc45..ab658d9410d 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/test/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshotTest.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/test/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshotTest.kt @@ -83,6 +83,22 @@ class ClasspathSnapshotTest { assertEquals(setOf("library/C", "first/A", "first/B"), diff.names) } + @Test + fun testNoChangedFileButPathsChanged() { + val dataFile = generateStructureData( + ClassData("first/A"), + ClassData("first/B").also { it.withAbiDependencies("first/A") } + ) + val firstSnapshot = ClasspathSnapshot.ClasspathSnapshotFactory.createCurrent(tmp.newFolder(), listOf(), setOf(dataFile)) + firstSnapshot.writeToCache() + + val copyOfDataFile = dataFile.copyTo(tmp.newFile(), overwrite = true) + val secondSnapshot = ClasspathSnapshot.ClasspathSnapshotFactory.createCurrent(tmp.newFolder(), listOf(), setOf(copyOfDataFile)) + + val diff = secondSnapshot.diff(firstSnapshot, setOf()) as KaptClasspathChanges.Known + assertEquals(emptySet(), diff.names) + } + private fun generateStructureData(vararg classData: ClassData, outputFile: File = tmp.newFile()): File { val data = ClasspathEntryData() classData.forEach {