RUMM-3338: Introduce known file cache and cleanup throttling in BatchFileOrchestrator in order to reduce the number of syscalls

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
     }
 }

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<File, Unit>(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"

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
         )
     }
 }

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