improvement: Base Metals view on indexing information #5622

tgodzik · 2023-09-06T14:22:13Z

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  391.553 ± 39.018  ms/op

as compared to the previous:

[info] Benchmark                  Mode  Cnt    Score    Error  Units
[info] ClasspathSymbolsBench.run    ss   10  286.283 ± 81.565  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 #2859

tgodzik · 2023-09-13T09:36:21Z

tests/unit/src/test/scala/tests/TreeViewLspSuite.scala

   */
-  val expectedLibraries: SortedSet[String] = {
-    lazy val jdk8Libraries = SortedSet(
-      "charsets", "jce", "jsse", "resources", "rt",


All these sources are contained in src.zip for the JDK

kasiaMarek

Maybe we could have some other entity responsible for calculating toplevelsAt and symbolsAt? It just feels like those don't use or change the state of SymbolIndexBucket, so all other things in that class are noise.

metals/src/main/scala/scala/meta/internal/tvp/IndexedSymbols.scala

kasiaMarek · 2023-09-13T14:37:47Z

metals/src/main/scala/scala/meta/internal/tvp/IndexedSymbols.scala

+  // Used for dependencies, is lazy, TopLevel is changed to AllSymbols when needed
+  private val jarCache = TrieMap.empty[
+    AbsolutePath,
+    TrieMap[String, Either[TopLevel, AllSymbols]],


So the path here is the path to the jar? Maybe it's a bit more readable to have a separate map, where you have (scala file path -> AllSymbols) for jars too. I also will avoid repeating the same set for all top-levels in the same file.

metals/src/main/scala/scala/meta/internal/tvp/IndexedSymbols.scala

kasiaMarek · 2023-09-13T14:59:58Z

metals/src/main/scala/scala/meta/internal/tvp/IndexedSymbols.scala

+  private def symbolsForPackage(
+      cached: Either[TopLevel, AllSymbols],
+      dialect: Dialect,
+      jarSymbols: TrieMap[String, Either[TopLevel, AllSymbols]],


For me passing it and changing it in the method obscures the updates made on jarCache.

Sure, I inlined the method back, wasn't sure how to make the method less long.

tgodzik · 2023-10-25T13:47:24Z

Maybe we could have some other entity responsible for calculating toplevelsAt and symbolsAt? It just feels like those don't use or change the state of SymbolIndexBucket, so all other things in that class are noise.

I inlined everything into IndexedSymbols class since it's used only there.

tgodzik · 2023-10-25T14:55:13Z

metals/src/main/scala/scala/meta/internal/tvp/ClasspathTreeView.scala

-          case k.FIELD =>
-            if (child.properties.isEnum) "enum"
-            else "field"
+          case k.OBJECT | k.PACKAGE_OBJECT => "symbol-object"


I changed these to use the VS Code defaults (which can be replaced by a theme etc.) instead of the custom one. I think it makes sense to use the same everywhere and we were missing some such as type.

tgodzik · 2023-10-25T16:18:10Z

Och damn, one test is failing on ubuntu :/

tgodzik · 2023-10-25T19:18:21Z

tests/unit/src/main/scala/tests/TestingServer.scala

@@ -1831,13 +1835,34 @@ final case class TestingServer(
      .toMap
      .updated("root", "root")

+    def label(uri: String, default: String): String =
+      labelsMap.get(uri) match {
+        /* fallback for JDK sources since we seem to have issues on CI


I had to work around an issue in ubuntu CI jobs. We somehow had two different src.zip for Java in two different java homes. One was produced by reveal and the other from workspace search. Both should use exactly the same JDK homes, but somehow they don't.

I wasn't able to reproduce or figure it out at any point and I think it would be waste of time to fix it since it only influences tests.

Ok, maybe I will try one more thing.

tgodzik · 2023-10-25T19:22:09Z

mtags/src/main/scala/scala/meta/internal/mtags/CharArrayReader.scala

-        if (!Character.isLowSurrogate(lo))
-          readerError("invalid unicode surrogate pair", at = begCharOffset)
-        else {
+        if (!Character.isLowSurrogate(lo)) {


This started failing on some of the sources and I have no idea why. It's not really important as it's only used for indexing.

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

Also fix reveal, which didn't work before

mtags/src/main/scala/scala/meta/internal/metals/JdkSources.scala

tests/unit/src/main/scala/tests/QuickBuild.scala

mtags/src/main/scala/scala/meta/internal/mtags/Symbol.scala

mtags/src/main/scala/scala/meta/internal/mtags/Mtags.scala

metals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scala

kasiaMarek

looks good 👍

tgodzik · 2023-11-08T16:24:02Z

Double checked, but it seems I managed to get it faster than the previous approach now:

[info] Benchmark                  Mode  Cnt    Score     Error  Units
[info] ClasspathSymbolsBench.run    ss   10  804.875 ± 246.653  ms/op

Current:

[info] Benchmark                  Mode  Cnt    Score    Error  Units
[info] ClasspathSymbolsBench.run    ss   10  378.369 ± 18.148  ms/op

tgodzik force-pushed the alternative-tree-view branch 3 times, most recently from bf876d3 to 658291e Compare September 13, 2023 09:34

tgodzik commented Sep 13, 2023

View reviewed changes

tgodzik marked this pull request as ready for review September 13, 2023 09:36

tgodzik requested review from kasiaMarek, ckipp01 and jkciesluk September 13, 2023 09:36

kasiaMarek requested changes Sep 13, 2023

View reviewed changes

tgodzik force-pushed the alternative-tree-view branch 2 times, most recently from c077675 to 2cbdc08 Compare October 25, 2023 13:43

tgodzik force-pushed the alternative-tree-view branch from 2cbdc08 to 36e9926 Compare October 25, 2023 14:47

tgodzik requested a review from kasiaMarek October 25, 2023 14:47

tgodzik commented Oct 25, 2023

View reviewed changes

tgodzik force-pushed the alternative-tree-view branch 3 times, most recently from 5ef9193 to fc9cf4a Compare October 25, 2023 18:38

tgodzik commented Oct 25, 2023

View reviewed changes

tgodzik force-pushed the alternative-tree-view branch 7 times, most recently from 82738ec to 26ddcda Compare October 27, 2023 08:34

tgodzik added 2 commits October 27, 2023 12:09

bugfix: Fix issues with showing JDK sources

2ac898f

Also fix reveal, which didn't work before

tgodzik added 2 commits October 27, 2023 12:09

refactor: Use semanticdb and move everything to Indexed symbol class

507f837

refactor: Refactor symbols to use default icons

302be18

tgodzik force-pushed the alternative-tree-view branch 8 times, most recently from 8da5d1a to 55ba37c Compare November 2, 2023 15:03

refactor: Inline symbolsForPackage method

5f42d60

tgodzik force-pushed the alternative-tree-view branch from 55ba37c to 5f42d60 Compare November 2, 2023 16:25

kasiaMarek reviewed Nov 3, 2023

View reviewed changes

refactor: Move scala 3 library patching to mtags

c758243

tgodzik force-pushed the alternative-tree-view branch from 270b7a1 to a4558e8 Compare November 3, 2023 21:29

tgodzik requested a review from kasiaMarek November 3, 2023 21:32

bugfix: Fix off by one error when doing reveal

00371af

tgodzik force-pushed the alternative-tree-view branch from a4558e8 to 00371af Compare November 5, 2023 16:31

kasiaMarek approved these changes Nov 6, 2023

View reviewed changes

tgodzik merged commit b5177c9 into scalameta:main Nov 8, 2023
24 checks passed

tgodzik deleted the alternative-tree-view branch November 8, 2023 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement: Base Metals view on indexing information #5622

improvement: Base Metals view on indexing information #5622

tgodzik commented Sep 6, 2023 •

edited

Loading

tgodzik Sep 13, 2023

kasiaMarek left a comment

kasiaMarek Sep 13, 2023

kasiaMarek Sep 13, 2023

tgodzik Oct 25, 2023

tgodzik commented Oct 25, 2023

tgodzik Oct 25, 2023

tgodzik commented Oct 25, 2023

tgodzik Oct 25, 2023 •

edited

Loading

tgodzik Oct 25, 2023

tgodzik Oct 25, 2023

kasiaMarek left a comment

tgodzik commented Nov 8, 2023

improvement: Base Metals view on indexing information #5622

improvement: Base Metals view on indexing information #5622

Conversation

tgodzik commented Sep 6, 2023 • edited Loading

tgodzik Sep 13, 2023

Choose a reason for hiding this comment

kasiaMarek left a comment

Choose a reason for hiding this comment

kasiaMarek Sep 13, 2023

Choose a reason for hiding this comment

kasiaMarek Sep 13, 2023

Choose a reason for hiding this comment

tgodzik Oct 25, 2023

Choose a reason for hiding this comment

tgodzik commented Oct 25, 2023

tgodzik Oct 25, 2023

Choose a reason for hiding this comment

tgodzik commented Oct 25, 2023

tgodzik Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

tgodzik Oct 25, 2023

Choose a reason for hiding this comment

tgodzik Oct 25, 2023

Choose a reason for hiding this comment

kasiaMarek left a comment

Choose a reason for hiding this comment

tgodzik commented Nov 8, 2023

tgodzik commented Sep 6, 2023 •

edited

Loading

tgodzik Oct 25, 2023 •

edited

Loading