improvement: use pc for finding references of local symbols and when semanticdb is missing #5940

kasiaMarek · 2023-12-13T15:04:51Z

semanticdb only:
time: found 459 references to symbol 'scala/Boolean#`&&`().' in 0.25s
bloom filters + pc (that's a bit nonsensical, since bloom filters are build based on semanticdb):
time: found 429 references to symbol 'scala/Boolean#`&&`().' in 6.47s

Note: This won't work for definitionOrReferences if semanticdb is missing. But it requires a bit more effort to make it work reasonably, so I'd rather leave it as a followup.
connected to: #5759

jkciesluk

Great work!
I have some questions, but nothing major

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

jkciesluk · 2024-01-02T11:35:03Z

metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala

+ set.foreach(bloom.put)
+ }
+
+ private def collectIdentifiers(


Why do we need this? Isn't the referencesProvider().addIdentifiers(source, identifiers) in Indexer.scala enough?

It updates identifiers on didSave.

metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala

mtags/src/main/scala-3/scala/meta/internal/pc/PcSymbolSearch.scala

mtags/src/main/scala-3/scala/meta/internal/pc/PcRenameProvider.scala

mtags/src/main/scala-3/scala/meta/internal/pc/PcReferencesProvider.scala

mtags/src/main/scala-3-wrapper/ScalaPresentationCompiler.scala

tgodzik

That is some great work, would be awesome if we really just allow user only a bit slower experience when semanticdb is missing.

tgodzik · 2023-12-29T11:44:24Z

mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java

+ /**
+ * Returns the references of the symbol under the current position in the target files.
+ */
+ public CompletableFuture<java.util.List<DefinitionResult>> references(OffsetParams params, java.util.List<VirtualFileParams> targetFiles, boolean includeDefinition) {


We should probably add an additional ReferenceResults class just for the sake of clarity. And also we could have MultipleFileOffsetParams or ReferenceOffsetParams, so we would not need more params

tgodzik · 2024-01-18T08:19:39Z

metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala


 case class IndexEntry(
 id: BuildTargetIdentifier,
 bloom: BloomFilter[CharSequence],
 )
 val index: TrieMap[Path, IndexEntry] = TrieMap.empty
+ val tokenIndex: TrieMap[Path, IndexEntry] = TrieMap.empty


Do you have an idea how much more memory this takes up? And maybe how much longer indexing takes? Or are the indexing times negligible since we already tokenize the files?

I'll check both. Indexing should be similar but there may be additional cost to building the bloom filter.

In metals repo:
memory: identifier index using 1.37M (77,536 elements)
Benchmarks:

Benchmark Mode Cnt Score Error Units MetalsBench.alltoplevelsScalaIndex ss 10 0.419 ± 0.003 s/op MetalsBench.alltoplevelsScalaIndexWithCollectIdents ss 10 0.499 ± 0.011 s/op MetalsBench.alltoplevelsScalaIndexWithBuildIdentifierIndex ss 10 0.627 ± 0.016 s/op

We should be fine then, that's not a lot 👍

The only issue I can think of is anything that is not named like apply methods, but we can try and handle that later.

Sure. I guess the best way to go about it would be not to send targetUris right away but have pc resolve the name. We could also use untyped trees. Anyway it will quickly get hairy for things like:

case class Foo() { def apply() } val f = Foo() val h = f()

We could also look at synthetic symbols as we already have some logic for it (or at least the apply one) and we search for apply, it should be found properly

tgodzik · 2024-01-18T08:23:00Z

metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala

+ }
+ if targetFiles.nonEmpty
+ } yield compilers
+ .references(params, targetFiles, EmptyCancelToken)


Would be great to forward an actual cancel token so if it takes longer to find references this can actually be cancelled.

metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala

tgodzik · 2024-01-18T08:30:07Z

mtags/src/main/scala-2/scala/meta/internal/pc/PcReferencesProvider.scala

+class PcReferencesProvider(
+ compiler: MetalsGlobal,
+ params: OffsetParams,
+ targetFiles: List[VirtualFileParams],


I wonder if we shouldn't read the files on demand instead of sending them in the params here. There are cases when we will not need targetFiles to be read at all. So maybe we should just read it here. Also sending too many large files might cause OOM maybe?

Yeah, I think it's reasonable to ask for batches instead of giving all at once.

mtags/src/main/scala-2/scala/meta/internal/pc/PcReferencesProvider.scala

kasiaMarek · 2024-02-20T11:00:46Z

Maybe we should also use pc for files with stale semanticDB?

tgodzik · 2024-02-20T11:28:48Z

Maybe we should also use pc for files with stale semanticDB?

That makes sense, would reduce the need to calculate the diff, which isn't 100% sure anyway. We can do that later if needed.

tgodzik · 2024-04-02T11:19:32Z

mtags/src/main/scala-3-wrapper/ScalaPresentationCompiler.scala

+ params.params().uri().toString(),
+ highlight.getRange(),
+ )
+ val adjust =


