ShakeSession and shakeEnqueue

[pepeiborra]

Currently we start a new Shake session for every interaction with the Shake
database, including type checking, hovers, code actions, completions, etc.
Since only one Shake session can ever exist, we abort the active session if any
in order to execute the new command in a responsive manner.

This is suboptimal in many, many ways:

• A hover in module M aborts the typechecking of module M, only to start over!
• Read-only commands (hover, code action, completion) need to typecheck all the
  modules! (or rather, ask Shake to check that the typechecks are current)
• There is no way to run non-interfering commands concurrently

This is an experiment inspired by @mpickering ShakeQueue and
the follow-up discussion in mpickering#7

We introduce the concept of the ShakeSession as part of the IDE state.
The ShakeSession is initialized by a call to shakeRun, and survives until
the next call to shakeRun. It is important that the session is restarted as
soon as the filesystem changes, to ensure that the database is current.

The ShakeSession enables a new command shakeEnqueue, which appends work to
the existing ShakeSession. This command can be called in parallel without any
restriction.

shakeEnqueue then replaces shakeRun for all user actions that do not involve a change to the filesystem. It's very neat that this requires a single change in runAction.

The result is lightning fast performance for hover and code actions. Not so for completions, which usually arise while typing, and do not find an existing, warm ShakeSession. More thought is needed for these.

The hack used to inject more work in the ShakeSession quite possibly breaks internal Shake invariants, which is why this is a Request For Comments. I have tested it with GHC and ghcide itself, and haven't observed any issues.

TODO

• rework progress reporting mechanism

[mpickering]

Am I correct that this approach doesn't guarantee that all actions are completed? How would you implement the initial loading action or the propagating to parents behaviour of the shake queue?

It also seems hard to make completions fast using this approach without implementing something quite similar to what is implemented in the shake queue approach.

[pepeiborra]

Am I correct that this approach doesn't guarantee that all actions are completed? How would you implement the initial loading action or the propagating to parents behaviour of the shake queue?

It also seems hard to make completions fast using this approach without implementing something quite similar to what is implemented in the shake queue approach.

Correct, this is a minimal change to address only the issue of suboptimal cancellations. The idea is to address the performance issues one by one, step by step.

How does the queue make completions fast?

[mpickering]

The queue does not help directly but because completions uses stale information, the request to recompute the completions is placed into the queue after the request is dealt with to ensure that the completions information is refreshed.

The main benefit of the queue in my opinion is that every action is guaranteed to be completed so you can remove the very suboptimal kick.

[pepeiborra]

What is the actual impact of kick?

It causes a typecheck of all the files of interest, so if I only have one file open, then kick is actually optimal. But even in that scenario completions take >1s to appear, and that would not get better with improvements to kick as it is already optimal in this situation.

In another experiment (#555) I have confirmed that completions are blazingly fast with no kick at all, at the cost of soundness of course :)

[mpickering]

Is your question rhetorical or would you like me to give you my opinion?

[ndmitchell]

As to the Shake parts, that seems fine. Nothing breaks any internal invariants. In fact, I think you can go even further, and have pumpAction both continue pumping and fork off a new action in parallel. I'm not totally sure you can do that without consuming stack space though.

Something along these lines (either this approach or the one in @mpickering 's branch) seems like a good thing to do. I think we are quite quickly approaching the trade-off between correctness and performance, so maybe it's worth discussing that in words first? I believe the position of @cocreature is (was?) that we should never give incorrect information to an LSP query? It seems from this code and the branch that people (@pepeiborra and @mpickering) want to weaken that. How far? What invariants are required? What can we relax? How far can we relax it?

[pepeiborra]

Thanks Neil for confirming that this trick won't break any Shake invariants. Hopefully it doesn't break any ghcide invariants either.

While there is indeed a tension between accuracy and responsiveness, and I think accuracy can be relaxed in some cases (completions), I would not expect this PR to alter the current balance in ghcide. It increases the responsiveness of hovers, code actions and go-to definition, for free!

[pepeiborra]

Promoting this from RFC to Merge Request

[pepeiborra]

One issue with this approach is that ghcide doesn't send the WorkDoneProgressEnd notification timely, as the runShake thread stays up to support the ShakeSession.

I plan to fix this by attaching the progress reporting directly to kick

[pepeiborra]

Hmm, progress reporting still not working as expected.

UPDATE: pushed a change to fix progress reporting

[pepeiborra]

This is ready for final review. We have been using this branch at Strats for the past week, without issues, and:

• The perf improvements for hover, code actions and goto-def are very noticeable.
• It makes a huge difference in clients like emacs-lsp which send a hover request for every movement of the cursor.
• ShakeSession removes the need for code actions and other plugins to batch their calls to runAction, e.g. https://github.com/digital-asset/ghcide/blob/9129475b87f9ddf52b1599b086dc93c2f206d969/src/Development/IDE/Plugin/CodeAction.hs#L77-L80
  This is not only a nice-to-have, it's vital for compositional performance.

[ndmitchell]

The Shake bits look fine to me, and if it gives a measurable performance boost, that's all win. I appreciate @mpickering has gone off in a slightly different direction, so it might be worth thinking if that's the ultimate state, and if this is a good step in the direction, but in isolation it looks good and better to me.

The ghcide kick stuff is wrong, since if you have a file that used to be open (e.g. Temp.hs), and then shut it, and Temp.hs is no longer relevant, you should get rid of the warnings/errors about Temp.hs. In DAML that happens with garbage collect and a carefully written kick that knows about project roots etc. In ghcide that doesn't happen at all, since we wrote a crappy kick to get something released. Moving the kick logic into ghcide and doing it properly between DAML and Ghcide would be a good thing, but I think we should keep garbageCollect etc alive until we've done that.

[cocreature]

This looks great, thanks a lot! Having to kill the current shake session for actions that don’t change anything was by far my biggest gripe with the current architecture.

It would be great if @mpickering could weigh in here. I know he has a different plan here but I’m not too familiar with the details. If they don’t conflict, let’s merge this first. If they do conflict, I’d like to get some idea of the end state before we go off in one direction.

[pepeiborra]

I have pushed changes to restart the Shake session when a new component added. For this to work well (and pass the test suite), I had to implement re-enqueueing of cancelled user actions too.

[pepeiborra]

Nope, it's still failing occasionally, although I can no longer repro in VS Code

[pepeiborra]

Bumping the delay from 5 to 6 seconds in the multi2 test makes it always pass locally.

[mpickering]

Why is shakeSession wrapped in an MVar if you are already using a TQueue?

[pepeiborra]

The TQueue cannot be closed, so the MVar is needed to ensure that no more actions are added while the session is being restarted. It's likely that this can be simplified further, this is mostly an iteration on top of the previous design.

[mpickering]

Can you explain what is going on here? How are things requeued?

[pepeiborra]

cancelShakeSession returns a list of the pending tasks, which are then fed back into the new session in line 438

[mpickering]

Not sure this is the right way around. It is really the system actions we want to requeue in the end I think. This distinction seems premature anyway.

[pepeiborra]

'system' and 'user' actions are not the best names, but are the best I could come up late at night, sorry about that.

• system actions: typechecks. We don't want to reenqueue typechecks since a fresh set of them are always provided when restarting the session.

• user actions: everything else. They must always be completed (unless the lsp-client cancels them) or the clientMsgChan thread will deadlock.

[cocreature]

Awesome, thanks a lot!