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

Improve checking resumption on document update (document/didChange) #110

Closed
ejgallego opened this issue Dec 22, 2022 · 4 comments · Fixed by #111 or ocaml/opam-repository#22802
Closed
Milestone

Comments

@ejgallego
Copy link
Owner

ejgallego commented Dec 22, 2022

Resumption from the interruption case seems to be working pretty well.

However, for document update, we just restart from the beginning. This can be improved by actually finding a common prefix.

The original idea was to memoize parsing (see #36 and #25 ), however, at least in the current implementation, a much simpler computation of a common prefix would bring the same benefits AFAICS.

@ejgallego ejgallego added this to the 0.1.1 milestone Dec 22, 2022
@ejgallego ejgallego changed the title Fine tuning of resumption Improve checking resumption on document update (document/didChange) Dec 22, 2022
ejgallego added a commit that referenced this issue Dec 22, 2022
This does greatly reduce latency when editing deep down in a
document; closes #110 .

Also, kind of makes the need for parsing caching (#25, #36) moot, at
least with the current checking workflow.
@Alizter
Copy link
Collaborator

Alizter commented Dec 23, 2022

That's a smart idea. In fact we could easily just parse up until a ..

@ejgallego
Copy link
Owner Author

In fact we could easily just parse up until a . .

That's interesting, can you provide some more info (or an example) so I'm sure I understand what you mean?

@Alizter
Copy link
Collaborator

Alizter commented Dec 24, 2022

I mean roughly parse each step in Coq, by comparing prefixes at the . level. I.e.

Goal True.
foo.
bar.

and

Goal True.
foo.
baz.

Would have the same prefix, but only upto

Goal True.
foo.

rather than

Goal True.
foo.
ba

This is a rough approximation, and will suffer when we include statements like { and } but should be more reliable than just checking the text prefix.

@ejgallego
Copy link
Owner Author

The whole point of this is to avoid the latency parsing introduces; also note that in general you can't do that because it is perfectly possible that many parts of the document don't parse, also that's tricky due to the way Coq parsing is stateful.

should be more reliable than just checking the test prefix

Interesting, it seems the oposite to me. What is a case where that approach is more reliable that text-prefix?

Note that in a sense, using the common text prefix does get you the previous sentence (with lookup cost linear until we introduce a range map in Doc.t) as indeed you need to resume from a last valid token / state.

But indeed there are many details for this kind of algos so I'm happy to see a PR doing that and understanding how it would compare.

ejgallego added a commit that referenced this issue Dec 26, 2022
This does greatly reduce latency when editing deep down in a
document; closes #110 .

Also, kind of makes the need for parsing caching (#25, #36) moot, at
least with the current checking workflow.
ejgallego added a commit that referenced this issue Dec 26, 2022
This does greatly reduce latency when editing deep down in a
document; closes #110 .

Also, kind of makes the need for parsing caching (#25, #36) moot, at
least with the current checking workflow.
ejgallego added a commit that referenced this issue Dec 26, 2022
This does greatly reduce latency when editing deep down in a
document; closes #110 .

Also, kind of makes the need for parsing caching (#25, #36) moot, at
least with the current checking workflow.
ejgallego added a commit to ejgallego/opam-repository that referenced this issue Dec 27, 2022
CHANGES:

-------------------------

 - Don't crash if the log file can't be created (@ejgallego, ejgallego/coq-lsp#87)
 - Use LSP functions for client-side logging (@ejgallego, ejgallego/coq-lsp#87)
 - Log `_CoqProject` detection settings to client window (@ejgallego, ejgallego/coq-lsp#88)
 - Use plugin include paths from `_CoqProject` (@ejgallego, ejgallego/coq-lsp#88)
 - Support OCaml >= 4.12 (@ejgallego, ejgallego/coq-lsp#93)
 - Optimize the number of diagnostics sent in eager mode (@ejgallego, ejgallego/coq-lsp#104)
 - Improved syntax highlighting on VSCode client (@artagnon, ejgallego/coq-lsp#105)
 - Resume document checking from the point it was interrupted
   (@ejgallego, ejgallego/coq-lsp#95, ejgallego/coq-lsp#99)
 - Don't convert Coq "Info" messages such as "Foo is defined" to
   feedback by default; users willing to see them can set the
   corresponding option (@ejgallego, ejgallego/coq-lsp#113)
 - Send `$/coq/fileProgress` progress notifications from server,
   similarly to what Lean does; display them in Code's right gutter
   (@ejgallego, ejgallego/coq-lsp#106, fixes ejgallego/coq-lsp#54)
 - Show goals on click by default, allow users to configure the
   behavior to follow cursor in different ways (@ejgallego, ejgallego/coq-lsp#116,
   fixes ejgallego/coq-lsp#89)
 - Show file position in goal buffer, use collapsible elements for
   goal list (@ejgallego, ejgallego/coq-lsp#115, fixes ejgallego/coq-lsp#109)
 - Resume checking from common prefix on document update (@ejgallego,
   ejgallego/coq-lsp#111, fixes ejgallego/coq-lsp#110)
 - Only serve goals, hover, and symbols requests when the document
   has been sufficiently processed (@ejgallego, ejgallego/coq-lsp#120, fixes ejgallego/coq-lsp#100)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants