From ecbf69504a9e4cee234ab500111f834b4546f45b Mon Sep 17 00:00:00 2001 From: Hung Nguyen Date: Mon, 16 Oct 2023 18:30:47 +0000 Subject: [PATCH] [IC] Add back `flush()` API to `PersistentStorage` In commit 4e89dcf, we removed `flush()` from `PersistentStorage` (previously called `LazyStorage`) because it is not used in Gradle builds. However, `flush()` is still used in JPS builds, so we need to add that API back in this commit. #KT-62486 Fixed Co-authored-by: Hung Nguyen Merge-request: KOTLIN-MR-796 Merged-by: Aleksei Cherepanov --- .../kotlin/incremental/LookupStorage.kt | 4 +-- .../kotlin/incremental/storage/BasicMap.kt | 21 ----------- .../incremental/storage/BasicMapsOwner.kt | 35 ++++++++++++++++--- .../incremental/storage/InMemoryStorage.kt | 15 ++++++-- .../kotlin/incremental/storage/LazyStorage.kt | 5 +++ .../incremental/storage/PersistentStorage.kt | 12 +++++-- .../storage/PersistentStorageAdapter.kt | 5 +++ .../incremental/CompilationTransactionTest.kt | 2 ++ .../storage/RelocatableCachesTest.kt | 2 +- 9 files changed, 68 insertions(+), 33 deletions(-) diff --git a/build-common/src/org/jetbrains/kotlin/incremental/LookupStorage.kt b/build-common/src/org/jetbrains/kotlin/incremental/LookupStorage.kt index 97b4a2e37e4..74dbdb9da2a 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/LookupStorage.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/LookupStorage.kt @@ -197,12 +197,12 @@ open class LookupStorage( @TestOnly fun forceGC() { removeGarbageForTests() - flush(false) + flush() } @TestOnly fun dump(lookupSymbols: Set): String { - flush(false) + flush() val sb = StringBuilder() val p = Printer(sb) diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMap.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMap.kt index 6bf77b461fe..66913aef9e0 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMap.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMap.kt @@ -33,15 +33,6 @@ interface BasicMap : PersistentStorage { keys.forEach { remove(it) } } - /** - * DEPRECATED: This method should be removed because - * - The `memoryCachesOnly` parameter is not used (it is always `false`). - * - There is currently no good use case for flushing. In fact, the current implementation of this method does nothing. - */ - fun flush(memoryCachesOnly: Boolean) { - check(!memoryCachesOnly) { "Expected memoryCachesOnly = false but it is `true`" } - } - /** * Deletes [storageFile] or a group of files associated with [storageFile] (e.g., an implementation of [PersistentStorage] may use a * [com.intellij.util.io.PersistentHashMap], which creates files such as "storageFile.tab", "storageFile.tab.len", etc.). @@ -54,18 +45,6 @@ interface BasicMap : PersistentStorage { } } - /** - * DEPRECATED: This method should be removed because: - * - The method name is ambiguous (it may be confused with [clear], and it does not exactly describe the current implementation). - * - The method currently calls close(). However, close() is often already called separately and automatically, so calling this method - * means that close() will likely be called twice. - * Instead, just inline this method or call either close() or deleteStorageFiles() directly. - */ - fun clean() { - close() - deleteStorageFiles() - } - @TestOnly fun dump(): String { return StringBuilder().apply { diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMapsOwner.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMapsOwner.kt index ecb6f052f1f..6b6d270cf7b 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMapsOwner.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/BasicMapsOwner.kt @@ -17,10 +17,11 @@ package org.jetbrains.kotlin.incremental.storage import org.jetbrains.annotations.TestOnly +import java.io.Closeable import java.io.File import java.io.IOException -open class BasicMapsOwner(val cachesDir: File) { +open class BasicMapsOwner(val cachesDir: File) : Closeable { private val maps = arrayListOf>() companion object { @@ -36,11 +37,11 @@ open class BasicMapsOwner(val cachesDir: File) { return map } - fun flush(memoryCachesOnly: Boolean) { - forEachMapSafe("flush") { it.flush(memoryCachesOnly) } + fun flush() { + forEachMapSafe("flush", BasicMap<*, *>::flush) } - open fun close() { + override fun close() { forEachMapSafe("close", BasicMap<*, *>::close) } @@ -48,8 +49,32 @@ open class BasicMapsOwner(val cachesDir: File) { forEachMapSafe("deleteStorageFiles", BasicMap<*, *>::deleteStorageFiles) } + /** + * DEPRECATED: This API should be removed because + * - It's not clear what [memoryCachesOnly] means. + * - In the past, when `memoryCachesOnly=true` we applied a small optimization: Checking + * [com.intellij.util.io.PersistentHashMap.isDirty] before calling [com.intellij.util.io.PersistentHashMap.force]. However, if that + * optimization is useful, it's better to always do it (perhaps inside the [com.intellij.util.io.PersistentHashMap.force] method + * itself) rather than doing it based on the value of this parameter. + * + * Instead, just call [flush] (without a parameter) directly. + */ + fun flush(@Suppress("UNUSED_PARAMETER") memoryCachesOnly: Boolean) { + flush() + } + + /** + * DEPRECATED: This API should be removed because: + * - It's not obvious what "clean" means: It does not exactly describe the current implementation, and it also sounds similar to + * "clear" which means removing all the map entries, but this method does not do that. + * - This method currently calls [close] (and [deleteStorageFiles]). However, [close] is often already called separately and + * automatically, so this API makes it more likely for [close] to be accidentally called twice. + * + * Instead, just call [close] and/or [deleteStorageFiles] explicitly. + */ fun clean() { - forEachMapSafe("clean", BasicMap<*, *>::clean) + close() + deleteStorageFiles() } @Synchronized diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/InMemoryStorage.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/InMemoryStorage.kt index 4762d3f1d3b..c21b10d35c8 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/InMemoryStorage.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/InMemoryStorage.kt @@ -9,11 +9,16 @@ import org.jetbrains.kotlin.utils.ThreadSafe /** * [PersistentStorage] which reads from another underlying [PersistentStorage], keeps all changes to it in memory, and writes the changes - * back to the underlying storage on [applyChanges] or [close]. + * back to the underlying storage on [applyChanges], [flush], or [close]. */ interface InMemoryStorageInterface : PersistentStorage { - /** Applies in-memory changes to the underlying [PersistentStorage], then calls [clearChanges]. */ + /** + * Applies in-memory changes to the underlying [PersistentStorage], then calls [clearChanges]. + * + * Note that the changes are only propagated to the underlying [PersistentStorage], they may not be written to [storageFile] yet. If + * you want to propagate the changes to [storageFile], call [flush] instead. + */ fun applyChanges() /** Removes all in-memory changes. */ @@ -120,6 +125,12 @@ open class InMemoryStorage( removedKeys.clear() } + @Synchronized + override fun flush() { + applyChanges() + storage.flush() + } + @Synchronized override fun close() { applyChanges() diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/LazyStorage.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/LazyStorage.kt index 08f557cc6c9..08d932fb2ee 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/LazyStorage.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/LazyStorage.kt @@ -83,6 +83,11 @@ open class LazyStorage( getStorageIfExists()?.remove(key) } + @Synchronized + override fun flush() { + storage?.force() + } + @Synchronized override fun close() { storage?.close() diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorage.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorage.kt index 67e1f0fd25f..b18061d93b1 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorage.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorage.kt @@ -23,7 +23,7 @@ import java.io.File /** * Represents an in-memory map that is backed by a [storageFile]. * - * Changes to this map may be written to [storageFile] at any time, and it is guaranteed to be written on [close]. + * Changes to this map may be written to [storageFile] at any time, and it is guaranteed to be written on [flush] or [close]. * * This interface is similar to but simpler than [com.intellij.util.io.PersistentMapBase]. */ @@ -43,7 +43,10 @@ interface PersistentStorage : Closeable { fun remove(key: KEY) - /** Writes any remaining in-memory changes to [storageFile] and closes this map. */ + /** Writes any remaining in-memory changes to [storageFile]. */ + fun flush() + + /** Writes any remaining in-memory changes to [storageFile] ([flush]) and closes this map. */ override fun close() } @@ -97,6 +100,11 @@ abstract class PersistentStorageWrapper( storage.remove(key) } + @Synchronized + override fun flush() { + storage.flush() + } + @Synchronized override fun close() { storage.close() diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorageAdapter.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorageAdapter.kt index 4bfddfe880b..3c3a68c6a7f 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorageAdapter.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/PersistentStorageAdapter.kt @@ -47,6 +47,11 @@ abstract class PersistentStorageAdapter { override val keys: Set = emptySet() + override fun flush() {} + override fun close() {} override fun remove(key: Any) {} diff --git a/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableCachesTest.kt b/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableCachesTest.kt index e09ce6734db..77c6dcce350 100644 --- a/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableCachesTest.kt +++ b/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableCachesTest.kt @@ -73,7 +73,7 @@ class RelocatableCachesTest : TestWithWorkingDir() { val filesToAdd = if (reverseFiles) files.reversedSet() else files val lookupsToAdd = if (reverseLookups) lookups.reversedMultiMap() else lookups lookupStorage.addAll(lookupsToAdd, filesToAdd) - lookupStorage.flush(memoryCachesOnly = false) + lookupStorage.flush() } private val File.storageRoot: File