Could we adjsut things in Compilers as with all other requests?

tgodzik · 2024-04-02T11:20:40Z

metals/src/main/scala/scala/meta/internal/metals/Compilers.scala

@@ -1362,3 +1401,11 @@ object Compilers {
 case object Default extends PresentationCompilerKey
 }
 }
+
+case class PcAdjustFileParams(


I would not add these new params if possible to just adjsut results afterward.

mtags-interfaces/src/main/java/scala/meta/pc/ReferencesRequest.java

tgodzik · 2024-04-02T11:25:10Z

tests/slow/src/test/scala/tests/feature/PcReferencesLspSuite.scala

+
+ check(
+ "basic3",
+ """|/a/src/main/scala/O.scala


Might be worth adding a few mroe tests here

tgodzik · 2024-04-26T16:39:58Z

metals/src/main/scala/scala/meta/internal/metals/Compilers.scala

@@ -703,6 +719,28 @@ class Compilers(
 }
 }.getOrElse(Future.successful(Nil.asJava))

+ def references(
+ params: ReferenceParams,
+ targetFiles: List[AbsolutePath],


Suggested change

targetFiles: List[AbsolutePath],

searchInFiles: List[AbsolutePath],

tgodzik · 2024-04-26T16:49:44Z

metals/src/main/scala/scala/meta/internal/metals/Compilers.scala

+ .map(
+ _.asScala.toList.map { defRes =>
+ val locations = defRes.locations().asScala.toList
+ ReferencesResult(defRes.symbol(), locations)


Can we run adjust on locations here?

tgodzik · 2024-04-26T16:55:11Z

metals/src/main/scala/scala/meta/internal/metals/Compilers.scala

+ val request = PcReferencesRequest(
+ CompilerOffsetParamsUtils.fromPos(pos, token),
+ params.getContext().isIncludeDeclaration(),
+ targetFiles.map(_.toURI).asJava,


What if instead, we first run references on the main file, which then returns the symbol and then we use that symbol on each of the other files? I think we were talking about it at some point, but it seems much less added mental overhead.

We would need to maybe create ReferenceParams that contain:
Either[Position, Symbol]

If it's a position then we search at the specific position and all references, otherwise we search for symbol. We don't need to have the additional Buffers etc.

tgodzik · 2024-04-26T17:04:15Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala


- private val compilers: Compilers = register(
+ val compilers: Compilers = register(


Why remove private?

I can't find it being used currently, could be private

tgodzik

Some minor comments, otherwise a see a few things for a follow up:

we should close presentation compiler for non active targets, otherwise we might easily go OOM in for example Bazel
we don't search for references if we do go to def on definition
it seems like sometimes we get duplicated location, where one location is empty, not sure if I can reproduce, but will try

tgodzik · 2024-05-08T14:37:49Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala


- private val compilers: Compilers = register(
+ val compilers: Compilers = register(


I can't find it being used currently, could be private

metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala

tgodzik · 2024-05-08T14:44:25Z

mtags/src/main/scala-2/scala/meta/internal/pc/PcReferencesProvider.scala

+import org.eclipse.{lsp4j => l}
+
+trait PcReferencesProvider {
+ _: WithCompilationUnit with PcCollector[(String, Option[l.Range])] =>


Ideally WithCompilationUnit would also be a trait so that we can use normal inheritance, but if possible it can be fixed later.

I'm not sure yet how to do that nicely.

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

kasiaMarek · 2024-05-09T10:25:37Z

we should close presentation compiler for non active targets, otherwise we might easily go OOM in for example Bazel

👍

we don't search for references if we do go to def on definition

included in the PR description

it seems like sometimes we get duplicated location, where one location is empty, not sure if I can reproduce, but will try

I purposefully return ReferencesResult from pc even if there are no locations (can be only def, that we don't care about), so we get the full list of symbols, that we want to search for (

metals/metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala

Line 334 in 0985176

compilers.references(params, EmptyCancelToken).flatMap { foundRefs =>

). ~~I added a filter to the final result.~~ EDIT: Can't do that actually, is that a problem? Don't results get mapped to locations anyway?

kasiaMarek force-pushed the pc-references branch 5 times, most recently from a4d5d77 to af2ba5a Compare December 20, 2023 11:46

kasiaMarek force-pushed the pc-references branch 3 times, most recently from ec87650 to 64412d6 Compare December 21, 2023 09:50

kasiaMarek marked this pull request as ready for review December 21, 2023 15:34

kasiaMarek requested review from tgodzik and jkciesluk December 21, 2023 15:34

tgodzik added the backport-scala3-pc label Dec 27, 2023

jkciesluk reviewed Jan 2, 2024

View reviewed changes

kasiaMarek force-pushed the pc-references branch from 405ac4a to ca68174 Compare January 2, 2024 16:32

tgodzik requested changes Jan 18, 2024

View reviewed changes

kasiaMarek changed the title ~~improvement: use pc for finding references of local symbols~~ improvement: use pc for finding references of local symbols and when sementicdb is missing Jan 18, 2024

kasiaMarek force-pushed the pc-references branch from b0f6da9 to 27707f9 Compare January 22, 2024 20:39

kasiaMarek mentioned this pull request Feb 7, 2024

LSP references are not found for better-monadic-for implicits #6096

Open

kasiaMarek force-pushed the pc-references branch from 3993147 to 5e021f6 Compare February 13, 2024 17:18

kasiaMarek changed the title ~~improvement: use pc for finding references of local symbols and when sementicdb is missing~~ improvement: use pc for finding references of local symbols and when semanticdb is missing Feb 20, 2024

kasiaMarek force-pushed the pc-references branch from 5e021f6 to 3e2b2eb Compare February 20, 2024 16:29

kasiaMarek force-pushed the pc-references branch from 3e2b2eb to 8db6722 Compare March 15, 2024 10:39

kasiaMarek requested a review from tgodzik March 15, 2024 11:55

tgodzik requested changes Apr 2, 2024

View reviewed changes

tgodzik self-requested a review April 3, 2024 09:31

tgodzik reviewed Apr 26, 2024

View reviewed changes

kasiaMarek force-pushed the pc-references branch from 363234b to 0985176 Compare April 29, 2024 16:09

kasiaMarek requested a review from tgodzik April 30, 2024 07:36

tgodzik approved these changes May 8, 2024

View reviewed changes

kasiaMarek force-pushed the pc-references branch from 0985176 to 0368fc4 Compare May 9, 2024 10:25

kasiaMarek force-pushed the pc-references branch from 0368fc4 to 3e7362c Compare May 9, 2024 15:35

kasiaMarek force-pushed the pc-references branch 2 times, most recently from 04c2c9f to 3321128 Compare May 20, 2024 11:03

kasiaMarek added 18 commits May 21, 2024 13:09

improvement: use pc for finding references of local symbols

297db9a

delete unused referencedPackages

32ab999

use pc for references as fallback when missing semanticdb

70b682d

move collecting identifiers to mtags

68d012a

fixes

ac41e33

small review changes

294e8ec

add back compileAndLookForNewReferences

746ff16

benchmarks

532c63a

get file content on demand

f10b7e8

small fix

707f15a

scalafix

3869724

fixes after rebase

61e1c22

add test for rename with un-compiled build target

2c2f840

post rebase fixes

f6b2816

refactor: drop using buffers in pc

107421a

review fixes

5af876a

filter if empty locations

fbc44f2

post rebase fixes

5208db2

kasiaMarek force-pushed the pc-references branch from 3321128 to 5208db2 Compare May 21, 2024 11:18

kasiaMarek merged commit d12e21e into scalameta:main May 22, 2024
22 of 24 checks passed

kasiaMarek mentioned this pull request Jun 10, 2024

Port pc find references from metals scala/scala3#20544

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement: use pc for finding references of local symbols and when semanticdb is missing #5940

improvement: use pc for finding references of local symbols and when semanticdb is missing #5940

kasiaMarek commented Dec 13, 2023 •

edited

jkciesluk left a comment

jkciesluk Jan 2, 2024

kasiaMarek Jan 2, 2024

tgodzik left a comment

tgodzik Dec 29, 2023

tgodzik Jan 18, 2024

kasiaMarek Jan 18, 2024

kasiaMarek Jan 18, 2024

tgodzik Jan 18, 2024

kasiaMarek Jan 22, 2024

tgodzik Jan 22, 2024

tgodzik Jan 18, 2024

tgodzik Jan 18, 2024

kasiaMarek Jan 18, 2024

kasiaMarek commented Feb 20, 2024

tgodzik commented Feb 20, 2024

tgodzik Apr 2, 2024

tgodzik Apr 2, 2024

tgodzik Apr 2, 2024

tgodzik Apr 26, 2024

tgodzik Apr 26, 2024

tgodzik Apr 26, 2024

tgodzik Apr 26, 2024

tgodzik May 8, 2024 •

edited

tgodzik left a comment

tgodzik May 8, 2024 •

edited

tgodzik May 8, 2024

kasiaMarek May 9, 2024

kasiaMarek commented May 9, 2024 •

edited

	targetFiles: List[AbsolutePath],
	searchInFiles: List[AbsolutePath],


		private val compilers: Compilers = register(
		val compilers: Compilers = register(

improvement: use pc for finding references of local symbols and when semanticdb is missing #5940

improvement: use pc for finding references of local symbols and when semanticdb is missing #5940

Conversation

kasiaMarek commented Dec 13, 2023 • edited

jkciesluk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik 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

kasiaMarek commented Feb 20, 2024

tgodzik commented Feb 20, 2024

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

tgodzik May 8, 2024 • edited

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasiaMarek commented May 9, 2024 • edited

kasiaMarek commented Dec 13, 2023 •

edited

tgodzik May 8, 2024 •

edited

tgodzik May 8, 2024 •

edited

kasiaMarek commented May 9, 2024 •

edited