Implement goto definition with low CPU and memory overhead #332

olafurpg · 2018-10-08T21:15:31Z

This PR replaces the existing navigation solution with a new implementation that is fast and uses little memory. To avoid making this PR larger, only goto definition is implemented.

The fast indexing is enabled by a new Scala parser that extract only toplevel classes from Scala sources at ~800k loc/s according to JMH "single shot" average time. This parser is implemented on top of the LegacyScanner, which is a fork of the Scala compiler scanner and is used by the Scalameta tokenizer. Java source files don't need indexing since their toplevel classes are inferred from their filename and can be computed on-demand.

The low-memory is achieved by storing only "non-trivial" toplevel classes in the index, that is toplevel members that don't match their enclosing file path. An index of the combined sources for akka+spark+finch+scalameta+linkerd+kafka+grpc uses ~17MB of memory, at ~1.4MB per 100k lines of Scala code. The index uses Scala stdlib Map with String keys so it might be possible to compress this further with a smarter data structure like prefix trees.

This PR removes pretty much everything from the original Metals codebase, including functional parts like the Main entrypoint, LSP functionality, sbt server integration and all tests suites. Some of those parts may get revived in the future but I think first we should first write more comprehensive tests for those components. In particular, the old implementation suffered from lack of e2e integration tests making it easy to silently introduce regressions.

This PR introduces a new testing infrastructure based on expect tests that optimizes ease of writing tests, code reviewing tests and updating existing tests once the expected behavior changes. Some test like DefinitionSuite stress a range of important high-level properties like "every identifier token should resolve to a definition and every non-identifier token should not resolve to a definition". These property check have so far been very effective at catching bugs.

This commit removes pretty much everything in Metals, including the test suites. The current implementation severals from several problems - it is tied to Scalameta v2.1.7 and upgrading to v4.0.0 is difficult - it consumes a lot of memory, up to ~2-3GB for medium sized codebases. - it is slow at indexing dependency sources, importing a medium/large sized project can take several minutes. - it uses complex persistent caching to make up for slow indexing, storing data on the users disk. This implementation detail leaked into the user experience through the "Clear index cache" command. - tests are difficult to write, read and update to changing requirements. There are some good parts that are removed in this commit but may get revived in the future: - token edit distance works great for stale SemanticDB files - sbt server integration works and has an integration test - MetalsServices + Main + Configuration

- upgrade scalameta - remove unnecessary projects - use resource generators to feed input project classpath/sourceDirectories/sourceJars into test suite.

Reformat with Prettier.

We're going to implement a parser with the LegacyScanner (tokenizer) and this basic utilty will come in handy.

These methods were spread out through different objects and now they are in one place only.

This commit implements a high-performance Scala parser that only exctracts top-level symbols from Scala source files. This parser needs to be fast because it is on the critical path when importing a new project and we index the depenency sources.

This commit introduces a new module mtags that implements goto definition with a new design that achieves the following goals: - source indexing is fast, Scala sources are indexed at ~800k loc/s and Java sources don't require any indexing altogether. - the in-memory index uses little ram, an index for ~1.5 million lines of Scala takes ~15MB. The new symbol indexer is able to handle different kinds of sources: source files, sources.jar and SemanticDB files. Source and SemanticDB can complement each other, the indexer falls back source indexing when the SemanticDB file is either missing or is incomplete due to for example macro annotations. An important difference from the old indexer is that the new indexer is not responsible for computing the range position inside the source file. The final range position is left for a separate module that handles other concerns such as stale SemanticDBs using edit distance.

These are copy-pasted from the Scalameta test suite.

- BaseSuite: replaces utest DSL with ScalaTest FunSuite syntax. - BaseExpectSuite: replaces utest DSL with ScalaTest FunSuite syntax. - SingleFileExpectSuite: expect test with a single output file - DirectoryExpectSuite: expect test with a directory containing multiple expected output files. - InputProperties: metadata about the input project.

Test cases are plain Scala code, no magic assertions. This set of files stress a range of interesting Scala and Java language features.

This test suite is the most advanced of the current test suites, it tests a range of functionality: - synthetic symbols from case classes fallback to non-synthetic symbols - source + semanticdb indexing can be combined to support navigation to macro annotations.

We care equally about memory useage and CPU time to index sources. Rough summary of the results: - indexing 7 million loc Java and 1.5 million loc Scala takes ~15M ram - Scala topelevel parser handles ~800k loc/s The numbers are so good that I'm not too stressed about micro-optimization as the moment. We run the full indexer for a medium-sized classpath on every unit test without annoying me, total runtime of `unit/test` is ~4 seconds.

