refactor: simplify merlin diagnostics #1005
Conversation
b76d042 to
e0cbc79
Compare
| let+ document = f document in | ||
| { doc with document = Some document } | ||
| in | ||
| Table.set t.db uri doc) |
There was a problem hiding this comment.
@ulugbekna this code was evil. We're updating a value in the hash table after waiting for the termination of f, but the value might be out of date by then due to a concurrent document change notification.
| @@ -128,22 +128,15 @@ module Single_pipeline : sig | |||
| -> f:(Mpipeline.t -> 'a) | |||
| -> ('a, Exn_with_backtrace.t) result Fiber.t | |||
| end = struct | |||
| type t = | |||
| { thread : Lev_fiber.Thread.t | |||
| ; mutable last : (Text_document.t * Mconfig.t * Mpipeline.t) option | |||
There was a problem hiding this comment.
does this mean that we don't try reusing the last pipeline even if it's the same document?
There was a problem hiding this comment.
Yes, the optimization is gone. I don't think it ever worked anyway given that it relied on physical equality of the Mconfig.t being returned.
| Document_store.parallel_iter document_store ~f:(fun doc -> | ||
| match Document.kind doc with | ||
| | `Other -> Fiber.return doc | ||
| | `Other -> Fiber.return () | ||
| | `Merlin merlin -> | ||
| let doc = Document.update_text doc [] in | ||
| let+ () = | ||
| Diagnostics.merlin_diagnostics diagnostics merlin | ||
| in | ||
| doc) | ||
| Diagnostics.merlin_diagnostics diagnostics merlin) | ||
| in | ||
| Diagnostics.send diagnostics `All |
There was a problem hiding this comment.
Does this re-make pipelines and send diagnostics to all files on a dune build completion (when dune is in watch mode)? Sounds super expensive
There was a problem hiding this comment.
Wait until you see how expensive is the build itself :)
But more seriously, of course it would be better to recompute only the documents which were invalidated by the build. Unfortunately that would be far more difficult.
There was a problem hiding this comment.
the documents which were invalidated by the build. Unfortunately that would be far more difficult.
But dune knows what those documents are, right? (because of the incremental re-building it has)
If yes, that becomes a matter of extending the dune rpc?
There was a problem hiding this comment.
the documents which were invalidated by the build. Unfortunately that would be far more difficult.
actually, yes, I started noticing how my laptop fans go off often if I have dune running in watch mode
There was a problem hiding this comment.
Yes, that would be enough. In particular, the rule would be that a merlin document should be invalidated whenever:
- Any .cmi file in its
-Idirectories has been invalidated - Any of the preprocessing binaries has been invalidated
- Any of the runtime dependencies of the preprocessing binaries has been invalidated
There was a problem hiding this comment.
actually, yes, I started noticing how my laptop fans go off often if I have dune running in watch mode
Thankfully you can limit the concurrency with -j. That calms down the fans considerably.
e0cbc79 to
c438bc2
Compare
Updating the document with no changes is no longer needed because we don't reuse the pipeline. This also fixes a bug whenever we're editing a document and dune is running in watch mode. The issue was that we were updating a document the store in the background and overwriting updates to the document requested by the user. Signed-off-by: Rudi Grinberg <me@rgrinberg.com> <!-- ps-id: a79143e1-01bc-4565-b900-ee5d97474ced -->
c438bc2 to
917509f
Compare
CHANGES: ## Fixes - Fix race condition when a document was being edited and dune in watch mode was running ([ocaml/ocaml-lsp#1005](ocaml/ocaml-lsp#1005), fixes [ocaml/ocaml-lsp#941](ocaml/ocaml-lsp#941), [ocaml/ocaml-lsp#1003](ocaml/ocaml-lsp#1003))
CHANGES: ## Fixes - Fix race condition when a document was being edited and dune in watch mode was running ([ocaml/ocaml-lsp#1005](ocaml/ocaml-lsp#1005), fixes [ocaml/ocaml-lsp#941](ocaml/ocaml-lsp#941), [ocaml/ocaml-lsp#1003](ocaml/ocaml-lsp#1003))
Updating the document with no changes is no longer needed because we
don't reuse the pipeline.
Signed-off-by: Rudi Grinberg me@rgrinberg.com