From a4c62d156f12881215d3eb6b26dfac94eb76959e Mon Sep 17 00:00:00 2001 From: Alexey Tsvetkov Date: Sun, 14 Jul 2019 20:39:15 +0300 Subject: [PATCH] Make lookup storage addAll order independent Lookup storage output files could differ for projects with different absolute paths. This happened because, paths for lookups were relativized only before writing to the underlying storage. Storing absolute paths in a hash table could result in different order of adding files to the lookup storage. This commit fixes the issue by sorting lookups and files in LookupStorage#addAll #KT-32674 Fixed --- .../kotlin/incremental/LookupStorage.kt | 18 ++-- .../jetbrains/kotlin/incremental/buildUtil.kt | 3 +- .../storage/RelativeFileToPathConverter.kt | 34 +++++++ .../storage/RelocatableCachesTest.kt | 90 +++++++++++++++++++ .../experimental/RemoteLookupTrackerClient.kt | 7 +- .../jps/build/JpsFileToPathConverter.kt | 30 +------ .../kotlin/jps/build/KotlinBuilder.kt | 2 +- 7 files changed, 146 insertions(+), 38 deletions(-) create mode 100644 build-common/src/org/jetbrains/kotlin/incremental/storage/RelativeFileToPathConverter.kt create mode 100644 build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableCachesTest.kt diff --git a/build-common/src/org/jetbrains/kotlin/incremental/LookupStorage.kt b/build-common/src/org/jetbrains/kotlin/incremental/LookupStorage.kt index b865cd9646b..6dc7422835c 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/LookupStorage.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/LookupStorage.kt @@ -75,12 +75,13 @@ open class LookupStorage( } @Synchronized - fun addAll(lookups: Set>>, allPaths: Set) { - val pathToId = allPaths.keysToMap { addFileIfNeeded(File(it)) } + fun addAll(lookups: MultiMap, allPaths: Set) { + val pathToId = allPaths.sorted().keysToMap { addFileIfNeeded(File(it)) } - for ((lookupSymbol, paths) in lookups) { + for (lookupSymbol in lookups.keySet().sorted()) { val key = LookupSymbolKey(lookupSymbol.name, lookupSymbol.scope) - val fileIds = paths.mapTo(HashSet()) { pathToId[it]!! } + val paths = lookups[lookupSymbol]!! + val fileIds = paths.mapTo(TreeSet()) { pathToId[it]!! } fileIds.addAll(lookupMap[key] ?: emptySet()) lookupMap[key] = fileIds } @@ -227,4 +228,11 @@ class LookupTrackerImpl(private val delegate: LookupTracker) : LookupTracker { } } -data class LookupSymbol(val name: String, val scope: String) +data class LookupSymbol(val name: String, val scope: String) : Comparable { + override fun compareTo(other: LookupSymbol): Int { + val scopeCompare = scope.compareTo(other.scope) + if (scopeCompare != 0) return scopeCompare + + return name.compareTo(other.name) + } +} diff --git a/build-common/src/org/jetbrains/kotlin/incremental/buildUtil.kt b/build-common/src/org/jetbrains/kotlin/incremental/buildUtil.kt index 6bbc43cef2d..947486660e3 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/buildUtil.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/buildUtil.kt @@ -30,6 +30,7 @@ import org.jetbrains.kotlin.name.FqName import org.jetbrains.kotlin.progress.CompilationCanceledStatus import org.jetbrains.kotlin.synthetic.SAM_LOOKUP_NAME import org.jetbrains.kotlin.utils.addToStdlib.flattenTo +import org.jetbrains.kotlin.utils.keysToMap import java.io.File import java.util.* import kotlin.collections.HashSet @@ -126,7 +127,7 @@ fun LookupStorage.update( removeLookupsFrom(filesToCompile.asSequence() + removedFiles.asSequence()) - addAll(lookupTracker.lookups.entrySet(), lookupTracker.pathInterner.values) + addAll(lookupTracker.lookups, lookupTracker.pathInterner.values) } data class DirtyData( diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/RelativeFileToPathConverter.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/RelativeFileToPathConverter.kt new file mode 100644 index 00000000000..dd16c7b7832 --- /dev/null +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/RelativeFileToPathConverter.kt @@ -0,0 +1,34 @@ +/* + * Copyright 2010-2019 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.incremental.storage + +import java.io.File + +open class RelativeFileToPathConverter(baseDirFile: File?) : FileToPathConverter { + private val baseDirPath = baseDirFile?.canonicalFile?.invariantSeparatorsPath + + override fun toPath(file: File): String { + val path = file.canonicalFile.invariantSeparatorsPath + return when { + baseDirPath != null && path.startsWith(baseDirPath) -> + PROJECT_DIR_PLACEHOLDER + path.substring(baseDirPath.length) + else -> path + } + } + + override fun toFile(path: String): File = + when { + path.startsWith(PROJECT_DIR_PLACEHOLDER) -> { + val basePath = baseDirPath ?: error("Could not get project root dir") + File(basePath + path.substring(PROJECT_DIR_PLACEHOLDER.length)) + } + else -> File(path) + } + + private companion object { + private const val PROJECT_DIR_PLACEHOLDER = "${'$'}PROJECT_DIR$" + } +} \ No newline at end of file diff --git a/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableCachesTest.kt b/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableCachesTest.kt new file mode 100644 index 00000000000..30a72275506 --- /dev/null +++ b/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableCachesTest.kt @@ -0,0 +1,90 @@ +/* + * Copyright 2010-2019 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.incremental.storage + +import com.intellij.util.containers.MultiMap +import org.jetbrains.kotlin.TestWithWorkingDir +import org.jetbrains.kotlin.incremental.LookupStorage +import org.jetbrains.kotlin.incremental.LookupSymbol +import org.jetbrains.kotlin.incremental.testingUtils.assertEqualDirectories +import org.junit.Test +import java.io.File + +class RelocatableCachesTest : TestWithWorkingDir() { + @Test + fun testLookupStorageAddAllReversedFiles() { + val originalRoot = workingDir.resolve("original") + fillLookupStorage(originalRoot, reverseFiles = false, reverseLookups = false) + val reversedFilesOrderRoot = workingDir.resolve("reversedFiles") + fillLookupStorage(reversedFilesOrderRoot, reverseFiles = true, reverseLookups = false) + assertEqualDirectories(originalRoot, reversedFilesOrderRoot, forgiveExtraFiles = false) + } + + @Test + fun testLookupStorageAddAllReversedLookups() { + val originalRoot = workingDir.resolve("original") + fillLookupStorage(originalRoot, reverseFiles = false, reverseLookups = false) + val reversedLookupsOrderRoot = workingDir.resolve("reversedLookups") + fillLookupStorage(reversedLookupsOrderRoot, reverseFiles = false, reverseLookups = true) + assertEqualDirectories(originalRoot, reversedLookupsOrderRoot, forgiveExtraFiles = false) + } + + @Test + fun testLookupStorageAddAllReversedFilesReversedLookups() { + val originalRoot = workingDir.resolve("original") + fillLookupStorage(originalRoot, reverseFiles = false, reverseLookups = false) + val reversedFilesReversedLookupsOrderRoot = workingDir.resolve("reversedFilesReversedLookupsOrderRoot") + fillLookupStorage(reversedFilesReversedLookupsOrderRoot, reverseFiles = true, reverseLookups = true) + assertEqualDirectories(originalRoot, reversedFilesReversedLookupsOrderRoot, forgiveExtraFiles = false) + } + + /** + * Fills lookup storage in [projectRoot] with N fq-names, + * where i_th fq-name myscope_i.MyClass_i has lookups for previous fq-names (from 0 to i-1) + */ + private fun fillLookupStorage(projectRoot: File, reverseFiles: Boolean, reverseLookups: Boolean) { + val storageRoot = projectRoot.storageRoot + val fileToPathConverter = RelativeFileToPathConverter(projectRoot) + val lookupStorage = LookupStorage(storageRoot, fileToPathConverter) + val files = LinkedHashSet() + val symbols = LinkedHashSet() + val lookups = MultiMap.createOrderedSet() + + for (i in 0..10) { + val newSymbol = LookupSymbol(name = "MyClass_$i", scope = "myscope_$i") + val newSourcePath = projectRoot.resolve("src/${newSymbol.asRelativePath()}").canonicalFile.invariantSeparatorsPath + symbols.add(newSymbol) + + for (lookedUpSymbol in symbols) { + lookups.putValue(lookedUpSymbol, newSourcePath) + } + + files.add(newSourcePath) + } + + val filesToAdd = if (reverseFiles) files.reversedSet() else files + val lookupsToAdd = if (reverseLookups) lookups.reversedMultiMap() else lookups + lookupStorage.addAll(lookupsToAdd, filesToAdd) + lookupStorage.flush(memoryCachesOnly = false) + } + + private val File.storageRoot: File + get() = resolve("storage") + + private fun MultiMap.reversedMultiMap(): MultiMap { + val newMap = MultiMap.createOrderedSet() + for ((key, values) in entrySet().reversedSet()) { + newMap.putValues(key, values.reversed()) + } + return newMap + } + + private fun Set.reversedSet(): LinkedHashSet = + reversed().toCollection(LinkedHashSet(size)) + + private fun LookupSymbol.asRelativePath(): String = + if (scope.isBlank()) name else scope.replace('.', '/') + '/' + name +} \ No newline at end of file diff --git a/compiler/daemon/src/org/jetbrains/kotlin/daemon/experimental/RemoteLookupTrackerClient.kt b/compiler/daemon/src/org/jetbrains/kotlin/daemon/experimental/RemoteLookupTrackerClient.kt index 1ae91b8335d..36a9bcf7d5d 100644 --- a/compiler/daemon/src/org/jetbrains/kotlin/daemon/experimental/RemoteLookupTrackerClient.kt +++ b/compiler/daemon/src/org/jetbrains/kotlin/daemon/experimental/RemoteLookupTrackerClient.kt @@ -8,11 +8,8 @@ package org.jetbrains.kotlin.daemon.experimental import com.intellij.util.containers.StringInterner import kotlinx.coroutines.runBlocking import org.jetbrains.kotlin.daemon.EventManager +import org.jetbrains.kotlin.daemon.common.* import org.jetbrains.kotlin.daemon.common.experimental.CompilerCallbackServicesFacadeClientSide -import org.jetbrains.kotlin.daemon.common.DummyProfiler -import org.jetbrains.kotlin.daemon.common.Profiler -import org.jetbrains.kotlin.daemon.common.withMeasure -import org.jetbrains.kotlin.daemon.common.withMeasureBlocking import org.jetbrains.kotlin.incremental.components.LookupInfo import org.jetbrains.kotlin.incremental.components.LookupTracker import org.jetbrains.kotlin.incremental.components.Position @@ -20,7 +17,7 @@ import org.jetbrains.kotlin.incremental.components.ScopeKind class RemoteLookupTrackerClient( - val facade: CompilerCallbackServicesFacadeClientSide, + val facade: CompilerCallbackServicesFacadeAsync, eventManager: EventManager, val profiler: Profiler = DummyProfiler() ) : LookupTracker { diff --git a/jps-plugin/src/org/jetbrains/kotlin/jps/build/JpsFileToPathConverter.kt b/jps-plugin/src/org/jetbrains/kotlin/jps/build/JpsFileToPathConverter.kt index c8fe4555df7..4cdd0581928 100644 --- a/jps-plugin/src/org/jetbrains/kotlin/jps/build/JpsFileToPathConverter.kt +++ b/jps-plugin/src/org/jetbrains/kotlin/jps/build/JpsFileToPathConverter.kt @@ -7,30 +7,8 @@ package org.jetbrains.kotlin.jps.build import org.jetbrains.jps.model.JpsProject import org.jetbrains.jps.model.serialization.JpsModelSerializationDataService -import org.jetbrains.kotlin.incremental.storage.FileToPathConverter -import java.io.File +import org.jetbrains.kotlin.incremental.storage.RelativeFileToPathConverter -private const val PROJECT_DIR_PLACEHOLDER = "${'$'}PROJECT_DIR$" - -internal class JpsFileToPathConverter(jpsProject: JpsProject) : FileToPathConverter { - private val baseDirPath = JpsModelSerializationDataService - .getBaseDirectory(jpsProject)?.canonicalFile?.invariantSeparatorsPath - - override fun toPath(file: File): String { - val path = file.canonicalFile.invariantSeparatorsPath - return when { - baseDirPath != null && path.startsWith(baseDirPath) -> - PROJECT_DIR_PLACEHOLDER + path.substring(baseDirPath.length) - else -> path - } - } - - override fun toFile(path: String): File = - when { - path.startsWith(PROJECT_DIR_PLACEHOLDER) -> { - val basePath = baseDirPath ?: error("Could not get project root dir") - File(basePath + path.substring(PROJECT_DIR_PLACEHOLDER.length)) - } - else -> File(path) - } -} \ No newline at end of file +internal class JpsFileToPathConverter( + jpsProject: JpsProject +) : RelativeFileToPathConverter(JpsModelSerializationDataService.getBaseDirectory(jpsProject)) diff --git a/jps-plugin/src/org/jetbrains/kotlin/jps/build/KotlinBuilder.kt b/jps-plugin/src/org/jetbrains/kotlin/jps/build/KotlinBuilder.kt index a549d760545..b32b63ec971 100644 --- a/jps-plugin/src/org/jetbrains/kotlin/jps/build/KotlinBuilder.kt +++ b/jps-plugin/src/org/jetbrains/kotlin/jps/build/KotlinBuilder.kt @@ -648,7 +648,7 @@ class KotlinBuilder : ModuleLevelBuilder(BuilderCategory.SOURCE_PROCESSOR) { lookupStorageManager.withLookupStorage { lookupStorage -> lookupStorage.removeLookupsFrom(dirtyFilesHolder.allDirtyFiles.asSequence() + dirtyFilesHolder.allRemovedFilesFiles.asSequence()) - lookupStorage.addAll(lookupTracker.lookups.entrySet(), lookupTracker.pathInterner.values) + lookupStorage.addAll(lookupTracker.lookups, lookupTracker.pathInterner.values) } } }