``` > bench/jmh:run -i 10 -wi 10 -f1 -t1 [info] Benchmark Mode Cnt Score Error Units [info] MetalsBench.indexSources ss 10 0.620 ± 0.058 s/op [info] MetalsBench.javaMtags ss 10 7.233 ± 0.017 s/op [info] MetalsBench.scalaMtags ss 10 4.617 ± 0.034 s/op [info] MetalsBench.scalaToplevels ss 10 0.361 ± 0.005 s/op ```

- Only deal with definitions, no references. - No SemanticDB indexing, only mtags. - Document OnDemandSymbolIndex.

We needed to add the enclosing source directory to the API to handle this case. Added detailed documentation to the methods.

olafurpg · 2018-10-09T21:37:38Z

Updated the PR with more docs and simplified the GlobalSymbolIndex interface to only be about finding definitions of global symbols from plains source files, no SemanticDB needed.

gabro

First pass of comments, I haven't got to the bottom of this yet.

gabro · 2018-10-09T05:59:53Z

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

+          loadFromSourceJars(trivialPaths(toplevel)).foreach(addSemanticdb)
+      }
+    }
+    if (!definitions.contains(symbol)) {


Is this repeated because the above can mutate definitions?

gabro · 2018-10-13T12:10:30Z

build.sbt

-      "org.scalameta" %% "testkit" % V.scalameta % Test,
+      "org.scalameta" %% "scalameta" % V.scalameta,
+      "org.scalameta" %% "symtab" % V.scalameta,
+      "org.scalameta" % "interactive" % V.scalameta cross CrossVersion.full,


is this still in use?

Nope, ls4ps is not strictly used either because the metals project has 2 source files at the moment. I will clean up the metals project once we add language server stuff.

I meant the interactive part specifically, but there's no rush to clean this up now, I agree

gabro · 2018-10-13T12:12:26Z

docs/installation-contributors.md

-  2.12.6 are not supported.
-* Java 8. Note that Java 9 or higher has not been tested.
-* macOS or Linux. Note that there have been reported issues on Windows.
+- Scala 2.12.7, down to the exact PATCH version. Note that Scala versions 2.11.x


maybe make the Scala version dynamic using V.scala212?

Good idea, done.

The docs don't build actually at the moment because the Configuration object is now gone. We'll need to update the docs before there is a release.

gabro · 2018-10-13T12:15:27Z

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

+/**
+ * Utility to load relative paths from a classpath.
+ *
+ * Provides similar functionality as URLClassLoader but does no caching.


Is the no-caching a feature or a limitation? Maybe it's worth explaining why this class is needed over URLClassLoader

Most important bit is that it uses AbsolutePath and Option[T] instead of java.net.URL and nulls. Updated docstring to reflect that.

Lack of caching was actually out-dated, the underlying implementation uses URLClassLoader at the moment.

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

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

gabro · 2018-10-13T12:35:17Z

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

+
+}
+
+case class SymbolDefinition(


Maybe SymbolDefinitionPath or SymbolDefinitionFile?

Ending with Path makes the querySymbol/definitionSymbol fields a bit confusing IMO

At the same time, this is not really a symbol definition. It's the file where the symbol is defined, so that was confusing at a first read

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

gabro · 2018-10-13T12:36:33Z

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

+import scala.meta.internal.mtags.Enrichments._
+
+/**
+ * Utility to generate method symbol  disambiguators according to SemanticDB spec.


double space between symbol and disambiguators

Removed, only if we had a formatting tool that could reformat our comments 🤔

gabro · 2018-10-13T12:37:18Z

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

+          // java.lang.NullPointerException: null
+          // at com.thoughtworks.qdox.builder.impl.ModelBuilder.createTypeVariable(ModelBuilder.java:503)
+          // at com.thoughtworks.qdox.builder.impl.ModelBuilder.endMethod(ModelBuilder.java:470)
+          // TODO(olafur) report bug to qdox.


Has this been reported? 😇

I don't think so, didn't manage to minimize the error. Replaced the comment with explanation why we ignore parse errors.

While removing the unused `DefinitionAlternatives.methodToVal` I realized that DefinitionSuite didn't properly document which cases relied on fallbacks.

olafurpg

Thanks for the detailed review, a lot of great questions @gabro !

olafurpg · 2018-10-13T12:59:59Z

build.sbt

-      "org.scalameta" %% "testkit" % V.scalameta % Test,
+      "org.scalameta" %% "scalameta" % V.scalameta,
+      "org.scalameta" %% "symtab" % V.scalameta,
+      "org.scalameta" % "interactive" % V.scalameta cross CrossVersion.full,


Nope, ls4ps is not strictly used either because the metals project has 2 source files at the moment. I will clean up the metals project once we add language server stuff.

olafurpg · 2018-10-13T13:01:15Z

docs/installation-contributors.md

-  2.12.6 are not supported.
-* Java 8. Note that Java 9 or higher has not been tested.
-* macOS or Linux. Note that there have been reported issues on Windows.
+- Scala 2.12.7, down to the exact PATCH version. Note that Scala versions 2.11.x


Good idea, done.

olafurpg · 2018-10-13T13:02:23Z

docs/installation-contributors.md

-  2.12.6 are not supported.
-* Java 8. Note that Java 9 or higher has not been tested.
-* macOS or Linux. Note that there have been reported issues on Windows.
+- Scala 2.12.7, down to the exact PATCH version. Note that Scala versions 2.11.x


The docs don't build actually at the moment because the Configuration object is now gone. We'll need to update the docs before there is a release.

olafurpg · 2018-10-13T13:04:37Z

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

+/**
+ * Utility to load relative paths from a classpath.
+ *
+ * Provides similar functionality as URLClassLoader but does no caching.


Most important bit is that it uses AbsolutePath and Option[T] instead of java.net.URL and nulls. Updated docstring to reflect that.

olafurpg · 2018-10-13T13:05:47Z

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

+/**
+ * Utility to load relative paths from a classpath.
+ *
+ * Provides similar functionality as URLClassLoader but does no caching.


Lack of caching was actually out-dated, the underlying implementation uses URLClassLoader at the moment.

olafurpg · 2018-10-13T13:33:07Z

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

+    }
+
+  /** Fallback to the val term for a def with multiple params */
+  private def methodToVal(symbol: String): Option[String] =


It's not used, removed.

olafurpg · 2018-10-13T13:34:48Z

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

+   *
+   * Returns the path of the file that defines the symbol but does not include
+   * the exact position of the definition. Computing the range position of the
+   * definition is left for the client and can be done using the mtags module.


clarified with

the exact position of the definition. Computing the range position of the
definition is not handled by this method, it is left for the user and can
be done using the mtags module.

olafurpg · 2018-10-13T13:37:23Z

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

+  def addSourceFile(
+      file: AbsolutePath,
+      sourceDirectory: Option[AbsolutePath]
+  ): Unit


Currently this throws an exception for errors like tokenization failures. I'm not sure yet what the best error handling method so I'd prefer to refine it down the road once we have a clearer picture of how it will be used in the language server. Maybe the underlying implementation can accept a Reporter 🤔

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

olafurpg · 2018-10-13T13:38:53Z

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

+
+}
+
+case class SymbolDefinition(


Ending with Path makes the querySymbol/definitionSymbol fields a bit confusing IMO

gabro

Added two other minor comments

gabro · 2018-10-14T20:23:47Z

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

+ *
+ * Fast indexing is enabled by ScalaToplevelMtags, a custom parser that extracts
+ * only toplevel symbols from a Scala source file. Java source files don't need indexing
+ * because their file location can be inferred from the symbol itself.


I would add a reference to the note from above, where you list the limitations of this approach.

gabro · 2018-10-14T20:26:39Z

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

+) extends GlobalSymbolIndex {
+  val mtags = new Mtags
+  private val sourceJars = new ClasspathLoader(Classpath(Nil))
+  var indexedSources = 0L


Can this be accessed concurrently? Maybe using an AtomicInteger would be safer.

gabro · 2018-10-15T09:50:38Z

An overall comment that I have is about the pervasive use of String and extensions methods on it.

It feels a bit brittle and I suspect it makes it difficult to refactor some of the code.

Especially the use of String for symbols makes me a bit uneasy and I wished it were a distinct type (even through the use of a newtype).

Biggest change was wrapping all strings in a `Symbol` class. There's currently no standard symbol class in scalameta that we can reuse so we copy-paste what exists in scalafix and adjust to our needs.

olafurpg · 2018-10-17T09:21:32Z

An overall comment that I have is about the pervasive use of String and extensions methods on it.

That's a good point. I've updated the PR to use a wrapper class Symbol(val value: String). Let's see how that work, I'd love to get a data structure like this into org.scalameta:semanticdb that we can reuse but so far it doesn't exist so we live with copy-paste.

Thank you @gabro for the review, a lot of great comments 🙏

I will merge this now after CI is green, next step is to setup a bsp client 💪

Implement goto definition with low CPU and memory overhead

olafurpg added 23 commits October 8, 2018 20:34

Refactor the build

c18495b

- upgrade scalameta - remove unnecessary projects - use resource generators to feed input project classpath/sourceDirectories/sourceJars into test suite.

Update docs to reflect latest scala version

ec15dcf

Reformat with Prettier.

Add utility object to map LegacyScanner tokens to readable names

2c327bb

We're going to implement a parser with the LegacyScanner (tokenizer) and this basic utilty will come in handy.

Add useful extension methods.

f6c4366

These methods were spread out through different objects and now they are in one place only.

Expose helper method to change the scribe logging format

5e0a941

Add coursier utilities to fetch sources of libraries

ae09098

These are copy-pasted from the Scalameta test suite.

Write test cases.

4132081

Test cases are plain Scala code, no magic assertions. This set of files stress a range of interesting Scala and Java language features.

Add ToplevelSuite to test toplevel symbol extraction

17e30c9

Add SemanticdbSuite as a baseline for other test suites

07fdb8d

Add MtagsSuite to test JavaMtags and ScalaMtags

6f9acc4

Add ScalaToplevelSuite to assert that it's a subset of Mtags.

c94ea7a

Add SaveExpect class to save expect test outputs.

75ac015

Ignore generated expect files from scalafmt

4dcb2e9

Reformat sources

2566691

Remove dead imports with Scalafix

f91e7d8

Refine benchmarkd for toplevel symbols and memory usage

fa1351e

olafurpg changed the title ~~Implement high-performance goto definition~~ Implement goto definition with low CPU and memory overhead Oct 8, 2018

olafurpg added 5 commits October 8, 2018 23:30

Fix scripted tests

a2bb282

Add readme with results for benchmarks

6f4192b

Simplify GlobalSymbolIndex

6519b46

- Only deal with definitions, no references. - No SemanticDB indexing, only mtags. - Document OnDemandSymbolIndex.

Reproduce bug for in-project Java symbols referenced from Scala

3579a69

Fix bug in handling of Java source files on disk.

53cd1dd

We needed to add the enclosing source directory to the API to handle this case. Added detailed documentation to the methods.

olafurpg requested a review from gabro October 9, 2018 21:37

gabro reviewed Oct 13, 2018

View reviewed changes

Address review comments

44b0f4c

While removing the unused `DefinitionAlternatives.methodToVal` I realized that DefinitionSuite didn't properly document which cases relied on fallbacks.

olafurpg commented Oct 13, 2018

View reviewed changes

gabro approved these changes Oct 15, 2018

View reviewed changes

Address review comments

889b282

Biggest change was wrapping all strings in a `Symbol` class. There's currently no standard symbol class in scalameta that we can reuse so we copy-paste what exists in scalafix and adjust to our needs.

olafurpg merged commit 0c3aef0 into scalameta:master Oct 17, 2018

olafurpg deleted the super-fast branch October 17, 2018 09:29

olafurpg added a commit that referenced this pull request Feb 14, 2019

Merge pull request #332 from olafurpg/super-fast

a7ec32c

Implement goto definition with low CPU and memory overhead

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement goto definition with low CPU and memory overhead #332

Implement goto definition with low CPU and memory overhead #332

olafurpg commented Oct 8, 2018 •

edited

Loading

olafurpg commented Oct 9, 2018

gabro left a comment

gabro Oct 9, 2018

gabro Oct 13, 2018

olafurpg Oct 13, 2018

gabro Oct 13, 2018

gabro Oct 13, 2018

olafurpg Oct 13, 2018

olafurpg Oct 13, 2018

gabro Oct 13, 2018

olafurpg Oct 13, 2018

olafurpg Oct 13, 2018

gabro Oct 13, 2018

olafurpg Oct 13, 2018

gabro Oct 13, 2018

gabro Oct 13, 2018

olafurpg Oct 13, 2018

gabro Oct 13, 2018

olafurpg Oct 13, 2018

olafurpg left a comment

olafurpg Oct 13, 2018

olafurpg Oct 13, 2018

olafurpg Oct 13, 2018

olafurpg Oct 13, 2018

olafurpg Oct 13, 2018

olafurpg Oct 13, 2018

olafurpg Oct 13, 2018

olafurpg Oct 13, 2018

olafurpg Oct 13, 2018

gabro left a comment

gabro Oct 14, 2018

gabro Oct 14, 2018

gabro commented Oct 15, 2018

olafurpg commented Oct 17, 2018

Implement goto definition with low CPU and memory overhead #332

Implement goto definition with low CPU and memory overhead #332

Conversation

olafurpg commented Oct 8, 2018 • edited Loading

olafurpg commented Oct 9, 2018

gabro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabro commented Oct 15, 2018

olafurpg commented Oct 17, 2018

olafurpg commented Oct 8, 2018 •

edited

Loading