From 0f0c33f1a444b2a43279ee6811ada5b5d0a70eaa Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Thu, 5 Dec 2024 15:08:26 +0100 Subject: [PATCH] deduplicate reading file content --- .../meta/internal/metals/Compilations.scala | 15 +++-- .../internal/metals/MetalsLspService.scala | 35 ++++++---- .../metals/ProjectMetalsLspService.scala | 4 +- .../internal/metals/SavedFileSignatures.scala | 64 ++++++++++++++----- .../scala/tests/sbt/CancelCompileSuite.scala | 5 +- .../test/scala/tests/sbt/SbtServerSuite.scala | 3 +- .../src/main/scala/tests/TestingServer.scala | 9 ++- .../src/test/scala/tests/BillLspSuite.scala | 5 +- .../scala/tests/CancelCompileLspSuite.scala | 5 +- .../tests/UnsupportedDebuggingLspSuite.scala | 5 +- 10 files changed, 105 insertions(+), 45 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala index 257af451993..7d0969a7a30 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Compilations.scala @@ -108,28 +108,30 @@ final class Compilations( compileBatch(targets).ignoreValue } - def compileFile(path: AbsolutePath): Future[Option[b.CompileResult]] = { - if(fileSignatures.didSavedContentChanged(path)) { + def compileFile(path: PathWithContent): Future[Option[b.CompileResult]] = { + if (fileSignatures.didSavedContentChanged(path)) { def empty = new b.CompileResult(b.StatusCode.CANCELLED) for { - targetOpt <- expand(path) + targetOpt <- expand(path.path) result <- targetOpt match { case None => Future.successful(empty) case Some(target) => compileBatch(target) .map(res => res.getOrElse(target, empty)) } - _ <- compileWorksheets(Seq(path)) + _ <- compileWorksheets(Seq(path.path)) } yield Some(result) } else Future.successful(None) } def compileFiles( - paths: Seq[AbsolutePath], + pathsWithContent: Seq[PathWithContent], focusedDocumentBuildTarget: Option[BuildTargetIdentifier], ): Future[Unit] = { + val paths = + pathsWithContent.filter(fileSignatures.didSavedContentChanged).map(_.path) for { - targets <- expand(paths.filter(fileSignatures.didSavedContentChanged)) + targets <- expand(paths) _ <- compileBatch(targets) _ <- focusedDocumentBuildTarget match { case Some(bt) @@ -160,6 +162,7 @@ final class Compilations( lastCompile = Set.empty cascadeBatch.cancelAll() compileBatch.cancelAll() + fileSignatures.cancel() } def recompileAll(): Future[Unit] = { diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index 2c2502fdabe..5545c3aaeb7 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -750,8 +750,9 @@ abstract class MetalsLspService( .getOrElse(current) } + val content = FileIO.readAllBytes(path) // Update md5 fingerprint from file contents on disk - fingerprints.add(path, FileIO.slurp(path, charset)) + fingerprints.add(path, new String(content, charset)) // Update in-memory buffer contents from LSP client buffers.put(path, params.getTextDocument.getText) @@ -784,7 +785,10 @@ abstract class MetalsLspService( Future .sequence( List( - maybeCompileOnDidFocus(path, prevBuildTarget), + maybeCompileOnDidFocus( + PathWithContent(path, content), + prevBuildTarget, + ), compilers.load(List(path)), parser, interactive, @@ -821,20 +825,20 @@ abstract class MetalsLspService( CompletableFuture.completedFuture(DidFocusResult.RecentlyActive) } else { worksheetProvider.onDidFocus(path) - maybeCompileOnDidFocus(path, prevBuildTarget).asJava + maybeCompileOnDidFocus(PathWithContent(path), prevBuildTarget).asJava } } protected def maybeCompileOnDidFocus( - path: AbsolutePath, + path: PathWithContent, prevBuildTarget: b.BuildTargetIdentifier, ): Future[DidFocusResult.Value] = - buildTargets.inverseSources(path) match { + buildTargets.inverseSources(path.path) match { case Some(target) if prevBuildTarget != target => compilations .compileFile(path) .map(_ => DidFocusResult.Compiled) - case _ if path.isWorksheet => + case _ if path.path.isWorksheet => compilations .compileFile(path) .map(_ => DidFocusResult.Compiled) @@ -938,16 +942,22 @@ abstract class MetalsLspService( } protected def onChange(paths: Seq[AbsolutePath]): Future[Unit] = { - paths.foreach { path => - fingerprints.add(path, FileIO.slurp(path, charset)) - } + val pathsWithContent = + paths.map { path => + val content = FileIO.readAllBytes(path) + fingerprints.add(path, new String(content, charset)) + PathWithContent(path, content) + } Future .sequence( List( Future(indexer.reindexWorkspaceSources(paths)), compilations - .compileFiles(paths, Option(focusedDocumentBuildTarget.get())), + .compileFiles( + pathsWithContent, + Option(focusedDocumentBuildTarget.get()), + ), ) ++ paths.map(f => Future(interactiveSemanticdbs.textDocument(f))) ) .ignoreValue @@ -958,7 +968,10 @@ abstract class MetalsLspService( .sequence( List( compilations - .compileFiles(List(path), Option(focusedDocumentBuildTarget.get())), + .compileFiles( + List(PathWithContent.deleted(path)), + Option(focusedDocumentBuildTarget.get()), + ), Future { diagnostics.didDelete(path) testProvider.onFileDelete(path) diff --git a/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala index 79c005ce21f..55e8145c39f 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala @@ -635,7 +635,9 @@ class ProjectMetalsLspService( for { _ <- connect(ImportBuildAndIndex(session)) } { - focusedDocument().foreach(path => compilations.compileFile(path)) + focusedDocument().foreach(path => + compilations.compileFile(PathWithContent(path)) + ) } } } diff --git a/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala b/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala index 556f6f6f4b2..453915362bc 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/SavedFileSignatures.scala @@ -12,25 +12,55 @@ import scala.meta.io.AbsolutePath class SavedFileSignatures { private val previousCreateOrModify = TrieMap[AbsolutePath, String]() - def didSavedContentChanged(path: AbsolutePath): Boolean = { - try { - val md5 = - if (path.exists) Some(MD5.bytesToHex(FileIO.readAllBytes(path))) - else None - synchronized { - if (previousCreateOrModify.get(path) == md5) false - else { - md5 match { - case None => previousCreateOrModify.remove(path) - case Some(md5) => previousCreateOrModify.put(path, md5) + def didSavedContentChanged(pathWithContent: PathWithContent): Boolean = { + val path = pathWithContent.path + pathWithContent + .getSignature() + .map { md5 => + synchronized { + if (previousCreateOrModify.get(path) == md5) false + else { + md5 match { + case None => previousCreateOrModify.remove(path) + case Some(md5) => previousCreateOrModify.put(path, md5) + } + true } - true } } - } catch { - case e: IOException => - scribe.warn(s"Failed to read contents of $path", e) - true - } + .getOrElse(true) } + + def cancel(): Unit = previousCreateOrModify.clear() +} + +class PathWithContent( + val path: AbsolutePath, + optContent: Option[PathWithContent.Content], +) { + def getSignature(): Either[IOException, Option[String]] = { + optContent + .map(_.map(content => MD5.bytesToHex(content))) + .map(Right[IOException, Option[String]](_)) + .getOrElse { + try { + if (path.exists) + Right(Some(MD5.bytesToHex(FileIO.readAllBytes(path)))) + else Right(None) + } catch { + case e: IOException => + scribe.warn(s"Failed to read contents of $path", e) + Left(e) + } + } + } +} + +object PathWithContent { + // None if the file doesn't exist + type Content = Option[Array[Byte]] + def apply(path: AbsolutePath) = new PathWithContent(path, None) + def apply(path: AbsolutePath, content: Array[Byte]) = + new PathWithContent(path, Some(Some(content))) + def deleted(path: AbsolutePath) = new PathWithContent(path, Some(None)) } diff --git a/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala b/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala index f7370bb2cea..cbd08ea62b5 100644 --- a/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala +++ b/tests/slow/src/test/scala/tests/sbt/CancelCompileSuite.scala @@ -1,6 +1,7 @@ package tests import scala.meta.internal.metals.MetalsServerConfig +import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.ServerCommands import scala.meta.internal.metals.{BuildInfo => V} @@ -58,7 +59,7 @@ class CancelCompileSuite _ <- server.server.buildServerPromise.future (compileReport, _) <- server.server.compilations .compileFile( - workspace.resolve("c/src/main/scala/c/C.scala") + PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) ) .zip { // wait until the compilation start @@ -68,7 +69,7 @@ class CancelCompileSuite _ = assertNoDiff(client.workspaceDiagnostics, "") _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.CANCELLED) _ <- server.server.compilations.compileFile( - workspace.resolve("c/src/main/scala/c/C.scala") + PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) ) _ = assertNoDiff( client.workspaceDiagnostics, diff --git a/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala b/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala index 3cb60b7492c..508670c6ff3 100644 --- a/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala +++ b/tests/slow/src/test/scala/tests/sbt/SbtServerSuite.scala @@ -9,6 +9,7 @@ import scala.meta.internal.metals.CreateSession import scala.meta.internal.metals.Messages import scala.meta.internal.metals.Messages.ImportBuildChanges import scala.meta.internal.metals.MetalsEnrichments._ +import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.ServerCommands import scala.meta.internal.metals.{BuildInfo => V} import scala.meta.internal.process.SystemProcess @@ -409,7 +410,7 @@ class SbtServerSuite _ <- initialize(layout) // make sure to compile once _ <- server.server.compilations.compileFile( - workspace.resolve("target/Foo.scala") + PathWithContent(workspace.resolve("target/Foo.scala")) ) } yield { // Sleep 100 ms: that should be enough to see the compilation looping diff --git a/tests/unit/src/main/scala/tests/TestingServer.scala b/tests/unit/src/main/scala/tests/TestingServer.scala index 98042207732..db10b784a24 100644 --- a/tests/unit/src/main/scala/tests/TestingServer.scala +++ b/tests/unit/src/main/scala/tests/TestingServer.scala @@ -1057,7 +1057,10 @@ final case class TestingServer( val compilations = paths.map(path => - fullServer.getServiceFor(path).compilations.compileFile(path) + fullServer + .getServiceFor(path) + .compilations + .compileFile(m.internal.metals.PathWithContent(path)) ) for { @@ -1119,7 +1122,9 @@ final case class TestingServer( codeLenses.trySuccess(lenses.toList) else if (retries > 0) { retries -= 1 - server.compilations.compileFile(path) + server.compilations.compileFile( + m.internal.metals.PathWithContent(path) + ) } else { val error = s"Could not fetch any code lenses in $maxRetries tries" codeLenses.tryFailure(new NoSuchElementException(error)) diff --git a/tests/unit/src/test/scala/tests/BillLspSuite.scala b/tests/unit/src/test/scala/tests/BillLspSuite.scala index a5907133764..45a5ec7fd68 100644 --- a/tests/unit/src/test/scala/tests/BillLspSuite.scala +++ b/tests/unit/src/test/scala/tests/BillLspSuite.scala @@ -5,6 +5,7 @@ import scala.concurrent.Future import scala.meta.internal.metals.Directories import scala.meta.internal.metals.Messages import scala.meta.internal.metals.MetalsEnrichments._ +import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.RecursivelyDelete import scala.meta.internal.metals.ServerCommands import scala.meta.io.AbsolutePath @@ -241,7 +242,7 @@ class BillLspSuite extends BaseLspSuite("bill") { ) (compileReport, _) <- server.server.compilations .compileFile( - workspace.resolve("src/com/App.scala") + PathWithContent(workspace.resolve("src/com/App.scala")) ) .zip { // wait until the compilation start @@ -260,7 +261,7 @@ class BillLspSuite extends BaseLspSuite("bill") { _ = assert(currentTrace.contains(s"buildTarget/compile - ($cancelId)")) compileReport <- server.server.compilations .compileFile( - workspace.resolve("src/com/App.scala") + PathWithContent(workspace.resolve("src/com/App.scala")) ) _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.OK) } yield () diff --git a/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala b/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala index 52992251c3b..4f9e0e54d3f 100644 --- a/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala +++ b/tests/unit/src/test/scala/tests/CancelCompileLspSuite.scala @@ -1,5 +1,6 @@ package tests +import scala.meta.internal.metals.PathWithContent import scala.meta.internal.metals.ServerCommands import ch.epfl.scala.bsp4j.StatusCode @@ -48,7 +49,7 @@ class CancelCompileLspSuite extends BaseLspSuite("compile-cancel") { _ <- server.server.buildServerPromise.future (compileReport, _) <- server.server.compilations .compileFile( - workspace.resolve("c/src/main/scala/c/C.scala") + PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) ) .zip { // wait until the compilation start @@ -58,7 +59,7 @@ class CancelCompileLspSuite extends BaseLspSuite("compile-cancel") { _ = assertNoDiff(client.workspaceDiagnostics, "") _ = assertEquals(compileReport.get.getStatusCode(), StatusCode.CANCELLED) _ <- server.server.compilations.compileFile( - workspace.resolve("c/src/main/scala/c/C.scala") + PathWithContent(workspace.resolve("c/src/main/scala/c/C.scala")) ) _ = assertNoDiff( client.workspaceDiagnostics, diff --git a/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala b/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala index 144255114fe..c58e7a65a1c 100644 --- a/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala +++ b/tests/unit/src/test/scala/tests/UnsupportedDebuggingLspSuite.scala @@ -9,6 +9,7 @@ import scala.util.Success import scala.meta.internal.metals.ClientCommands import scala.meta.internal.metals.InitializationOptions import scala.meta.internal.metals.MetalsEnrichments._ +import scala.meta.internal.metals.PathWithContent class UnsupportedDebuggingLspSuite extends BaseLspSuite("unsupported-debugging") { @@ -62,7 +63,9 @@ class UnsupportedDebuggingLspSuite ) _ <- server.server.compilations - .compileFile(server.toPath("a/src/main/scala/Main.scala")) + .compileFile( + PathWithContent(server.toPath("a/src/main/scala/Main.scala")) + ) } yield { val clientCommands = client.clientCommands.asScala.map(_.getCommand).toSet assert(!clientCommands.contains(ClientCommands.RefreshModel.id))