From 9538a69d8c70676c57b7fb82be5ee48052eb2f89 Mon Sep 17 00:00:00 2001 From: Nikita Ogorodnikov Date: Wed, 28 Jun 2023 16:21:08 +0200 Subject: [PATCH] RUMM-3338: Introduce known file cache and cleanup throttling in BatchFileOrchestrator in order to reduce the number of syscalls --- .../persistence/file/FilePersistenceConfig.kt | 4 +- .../file/batch/BatchFileOrchestrator.kt | 49 +++++++++-- .../file/batch/BatchFileOrchestratorTest.kt | 83 ++++++++++++++++++- detekt_custom.yml | 4 + 4 files changed, 132 insertions(+), 8 deletions(-) diff --git a/dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/persistence/file/FilePersistenceConfig.kt b/dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/persistence/file/FilePersistenceConfig.kt index 501d993570..efab12d305 100644 --- a/dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/persistence/file/FilePersistenceConfig.kt +++ b/dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/persistence/file/FilePersistenceConfig.kt @@ -15,7 +15,8 @@ internal data class FilePersistenceConfig( val maxItemSize: Long = MAX_ITEM_SIZE, val maxItemsPerBatch: Int = MAX_ITEMS_PER_BATCH, val oldFileThreshold: Long = OLD_FILE_THRESHOLD, - val maxDiskSpace: Long = MAX_DISK_SPACE + val maxDiskSpace: Long = MAX_DISK_SPACE, + val cleanupFrequencyThreshold: Long = CLEANUP_FREQUENCY_THRESHOLD_MS ) { companion object { internal const val MAX_BATCH_SIZE: Long = 4L * 1024 * 1024 // 4 MB @@ -24,5 +25,6 @@ internal data class FilePersistenceConfig( internal const val OLD_FILE_THRESHOLD: Long = 18L * 60L * 60L * 1000L // 18 hours internal const val MAX_DISK_SPACE: Long = 128 * MAX_BATCH_SIZE // 512 MB internal const val MAX_DELAY_BETWEEN_MESSAGES_MS = 5000L + internal const val CLEANUP_FREQUENCY_THRESHOLD_MS = 1000L // 1s } } diff --git a/dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt b/dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt index 6d0ac6c11c..a56c0877dd 100644 --- a/dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt +++ b/dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt @@ -7,6 +7,7 @@ package com.datadog.android.core.internal.persistence.file.batch import androidx.annotation.WorkerThread +import androidx.collection.LruCache import com.datadog.android.core.internal.persistence.file.FileOrchestrator import com.datadog.android.core.internal.persistence.file.FilePersistenceConfig import com.datadog.android.core.internal.persistence.file.canWriteSafe @@ -22,6 +23,8 @@ import java.io.FileFilter import java.util.Locale import kotlin.math.roundToLong +// TODO RUMM-3373 Improve this class: need to make it thread-safe and optimize work with file +// system in order to reduce the number of syscalls (which are expensive) for files already seen internal class BatchFileOrchestrator( private val rootDir: File, private val config: FilePersistenceConfig, @@ -42,6 +45,10 @@ internal class BatchFileOrchestrator( private var previousFile: File? = null private var previousFileItemCount: Int = 0 + @Suppress("UnsafeThirdPartyFunctionCall") // argument is not negative + private val knownBatchFiles = LruCache(KNOWN_FILES_MAX_CACHE_SIZE) + private var lastCleanupTimestamp: Long = 0L + // region FileOrchestrator @WorkerThread @@ -50,8 +57,11 @@ internal class BatchFileOrchestrator( return null } - deleteObsoleteFiles() - freeSpaceIfNeeded() + if (canDoCleanup()) { + deleteObsoleteFiles() + freeSpaceIfNeeded() + lastCleanupTimestamp = System.currentTimeMillis() + } return if (!forceNewFile) { getReusableWritableFile() ?: createNewFile() @@ -67,6 +77,7 @@ internal class BatchFileOrchestrator( } deleteObsoleteFiles() + lastCleanupTimestamp = System.currentTimeMillis() val files = listSortedBatchFiles() @@ -186,6 +197,8 @@ internal class BatchFileOrchestrator( val newFile = File(rootDir, newFileName) previousFile = newFile previousFileItemCount = 1 + @Suppress("UnsafeThirdPartyFunctionCall") // value is not null + knownBatchFiles.put(newFile, Unit) return newFile } @@ -231,6 +244,8 @@ internal class BatchFileOrchestrator( .filter { (it.name.toLongOrNull() ?: 0) < threshold } .forEach { it.deleteSafe() + @Suppress("UnsafeThirdPartyFunctionCall") // value is not null + knownBatchFiles.remove(it) if (it.metadata.existsSafe()) { it.metadata.deleteSafe() } @@ -264,6 +279,8 @@ internal class BatchFileOrchestrator( if (!file.existsSafe()) return 0 val size = file.lengthSafe() + @Suppress("UnsafeThirdPartyFunctionCall") // value is not null + knownBatchFiles.remove(file) return if (file.deleteSafe()) { size } else { @@ -278,15 +295,33 @@ internal class BatchFileOrchestrator( private val File.metadata: File get() = File("${this.path}_metadata") + private fun canDoCleanup(): Boolean { + return System.currentTimeMillis() - lastCleanupTimestamp > config.cleanupFrequencyThreshold + } + // endregion // region FileFilter - internal class BatchFileFilter : FileFilter { + internal inner class BatchFileFilter : FileFilter { + @Suppress("ReturnCount") override fun accept(file: File?): Boolean { - return file != null && - file.isFileSafe() && + if (file == null) return false + + @Suppress("UnsafeThirdPartyFunctionCall") // value is not null + if (knownBatchFiles.get(file) != null) { + return true + } + + return if (file.isFileSafe() && file.name.matches(batchFileNameRegex) + ) { + @Suppress("UnsafeThirdPartyFunctionCall") // both values are not null + knownBatchFiles.put(file, Unit) + true + } else { + false + } } } @@ -297,6 +332,10 @@ internal class BatchFileOrchestrator( const val DECREASE_PERCENT = 0.95 const val INCREASE_PERCENT = 1.05 + // File class contains only few simple fields, so retained size usually is way below even 1Kb. + // Holding 400 items at max will be below 400 Kb of retained size. + private const val KNOWN_FILES_MAX_CACHE_SIZE = 400 + private val batchFileNameRegex = Regex("\\d+") internal const val ERROR_ROOT_NOT_WRITABLE = "The provided root dir is not writable: %s" internal const val ERROR_ROOT_NOT_DIR = "The provided root file is not a directory: %s" diff --git a/dd-sdk-android/src/test/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestratorTest.kt b/dd-sdk-android/src/test/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestratorTest.kt index 450e58fcfd..4a65f9aab2 100644 --- a/dd-sdk-android/src/test/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestratorTest.kt +++ b/dd-sdk-android/src/test/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestratorTest.kt @@ -47,7 +47,7 @@ import java.util.concurrent.TimeUnit @MockitoSettings(strictness = Strictness.LENIENT) internal class BatchFileOrchestratorTest { - lateinit var testedOrchestrator: FileOrchestrator + private lateinit var testedOrchestrator: FileOrchestrator @TempDir lateinit var tempDir: File @@ -202,6 +202,81 @@ internal class BatchFileOrchestratorTest { assertThat(youngFile).exists() } + @ParameterizedTest + @ValueSource(booleans = [true, false]) + fun `𝕄 respect time threshold to delete obsolete files 𝕎 getWritableFile() { below threshold }`( + forceNewFile: Boolean, + @LongForgery(min = OLD_FILE_THRESHOLD, max = Int.MAX_VALUE.toLong()) oldFileAge: Long + ) { + // Given + assumeTrue(fakeRootDir.listFiles().isNullOrEmpty()) + val oldTimestamp = System.currentTimeMillis() - oldFileAge + val oldFile = File(fakeRootDir, oldTimestamp.toString()) + oldFile.createNewFile() + val oldFileMeta = File("${oldFile.path}_metadata") + oldFileMeta.createNewFile() + val youngTimestamp = System.currentTimeMillis() - RECENT_DELAY_MS - 1 + val youngFile = File(fakeRootDir, youngTimestamp.toString()) + youngFile.createNewFile() + + // When + val start = System.currentTimeMillis() + val result = testedOrchestrator.getWritableFile(forceNewFile) + val end = System.currentTimeMillis() + // let's add very old file after the previous cleanup call. If threshold is respected, + // cleanup shouldn't be performed during the next getWritableFile call + val evenOlderFile = File(fakeRootDir, (oldTimestamp - 1).toString()) + evenOlderFile.createNewFile() + testedOrchestrator.getWritableFile(forceNewFile) + + // Then + checkNotNull(result) + assertThat(result) + .doesNotExist() + .hasParent(fakeRootDir) + assertThat(result.name.toLong()) + .isBetween(start, end) + assertThat(oldFile).doesNotExist() + assertThat(oldFileMeta).doesNotExist() + assertThat(youngFile).exists() + assertThat(evenOlderFile).exists() + } + + @ParameterizedTest + @ValueSource(booleans = [true, false]) + fun `𝕄 respect time threshold to delete obsolete files 𝕎 getWritableFile() { above threshold }`( + forceNewFile: Boolean, + @LongForgery(min = OLD_FILE_THRESHOLD, max = Int.MAX_VALUE.toLong()) oldFileAge: Long + ) { + // Given + assumeTrue(fakeRootDir.listFiles().isNullOrEmpty()) + val oldTimestamp = System.currentTimeMillis() - oldFileAge + val oldFile = File(fakeRootDir, oldTimestamp.toString()) + oldFile.createNewFile() + val oldFileMeta = File("${oldFile.path}_metadata") + oldFileMeta.createNewFile() + + // When + val start = System.currentTimeMillis() + val result = testedOrchestrator.getWritableFile(forceNewFile) + val end = System.currentTimeMillis() + Thread.sleep(CLEANUP_FREQUENCY_THRESHOLD_MS + 1) + val evenOlderFile = File(fakeRootDir, (oldTimestamp - 1).toString()) + evenOlderFile.createNewFile() + testedOrchestrator.getWritableFile(forceNewFile) + + // Then + checkNotNull(result) + assertThat(result) + .doesNotExist() + .hasParent(fakeRootDir) + assertThat(result.name.toLong()) + .isBetween(start, end) + assertThat(oldFile).doesNotExist() + assertThat(oldFileMeta).doesNotExist() + assertThat(evenOlderFile).doesNotExist() + } + @ParameterizedTest @ValueSource(booleans = [true, false]) fun `𝕄 return new File 𝕎 getWritableFile() {no available file}`(forceNewFile: Boolean) { @@ -413,6 +488,7 @@ internal class BatchFileOrchestratorTest { } // When + Thread.sleep(CLEANUP_FREQUENCY_THRESHOLD_MS + 1) val start = System.currentTimeMillis() val result = testedOrchestrator.getWritableFile(forceNewFile) val end = System.currentTimeMillis() @@ -1021,13 +1097,16 @@ internal class BatchFileOrchestratorTest { private const val OLD_FILE_THRESHOLD: Long = RECENT_DELAY_MS * 4 private const val MAX_DISK_SPACE = MAX_BATCH_SIZE * 4 + private const val CLEANUP_FREQUENCY_THRESHOLD_MS = 50L + private val TEST_PERSISTENCE_CONFIG = FilePersistenceConfig( RECENT_DELAY_MS, MAX_BATCH_SIZE.toLong(), MAX_ITEM_SIZE.toLong(), MAX_ITEM_PER_BATCH, OLD_FILE_THRESHOLD, - MAX_DISK_SPACE.toLong() + MAX_DISK_SPACE.toLong(), + CLEANUP_FREQUENCY_THRESHOLD_MS ) } } diff --git a/detekt_custom.yml b/detekt_custom.yml index 5a0af03f8d..3b73b68eb0 100644 --- a/detekt_custom.yml +++ b/detekt_custom.yml @@ -104,6 +104,10 @@ datadog: - "androidx.work.WorkManager.enqueueUniqueWork(kotlin.String, androidx.work.ExistingWorkPolicy, androidx.work.OneTimeWorkRequest):java.util.concurrent.RejectedExecutionException,java.lang.NullPointerException" - "android.content.res.Resources.getResourceName(kotlin.Int):android.content.res.Resources.NotFoundException" - "android.content.res.Resources.openRawResource(kotlin.Int):android.content.res.Resources.NotFoundException" + - "androidx.collection.LruCache.constructor(kotlin.Int):java.lang.IllegalArgumentException" + - "androidx.collection.LruCache.get(java.io.File):java.lang.NullPointerException" + - "androidx.collection.LruCache.put(java.io.File, kotlin.Unit):java.lang.NullPointerException" + - "androidx.collection.LruCache.remove(java.io.File):java.lang.NullPointerException" - "androidx.metrics.performance.JankStats.createAndTrack(android.view.Window, androidx.metrics.performance.JankStats.OnFrameListener):java.lang.IllegalStateException" # endregion # region Java File