Merge pull request #2504 from digma-ai/improve-navigation-change-service

shalom938 · web-flow · commit 0c8cf3535ae6 · 2024-09-11T23:58:18.000+03:00
Improve navigation change service
diff --git a/jvm-common/src/main/kotlin/org/digma/intellij/plugin/idea/navigation/NavigationDiscoveryChangeService.kt b/jvm-common/src/main/kotlin/org/digma/intellij/plugin/idea/navigation/NavigationDiscoveryChangeService.kt
@@ -1,6 +1,7 @@
 package org.digma.intellij.plugin.idea.navigation
 
 import com.intellij.openapi.Disposable
+import com.intellij.openapi.application.readAction
 import com.intellij.openapi.components.Service
 import com.intellij.openapi.diagnostic.Logger
 import com.intellij.openapi.project.Project
@@ -23,9 +24,8 @@ import kotlinx.coroutines.isActive
 import kotlinx.coroutines.launch
 import kotlinx.datetime.Clock
 import kotlinx.datetime.Instant
+import org.apache.commons.codec.digest.DigestUtils
 import org.digma.intellij.plugin.common.isProjectValid
-import org.digma.intellij.plugin.common.isValidVirtualFile
-import org.digma.intellij.plugin.common.runInReadAccessWithResult
 import org.digma.intellij.plugin.errorreporting.ErrorReporter
 import org.digma.intellij.plugin.idea.psi.isJvmSupportedFile
 import org.digma.intellij.plugin.log.Log
@@ -56,7 +56,7 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
 
     private val logger = Logger.getInstance(this::class.java)
 
-    private val quitePeriod = 5.seconds
+    private val quitePeriod = 30.seconds
 
     //using LinkedHashSet to keep insertion order so we process files in the order they were changed.
     //there is only write operation to this set, never read or remove. before processing, the set is replaced with a new instance
@@ -77,9 +77,9 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
     private var lastChangedFileEventTime: Instant? = null
     private var lastBulkFileChangeEventTime: Instant? = null
 
-    private val changeFilesProcessingJob: Job = launchBulkProcessingLoop(changedFilesBulks, ChangedFileProcessor())
-    private val bulkEventsProcessingJob: Job = launchBulkProcessingLoop(bulkEventsBulks, FileEventsProcessor())
-    private val quitePeriodManagerJob: Job = launchQuitePeriodManager()
+    private var changeFilesProcessingJob: Job? = null
+    private var bulkEventsProcessingJob: Job? = null
+    private var quitePeriodManagerJob: Job? = null
     private var launchFullUpdateJob: Job? = null
 
     private val paused = AtomicBoolean(false)
