Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make find implementation run on a separate thread for each file #989

Merged
merged 1 commit into from Oct 16, 2019

Conversation

@tgodzik
Copy link
Collaborator

tgodzik commented Oct 14, 2019

Previously, textDocument/implementation collected results in a single thread which could be slow in some large workspaces. This PR makes textDocument/implementation collect results in parallel showing up to 2x speedups in some cases.

@tgodzik tgodzik force-pushed the tgodzik:implementaion-future branch from ef0f6ce to f50a8c2 Oct 14, 2019
@tgodzik tgodzik requested review from marek1840 and olafurpg Oct 14, 2019
@tgodzik tgodzik force-pushed the tgodzik:implementaion-future branch 2 times, most recently from 2add119 to 55f6640 Oct 15, 2019
} parallelMap.foreach {
case (file, locations) =>
val implPath = AbsolutePath(file)
for {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 16, 2019

Member

readability: should we maybe move this for comprehension into a separate method?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 16, 2019

Author Collaborator

I can actually move it back in since foreach is being invoked by default there.

parallelMap = findImplementation(
symbolClass.symbol,
classContext
).par

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 16, 2019

Member

I'm not sure how the parallel map works, I generally only use parallel arrays

val map = findImplentation(...)
keys.toArray.par.foreach { file =>
  val locations = map(file)
  // ...
}

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 16, 2019

Author Collaborator

Shouldn't it work basically the same?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 16, 2019

Author Collaborator

From what I checked the only real issue is with collections like List, which are sequential by nature. Map should be fine since it allows easily for concurrent access.

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 16, 2019

Author Collaborator

After a bit more research, Arrays seem to be most efficient really, so changed to that. Thanks! :)

@tgodzik tgodzik force-pushed the tgodzik:implementaion-future branch 2 times, most recently from d7978a9 to 939e7da Oct 16, 2019
@tgodzik tgodzik force-pushed the tgodzik:implementaion-future branch from 939e7da to 6817154 Oct 16, 2019
@tgodzik tgodzik requested a review from olafurpg Oct 16, 2019
Copy link
Member

olafurpg left a comment

LGTM 💯

Do you maybe have any numbers from akka/akka before and after?

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 16, 2019

LGTM

Do you maybe have any numbers from akka/akka before and after?

Around twice from what I calculated quickly.
But usually it's around 200 ms while before around 500ms

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 16, 2019

Aside from the timeout, Implementation tests passed and since the change is isolated I am merging it

@tgodzik tgodzik merged commit b11321c into scalameta:master Oct 16, 2019
1 of 3 checks passed
1 of 3 checks passed
build build
Details
scalameta.metals Build #20191016.12 failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tgodzik tgodzik deleted the tgodzik:implementaion-future branch Oct 16, 2019
@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Oct 17, 2019

Un-scientific results from running "find implementation" on the Actor.receive method in akka/akka

// before
[Trace - 11:33:37 AM] Sending response 'textDocument/implementation - (19)'. Processing request took 1376ms

// after
[Trace - 11:28:40 AM] Sending response 'textDocument/implementation - (24)'. Processing request took 846ms

To reproduce,

  • setup metals in the akka/akka repo and index everything.
  • enable lsp.trace.json, create the file if it doesn't exist (exact path varies between OS)
tail -f /Users/lgeirsson/Library/Caches/org.scalameta.metals/lsp.trace.json | grep textDocument/
  • Run "Reload Window" to start a clean server.
  • Run "go to implementation" on the Actor trait to warm up the server.
  • Then run "go to implementation" on the def receive method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.