Civilized indexing progress reporting #1633
Conversation
| , _message = Just $ T.pack (fromNormalizedFilePath srcPath) <> progress | ||
| , _percentage = Nothing | ||
| , _message = Nothing | ||
| , _percentage = Just (100 * fromIntegral done / fromIntegral (done + remaining) ) |
There was a problem hiding this comment.
LSP says the _percentage has to be monotonic, but this may not be. That is why I chose not to fill this field in.
There was a problem hiding this comment.
It says "steadily rising", and "clients are free to ignore values that do not follow this rule".
Sounds like we should fill it in, and let clients decide what to do.
But I'm equally happy to just leave it empty. The main change for me here is making the message static by removing the module name from it
There was a problem hiding this comment.
Can we keep the progress numbers in the message?
Also, is there any particular reason why you want this change? I find the messages quite useful.
There was a problem hiding this comment.
It's very distracting on large repos. I will add an option to choose the style
There was a problem hiding this comment.
An option seems like it might not be worth it - its another choice a user has to make, and the value of making the choice seems outweighed by the existence of the choice.
FWIW, in Rust, the indexing shows indexing by package name. If we did that, would there be any problem on large repos? Or do we usually only index one package?
There was a problem hiding this comment.
I don't have strong opinions about keeping module names/files in the message, though I think keeping explicit progress numbers would be good.
For me, having the module names in there acts as a proxy for figuring out what files HLS is recompiling, which I find useful to track the state of the process and keep an eye out for in case I think its recompiling something I think it shouldn't.
I agree that an option doesn't seem ideal.
There was a problem hiding this comment.
Is the issue for large repos that it changes very often, to the point of almost flickering? If so, one way would be to say that we only show a module name after it's been taking > 0.3s to compile (and continue displaying it for 0.3s after it completes). That let's you see painful files, and if those painful files shouldn't be compiling people can act, but in many cases it won't say anything?
There was a problem hiding this comment.
Yes, something like that was on my TODO list as well. We should have a separate thread for reporting progress, and it can be responsible for debouncing as well.
That would also allow us to respect the spec in terms of progress messages. We aren't supposed to start sending progress events until we get a reply to the progressCreate message. I had implemented this (it is just a couple of lines waiting for a barrier), but removed it so as not to block the indexing thread on a response from the server. (also, all the other progress messages in ghcide also violate the spec in the same way, and I didn't have the stamina to fix those too).
There was a problem hiding this comment.
An option seems like it might not be worth it - its another choice a user has to make, and the value of making the choice seems outweighed by the existence of the choice.
I agree that an option doesn't seem ideal.
A user option wouldn't be ideal, agreed, however I wasn't talking about a user option, but a library option (IdeOptions) to control how progress is reported: via the _percentage field, via explicit numbers ("25/70") or none at all. HLS would then fix this to textual numbers to preserve the existing behaviour.
For me, having the module names in there acts as a proxy for figuring out what files HLS is recompiling, which I find useful to track the state of the process and keep an eye out for in case I think its recompiling something I think it shouldn't.
This information belongs in the diagnostics logs.
Is the issue for large repos that it changes very often, to the point of almost flickering?
Yes, it refreshes several times a second and includes absolute paths, making it unreadable (the progress numbers are not even visible). Even if it was readable it conveys no useful information anyway, since indexing is an implementation detail.
all the other progress messages in ghcide also violate the spec in the same way,
A simple improvement for this is to report progress only for a subset of rules, namely GenerateCore, Typecheck and GetModIface. The current behaviour when it progresses to the end, then back to 0, then back to the end is the result of reporting progress for all the rules.
But to make it truly monotonous we would need additional state. Something for another day though.
|
Updated with an option to choose the progress reporting style which defaults to the explicit style but allows to select percentage. |
pepeiborra
force-pushed
the
indexing-progress
branch
from
51e469c to
afa9c0b
Compare
pepeiborra
force-pushed
the
indexing-progress
branch
from
afa9c0b to
e84f658
Compare
* Civilized indexing progress reporting * optProgressStyle * Consistency: Indexing references ==> Indexing * Fix progress tests
* Civilized indexing progress reporting * optProgressStyle * Consistency: Indexing references ==> Indexing * Fix progress tests
* Start tracking provenance of stale data It's amazing how wrong this code used to be * Add some machinery for automagically updating the age * Add an applicative instance * Tracked ages makes everything much easier to reason about * Formatting * Haddock and small changes * Update haddock on IdeAction * Update to lsp-1.2 (#1631) * Update to lsp-1.2 * fix stack * fix splice plugin tests * fix tactic plugin tests * fix some tests * fix some tests * fix outline tests * hlint * fix func-test * Avoid reordering plugins (#1629) * Avoid reordering plugins Order of execution matters for notification plugins, so lets avoid unnecessary reorderings * remove duplicate plugins * fix tests * Civilized indexing progress reporting (#1633) * Civilized indexing progress reporting * optProgressStyle * Consistency: Indexing references ==> Indexing * Fix progress tests * Do not override custom user commands (#1650) Co-authored-by: Potato Hatsue <[email protected]> * Shut the Shake session on exit, instead of restarting it (#1655) Restarting the session will result in progress reporting and other messages being sent to the client, which might have already closed the stream Co-authored-by: Potato Hatsue <[email protected]> * Fix importing type operators (#1644) * Fix importing type operators * Update test * Add expected failure tests * log exceptions before killing the server (#1651) * log hiedb exceptions before killing the server * This is not the hiedb thread - fix message * Fix handler - either an error or success * additional .gitignore entries (#1659) * Fix ignore paths (#1656) * Skip individual steps * Skip individual steps * And needs pre_job * Add bounds for Diff (#1665) * Replace Barrier with MVar in lsp main (#1668) * Port UseStale to ghcide * Use the new ghcide UseStale machinery * Fix hlint complaints Co-authored-by: wz1000 <[email protected]> Co-authored-by: Pepe Iborra <[email protected]> Co-authored-by: Potato Hatsue <[email protected]> Co-authored-by: Javier Neira <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Screen.Recording.2021-03-28.at.19.56.05.mov