Metals: Feedback very slow in some cases

[japgolly]

I experimented with Metals recently and although it's still early days, it's already impressive and gives me a really good feeling. Thanks for the hard and good work! The potential is profound and I'm looking forward to seeing this grow. :)

One of the projects I tried using Metals with, is scalajs-react. Here in Callback.scala is a signature

  def widen[B >: A]: CallbackTo[B] =

I changed >: to <:, hit Save, and in a second or two, it was highlighted as in error in vscode. Great! Then I changed it back to >: again, hit Save, waited over 10 seconds and it was still being highlighted as an error. After some confusion and more experimentation, I later discovered that when I hit save it would kick off a compile, and because widen now has the correct signature, instead of the compiler failing quickly it would go on to compile all dependencies of widen and succeed after ~25sec. Only then would the results be processed by Metals in which case the compiler error on my screen would go away, but there'd by a 25sec delay and I'd need to know where to look to find the compilation status while I wait.

It would be awesome the compiler error would disappear soon after successfully being compiled, rather than at the end of all compilation.

If we're already streaming compiler errors and not waiting for a batch of results at the end of compilation, I appreciate this could still pose some difficulty. You'd need the compiler to also emit success events in addition to failure events which might need a filter or granularity adjustment for performance if emitting events for all successes comes with too much overhead... I mean I'm just imagining how this stuff would work, I actually have no idea how it does in reality so I might be way off. If my thoughts aren't helpful, just ignore them. Thanks anyway for the great work!

[olafurpg]

Thanks a lot for the feedback! I agree ~25s is too slow to clear such a simple error. The cause of this issue is that we currently wait until the whole build target finishes compilation to clear diagnostics. This means that displaying compile errors is often faster than removing them.

We should investigate how to speed up removal of diagnostics. The actual fix will be in Bloop where diagnostics are published (Metals just forwards them) but it's good to keep this open here if other people experience something similar. cc/ @jvican

[jvican]

If I understand this ticket correctly, bloop's behavior is correct and the slow feedback should be attributed to Metals. When errors disappear, bloop clears diagnostics right after it successfully compiles the target -- it doesn't wait until all its dependencies are compiled to clear errors.

[olafurpg]

@jvican Metals immediately forwards diagnostics from bloop as they are published. The problem is that bloop calls noDiagnostic at the end of a compilation https://github.com/scalacenter/bloop/blob/e28603007bd1d2985fa2f461d4435d4bd79cde15/frontend/src/main/scala/bloop/reporter/BspProjectReporter.scala#L102
It might be possible to publish those empty diagnostics sooner for example right after typer for each individual compilation unit. One possible workaround we could do on the Metals side is publish empty diagnostics when a *.semanticdb file is created.

[jvican]

After some confusion and more experimentation, I later discovered that when I hit save it would kick off a compile, and because widen now has the correct signature, instead of the compiler failing quickly it would go on to compile all dependencies of widen and succeed after ~25sec.

@olafurpg Are you sure that's the problem at hand? ^ This quote makes me think the problem is not that bloop calls noDiagnostic at the end of compilation, otherwise I don't understand why @japgolly refers to "compile all dependencies" and the compilation takes 25s (when it's incremental and for a single target).

It might be possible to publish those empty diagnostics sooner for example right after typer for each individual compilation unit.

If we eventually agree and zoom in on that being an issue, we could give it a try, but I'm skeptical it would be worth it. Typer usually takes 60-70% of the compilation time being optimistic, so the gain would be marginal. This could also cause glitches in the user experience if the error cleared after typer was not emitted by typer but by a posterior compiler phase such as refchecks

Let's figure out what's causing this issue and we decide what the best solution is. I personally like the simplicity of our current implementation

[japgolly]

If I've been unclear about anything, you'd like to know more about how to reproduce this, or any questions at all, just give me a shout :)

[jvican]

Some basic data points for scalajs-react:

1. Compiling core in a more or less hot bloop server takes 9 seconds, in a cold server takes 26s.
2. If you start with a cold metals session and don't have bloop installed in your machine, the following numbers will be slower.

I later discovered that when I hit save it would kick off a compile, and because widen now has the correct signature, instead of the compiler failing quickly it would go on to compile all dependencies of widen and succeed after ~25sec.

I've looked into your example, there were two things going wrong that are now fixed and will in bloop master (soon 1.2.1):

1. Making a change, compiling with an error and then undoing the change should give a noop compilation. In our case, it wasn't.
2. As the noop compilation wasn't happening, zinc would try to compile your project incrementally. This incremental compile is exceptionally inefficient for a change in the public interface of an important class such as Callback. The inefficiency comes from zinc triggering 4 incremental compiler cycles (first for 1 source, then for 17, then for 35 and then for 71 sources), then delaying until the very end the clearing of the errors.

This is what changes with bloop 1.2.1:

1. Bloop does the noop compilation correctly.
2. Even if you manage to find another corner case in the incremental compiler and trigger several subsequent incremental compiles, the clearing of errors will happen after every incremental compiler cycle, meaning bloop will gradually clear diagnostics. For example, in the case reported in scalajs-react, the first compilation will clear diagnostics in one source, the second will clear them in 17 sources, the third one will clear them in 35 sources and in the last incremental compilation will clear diagnostics in 71 sources. This incremental clearing of diagnostics will give the impression of a faster feedback cycle

Summary:

1. Zinc is inefficient when compiling incrementally Callback, similar errors can happen in sources of other codebases that are very popular within the codebase. Until that inefficiency is not improved (I plan to look into a fix for the next major zinc release) this kind of slow feedback can still happen
2. Get faster feedback by having a bloop installed globally if you open/close vscode often (more than 3 times a day)
3. Even if the problem happens, latest bloop improves the responsiveness of the reporting by clearing the allowed errors after every incremental compiler cycle

[japgolly]

Thanks so much @jvican, that's really amazing!! I really love all the work you and @olafurpg are investing into Bloop & Metals! Thank you

[olafurpg]

@japgolly I was able to reproduce your widen example in the scalajs-react repo and can confirm that the error message now disappears right after save

However, the slow >20 second compilation still happens preventing blocking any subsequent feedback. Something is happening in Callback.scala that makes zinc recompile everything on every change even for adding a local println. Although the long-term solution may be achievable by improving zinc, we might want to consider ways to stop ongoing compilation to unblock new compilations...

Please keep the feedback coming! Concrete examples like widen are very helpful to track down what's going on.

[olafurpg]

@japgolly you might want to enable the compiler option -Ycache-plugin-class-loader:last-modified in scalajs-react. It makes a big difference in my experiments, see #455.

[japgolly]

@olafurpg Ok great! I'll give that a try, thanks for the suggestion. And yeah I'll keep the feedback coming, I'm glad to help found a troublesome case 😁