Skip to content

Commit

Permalink
improvement: Base Metals view on indexing information
Browse files Browse the repository at this point in the history
Previously, we would read the classpath to create the metals view, which worked well for Scala 2, but needed a reimplementation for Scala 3. So now, instead of doing this reimplementation, which would probably need to use tasty and might be quite complex, I decided to base it on the same approach we use for inexing.

This makes the feature more reliable since, we now used the same well tested functionaties as things like go to definition.

Unfortunately, we might index the particular files the second time, because the basic indexing caches are reversed and finding all symbols in a file from those maps was much slower than just doing it the second time. We later cache that information the same way we did in the previous tree view implementation. So we won't use more memory and running the benchmarks show that the new approach is similarily fast:

```
[info] Benchmark                  Mode  Cnt    Score    Error  Units
[info] ClasspathSymbolsBench.run    ss   10  542.090 ± 61.953  ms/op
```

as compared to the previous:
```
[info] Benchmark                  Mode  Cnt    Score    Error  Units
[info] ClasspathSymbolsBench.run    ss   10  477.283 ± 93.933  ms/op
```

Also it seems we index much more information, since mtags index all symbols, which would also explain the slight difference in timings as I think the difference was 1300 symbols previously and 17000 after this change (number of symbols detected in the benchmarks)

Fixes scalameta#2859
  • Loading branch information
tgodzik committed Sep 6, 2023
1 parent ccd5153 commit f9246de
Show file tree
Hide file tree
Showing 27 changed files with 548 additions and 374 deletions.
18 changes: 14 additions & 4 deletions metals-bench/src/main/scala/bench/ClasspathSymbolsBench.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ package bench

import java.util.concurrent.TimeUnit

import scala.meta.internal.tvp.ClasspathSymbols
import scala.meta.dialects
import scala.meta.internal.metals.EmptyReportContext
import scala.meta.internal.metals.MetalsEnrichments._
import scala.meta.internal.mtags.OnDemandSymbolIndex
import scala.meta.internal.tvp.IndexedSymbols
import scala.meta.io.AbsolutePath

import org.openjdk.jmh.annotations.Benchmark
Expand All @@ -21,7 +25,7 @@ class ClasspathSymbolsBench {

@Setup
def setup(): Unit = {
classpath = Library.cats
classpath = Library.catsSources.filter(_.filename.contains("sources"))
}

@TearDown
Expand All @@ -31,8 +35,14 @@ class ClasspathSymbolsBench {
@BenchmarkMode(Array(Mode.SingleShotTime))
@OutputTimeUnit(TimeUnit.MILLISECONDS)
def run(): Unit = {
val jars = new ClasspathSymbols()
classpath.foreach { jar => jars.symbols(jar, "cats/") }
implicit val reporting = EmptyReportContext
val jars = new IndexedSymbols(
OnDemandSymbolIndex.empty(),
isStatisticsEnabled = false,
)
classpath.foreach { jar =>
jars.jarSymbols(jar, "cats/", dialects.Scala213)
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,7 @@ final case class Indexer(
source,
info.symbol,
dialect,
info,
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,9 @@ object JdkVersion {
}

def fromReleaseFile(javaHome: AbsolutePath): Option[JdkVersion] =
fromReleaseFileString(javaHome).flatMap(f => parse(f))

def fromReleaseFileString(javaHome: AbsolutePath): Option[String] =
Seq(javaHome.resolve("release"), javaHome.parent.resolve("release"))
.filter(_.exists)
.flatMap { releaseFile =>
Expand All @@ -273,7 +276,6 @@ object JdkVersion {
props.asScala
.get("JAVA_VERSION")
.map(_.stripPrefix("\"").stripSuffix("\""))
.flatMap(jv => JdkVersion.parse(jv))
}
.headOption

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -783,9 +783,9 @@ class MetalsLspService(
buildTargets,
() => buildClient.ongoingCompilations(),
definitionIndex,
id => compilations.compileTarget(id),
() => bspSession.map(_.mainConnectionIsBloop).getOrElse(false),
clientConfig.initialConfig.statistics,
optJavaHome,
scalaVersionSelector,
)

private val popupChoiceReset: PopupChoiceReset = new PopupChoiceReset(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class MetalsSymbolSearch(
} else {
dependencySourceCache.getOrElseUpdate(
path,
Mtags.toplevels(input).asJava,
Mtags.topLevelSymbols(input).asJava,
)
}
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,22 +57,21 @@ class ScalaVersionSelector(
)
}

def getDialect(path: AbsolutePath): Dialect = {

def dialectFromBuildTarget = buildTargets
.inverseSources(path)
.flatMap(id => buildTargets.scalaTarget(id))
.map(_.dialect(path))
def dialectFromBuildTarget(path: AbsolutePath): Option[Dialect] = buildTargets
.inverseSources(path)
.flatMap(id => buildTargets.scalaTarget(id))
.map(_.dialect(path))

def getDialect(path: AbsolutePath): Dialect = {
Option(path.extension) match {
case Some("scala") =>
dialectFromBuildTarget.getOrElse(
dialectFromBuildTarget(path).getOrElse(
fallbackDialect(isAmmonite = false)
)
case Some("sbt") => dialects.Sbt
case Some("sc") =>
// worksheets support Scala 3, but ammonite scripts do not
val dialect = dialectFromBuildTarget.getOrElse(
val dialect = dialectFromBuildTarget(path).getOrElse(
fallbackDialect(isAmmonite = path.isAmmoniteScript)
)
dialect
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class StandaloneSymbolSearch(
val input = symDef.path.toInput
dependencySourceCache.getOrElseUpdate(
symDef.path,
mtags.toplevels(input).asJava,
mtags.topLevelSymbols(input).asJava,
)
}
.orElse(workspaceFallback.map(_.definitionSourceToplevels(sym, source)))
Expand Down
231 changes: 0 additions & 231 deletions metals/src/main/scala/scala/meta/internal/tvp/ClasspathSymbols.scala

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ class ClasspathTreeView[Value, Key](
val label =
if (child.kind.isPackage) {
displayName + "/"
} else if (child.kind.isMethod && !child.isVal && !child.isVar) {
} else if (
child.kind.isConstructor || (child.kind.isMethod && !child.isVal && !child.isVar)
) {
displayName + "()"
} else {
displayName
Expand All @@ -87,6 +89,7 @@ class ClasspathTreeView[Value, Key](
// Get the children of this child to determine its collapse state.
val grandChildren =
transitiveChildren.filter(_.symbol.owner == child.symbol)

val collapseState =
if (!childHasSiblings && grandChildren.nonEmpty)
MetalsTreeItemCollapseState.expanded
Expand All @@ -106,13 +109,16 @@ class ClasspathTreeView[Value, Key](
case k.TRAIT => "trait"
case k.CLASS => "class"
case k.INTERFACE => "interface"
case k.CONSTRUCTOR => "method"
case k.METHOD | k.MACRO =>
if (child.properties.isVal) "val"
else if (child.properties.isVar) "var"
else "method"
case k.FIELD =>
if (child.properties.isEnum) "enum"
else "field"
case k.TYPE_PARAMETER => "type"
case k.TYPE => "type"
case _ => null
}

Expand Down
Loading

0 comments on commit f9246de

Please sign in to comment.