@@ -88,6 +88,11 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
 
 
     companion object {
+
+        fun isEnabled(): Boolean {
+            return java.lang.Boolean.parseBoolean(System.getProperty("org.digma.intellij.plugin.enableNavigationDiscoveryChangedFilesUpdate", "true"))
+        }
+
         fun createChangedFilesSet(): MutableSet<String> {
             return LinkedHashSet()
         }
@@ -99,15 +104,24 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
 
 
     init {
-        PsiManager.getInstance(project).addPsiTreeChangeListener(NavigationDiscoveryPsiTreeChangeListener(), this)
-        VirtualFileManager.getInstance().addAsyncFileListener(NavigationDiscoveryAsyncFileChangeListener(), this)
+        if (isEnabled()) {
+
+            changeFilesProcessingJob = launchBulkProcessingLoop(changedFilesBulks, ChangedFileProcessor())
+            bulkEventsProcessingJob = launchBulkProcessingLoop(bulkEventsBulks, FileEventsProcessor())
+            quitePeriodManagerJob = launchQuitePeriodManager()
+
+            PsiManager.getInstance(project).addPsiTreeChangeListener(NavigationDiscoveryPsiTreeChangeListener(), this)
+            VirtualFileManager.getInstance().addAsyncFileListener(NavigationDiscoveryAsyncFileChangeListener(), this)
+        } else {
+            Log.log(logger::info, project, "I am disabled, not starting listeners")
+        }
     }
 
 
     override fun dispose() {
-        changeFilesProcessingJob.cancel()
-        bulkEventsProcessingJob.cancel()
-        quitePeriodManagerJob.cancel()
+        changeFilesProcessingJob?.cancel()
+        bulkEventsProcessingJob?.cancel()
+        quitePeriodManagerJob?.cancel()
     }
 
 
@@ -173,6 +187,7 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
 
 
     private fun <T> launchBulkProcessingLoop(queue: Queue<Iterable<T>>, itemProcessor: ItemProcessor<T>): Job {
+
         return cs.launch {
             Log.log(logger::trace, project, "Starting bulk processing loop for {}", itemProcessor::class.java)
             while (isActive && isProjectValid(project)) {
@@ -183,9 +198,9 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
                     if (bulk == null) {
                         delay(5.seconds)
                     } else {
+                        Log.log(logger::trace, project, "processing bulk of {}", itemProcessor::class.java)
                         bulk.forEach {
-                            if (!isActive) {
-                                // Cancelled, stop processing
+                            if (!isActive || isPaused()) {
                                 return@forEach
                             }
                             itemProcessor.process(it)
@@ -230,6 +245,8 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
             return
         }
 
+        pause()
+
         /*
         when the currently running coroutine completes another one can start.
         if another one wants to start while calling the buildNavigationDiscoveryFullUpdate it will be
@@ -239,8 +256,7 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
         but as said there is a very small chance for that to happen.
          */
 
-
-        //make sure not to launch twice because events keep coming until pause is called
+        //another safety lock to make sure not to launch twice because events keep coming.
         launchFullUpdateLock.withLock {
 
             if (launchFullUpdateJob?.isActive == true) {
@@ -257,7 +273,7 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
 
                     //pause and clear all collected events so no more update tasks will run until resume
                     Log.log(logger::trace, project, "pausing and clearing all collected events in launchFullUpdateJob")
-                    pause()
+
                     //replace the lists , never call clear or remove to avoid concurrent modifications
                     changedFiles = createChangedFilesSet()
                     bulkEvents = createBulkEventsList()
@@ -269,7 +285,7 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
                     //during this time the update tasks that already started will probably finish
                     Log.log(logger::trace, project, "waiting for quite period in launchFullUpdateJob")
                     var quitePeriod = ZERO
-                    while (isActive && quitePeriod < 10.seconds) {
+                    while (isActive && quitePeriod < 30.seconds) {
 
                         Log.log(logger::trace, project, "checking quite period in launchFullUpdateJob, current quite period {}", quitePeriod)
 
@@ -312,6 +328,8 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
                 Log.log(logger::trace, project, "quite period completed in launchFullUpdateJob, launching full update")
                 //what ever happens resume must be called before this coroutine completes
                 try {
+                    //resume before launching full update to make sure we don't miss updates
+                    resume()
                     //if the coroutine was canceled don't call the update, probably the project was closed
                     if (isActive) {
                         Log.log(logger::info, project, "launching full update span discovery in launchFullUpdateJob")
@@ -343,20 +361,29 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
             return
         }
 
-        //protect against high memory consumption, if so many files are changed at once pause collecting changed files and launch full update
-        //200 is just a number that seems reasonable to do a full update instead of updating separate files
-        if (changedFiles.size > 200) {
+        //protect against high memory consumption, if so many files are changed at once, pause collecting changed files and launch full update.
+        //100 is just a number that seems reasonable to do a full update instead of updating separate files.
+        if (changedFiles.size > 100) {
 
             Log.log(logger::trace, project, "discovered too many changed files {}", changedFiles.size)
 
-            pauseAndLaunchFullUpdate()
+            launchFullUpdateLock.withLock {
+
+                if (isPaused()) {
+                    return@withLock
+                }
+
+                pauseAndLaunchFullUpdate()
 
-            ActivityMonitor.getInstance(project).registerCustomEvent(
-                "LargeBulkUpdate", mapOf(
-                    "eventName" to "changeFiles",
-                    "changedFiles.size" to changedFiles.size
+                ActivityMonitor.getInstance(project).registerCustomEvent(
+                    "LargeBulkUpdate", mapOf(
+                        "eventName" to "changeFiles",
+                        "changedFiles.size" to changedFiles.size,
+                        "project.hash" to DigestUtils.md2Hex(project.name)
+                    )
                 )
-            )
+            }
+
             return
         }
         Log.log(logger::trace, project, "adding changed file {}", virtualFile.url)
@@ -397,20 +424,30 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
             return
         }
 
-        //protect against high memory consumption, if so many files are changed at once pause collecting events and launch a full update
-        //200 is just a number that seems reasonable to do a full update instead of updating separate files
-        if (bulkEvents.size > 200) {
+        //protect against high memory consumption, if so many files are changed at once, pause collecting events and launch a full update.
+        //100 is just a number that seems reasonable to do a full update instead of updating separate files.
+        if (bulkEvents.size > 100 || events.size > 100) {
 
             Log.log(logger::trace, project, "discovered too many bulk change events {}", bulkEvents.size)
 
-            pauseAndLaunchFullUpdate()
+            launchFullUpdateLock.withLock {
+
+                if (isPaused()) {
+                    return@withLock
+                }
 
-            ActivityMonitor.getInstance(project).registerCustomEvent(
-                "LargeBulkUpdate", mapOf(
-                    "eventName" to "bulkEvents",
-                    "bulkEvents.size" to bulkEvents.size
+                pauseAndLaunchFullUpdate()
+
+                val size = if (bulkEvents.size > 100) bulkEvents.size else events.size
+                ActivityMonitor.getInstance(project).registerCustomEvent(
+                    "LargeBulkUpdate", mapOf(
+                        "eventName" to "bulkEvents",
+                        "bulkEvents.size" to size,
+                        "project.hash" to DigestUtils.md2Hex(project.name)
+                    )
                 )
-            )
+            }
+
             return
         }
         bulkEvents.addAll(events)
@@ -449,25 +486,21 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
 
 
     private interface ItemProcessor<T> {
-        fun process(item: T)
+        suspend fun process(item: T)
     }
 
 
     private inner class FileEventsProcessor : ItemProcessor<VFileEvent> {
-        override fun process(item: VFileEvent) {
+        override suspend fun process(item: VFileEvent) {
 
             try {
 
                 if (isRelevantFile(project, item.file)) {
 
                     //run a quick check if the event is for a java or kotlin file and abort processing if not.
-                    //if we can't find PsiFile continue processing, maybe it's a copy or delete or other event that we want to process
                     item.file?.let { vf ->
-                        val psiFile = findPsiFileForVirtualFile(vf)
-                        psiFile?.let { psf ->
-                            if (!isJavaOrKotlinFile(psf)) {
-                                return
-                            }
+                        if (!isJavaOrKotlinFile(vf)) {
+                            return
                         }
                     }
 
@@ -524,7 +557,7 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
 
 
     private inner class ChangedFileProcessor : ItemProcessor<String> {
-        override fun process(item: String) {
+        override suspend fun process(item: String) {
 
             try {
 
@@ -542,9 +575,10 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
     }
 
 
-    private fun updateNavigation(project: Project, file: VirtualFile) {
+    private suspend fun updateNavigation(project: Project, file: VirtualFile) {
         if (isValidRelevantFile(project, file)) {
-            val psiFile = runInReadAccessWithResult {
+
+            val psiFile = readAction {
                 PsiManager.getInstance(project).findFile(file)
             }
             psiFile?.let {
@@ -583,18 +617,6 @@ class NavigationDiscoveryChangeService(private val project: Project, private val
     }
 
 
-    private fun findPsiFileForVirtualFile(virtualFile: VirtualFile): PsiFile? {
-        return try {
-            runInReadAccessWithResult {
-                virtualFile.takeIf { isValidVirtualFile(virtualFile) }?.let {
-                    PsiManager.getInstance(project).findFile(it)
-                }
-            }
-        } catch (e: Throwable) {
-            null
-        }
-    }
-
     private fun isJavaOrKotlinFile(psiFile: PsiFile): Boolean {
         return psiFile.language.displayName.equals("Java", true) ||
                 psiFile.language.displayName.equals("Kotlin", true)
diff --git a/jvm-common/src/main/kotlin/org/digma/intellij/plugin/idea/navigation/navigationUtils.kt b/jvm-common/src/main/kotlin/org/digma/intellij/plugin/idea/navigation/navigationUtils.kt
@@ -1,18 +1,17 @@
 package org.digma.intellij.plugin.idea.navigation
 
+import com.intellij.openapi.application.readAction
 import com.intellij.openapi.project.Project
 import com.intellij.openapi.roots.ProjectFileIndex
 import com.intellij.openapi.util.Computable
 import com.intellij.openapi.vfs.VirtualFile
-import org.digma.intellij.plugin.common.ReadActions
 import org.digma.intellij.plugin.common.executeCatchingIgnorePCE
 import org.digma.intellij.plugin.common.executeCatchingWithResultIgnorePCE
 import org.digma.intellij.plugin.common.isProjectValid
 import org.digma.intellij.plugin.common.isValidVirtualFile
 import org.digma.intellij.plugin.common.runWIthRetryIgnorePCE
 import org.digma.intellij.plugin.common.runWIthRetryWithResultIgnorePCE
 import org.digma.intellij.plugin.idea.navigation.model.NavigationProcessContext
-import java.util.function.Supplier
 
 
 fun executeCatchingWithRetry(
@@ -53,7 +52,7 @@ fun isRelevantFile(project: Project, file: VirtualFile?): Boolean {
 
 //this method checks isInContent which needs read access. don't call it from EDT events like
 // onChange in psi tree listeners because it will cause short freezes, only call it on background processing
-fun isValidRelevantFile(project: Project, file: VirtualFile?): Boolean {
+suspend fun isValidRelevantFile(project: Project, file: VirtualFile?): Boolean {
     return isRelevantFile(project, file) &&
             isValidVirtualFile(file) &&
             file != null &&
@@ -63,8 +62,8 @@ fun isValidRelevantFile(project: Project, file: VirtualFile?): Boolean {
 
 //don't call this method on EDT events like onChange in psi tree listeners because it will
 // cause short freezes, only call it on background processing
-fun isInContent(project: Project, file: VirtualFile): Boolean {
-    return ReadActions.ensureReadAction(Supplier {
+suspend fun isInContent(project: Project, file: VirtualFile): Boolean {
+    return readAction {
         ProjectFileIndex.getInstance(project).isInContent(file)
-    })
+    }
 }
