Process shake restarts asynchronously

[crtschin]

Would close #4725.

Implements the ideas from #4725 (comment). Makes the following changes:

1. Processes notifications asynchronously keeping with the logic that notifications preceding any request, also finish before that request is processed. But does so asynchronously.
2. The above opens up the possibility that there could be multiple shake restarts in-flight because multiple notifications arriving in quick succession. Instead of storing these restarts in a queue, accumulate all restart changes in a single slot, to be processed at once.

[crtschin]

I can't get tests to pass with notifications fully async. Kinda makes sense looking at what these notifications indicate. I thought I could get away with only LSP handling everything synchronously of the VFS-side, but it appears not.

I'm now trying out a small variant where notifications are still synchronously, but that the session restarts are initiated async, while still blocking subsequent LSP requests. So restarts still get squashed.

[soulomoon]

good work on the restart squash.
But if notifications are still handled synchronously, #4725 won't be close since consective typing emits consective notifications and their restarts won't be able to merge

[soulomoon]

better to extend the logic of workerThread module so it can be reused here.

there is a slight chance that downstream code would swallow the async exception and hang the shutdown. workerThread handled that by setting up a shutdown flag .

[crtschin]

But if notifications are still handled synchronously, #4725 won't be close since consective typing emits consective notifications and their restarts won't be able to merge

Thanks for taking a peak at the PR! The idea I'm going for now here is to still have notifications be synchronous, but that they don't wait for the shake restart to finish, unlike now. So multiple subsequent edits will issue multiple synchronous notifications which will all start asynchronous shake restarts. This allows the restarts to merge.

Note that this is still me experimenting, I'm not sure if I'm breaking preconditions. This is all quite nuancy.

[soulomoon]

So multiple subsequent edits will issue multiple synchronous notifications which will all start asynchronous shake restarts. This allows the restarts to merge.

good idea

they don't wait for the shake restart to finish

I’ve experienced this before: if we don’t wait for Shake to fully restart, requests may observe stale state. I think we should ensure any pending restarts have completed before processing new requests but keep the notifications side of the story as you suggested, It might work.

Also it might be a good idea to go through the occurences of mkPluginNotificationHandler and find out if bug would occur.

----- update ----

Also we should move the "updatePositionMapping" in the notification handler into restart so every restart(merged or not) would see consistent mapping state ?

[crtschin]

Alright, report time. Tests now pass. It now runs the notification handlers synchronously but any shake restart is done asynchronously through a worker thread. Some notes.

• I refactored WorkerThread so I could swap out the queue for a single slot that merges elements.
• While shake restarts are asynchronously, most notifications have side-effects they want to have happen inbetween triggering shake restarts, e.g. changing the set of FOI. Tripped me up quite a bit what to do with this in the context of merging shake restarts, the current version runs these prior to even submitting the shake restart.
• The eval plugin contains a request that triggers a shake restart and subsequently waited on it. As the shake restart is now async, I had to add a blocking wait on the shake restart finishing.
• Other variations I tried but didn't pass tests:
  • Running notifications async.
  • Running notifications sync, with async shake restarts, but accumulating and calling the notification actions in between shake restarts.

I ran the benchmarks in #4934, with smaller (very small) typing delays, which is optimal for observing the effect of this PR, but not super realistic. I couldn't observe a difference with the default delay of 0.25s.

Running the typing burst benchmarks with very small typing delays

Example, Version, Configuration, name, success, samples, startup, setup, userT, delayedT, 1stBuildT, avgPerRespT, totalT, rulesBuilt, rulesChanged, rulesVisited, rulesTotal, ruleEdges, ghcRebuilds, maxResidency, allocatedBytes
cabal, upstream, All, hover after typing burst, True, 100, 4.07, 0.00, 6.49, 14.59, 0.55, 0.03, 21.09, 18, 15, 1164, 4042, 19942, 134, 286MB, 90219MB
cabal, HEAD, All, hover after typing burst, True, 100, 3.99, 0.00, 5.95, 14.93, 0.67, 0.03, 20.89, 18, 15, 1181, 4041, 19942, 114, 277MB, 88055MB
cabal, upstream, All, getDefinition after typing burst, True, 100, 0.66, 0.00, 7.51, 14.49, 0.64, 0.03, 22.02, 17, 14, 1183, 4045, 19949, 137, 271MB, 85329MB
cabal, HEAD, All, getDefinition after typing burst, True, 100, 0.57, 0.00, 6.10, 12.72, 0.75, 0.03, 18.83, 18, 15, 1181, 4043, 19949, 119, 279MB, 79739MB
cabal, upstream, All, completions after typing burst, True, 100, 0.67, 0.00, 9.62, 12.36, 0.76, 0.04, 21.99, 19, 16, 1182, 4043, 19941, 127, 348MB, 90711MB
cabal, HEAD, All, completions after typing burst, True, 100, 0.56, 0.00, 8.96, 11.11, 0.88, 0.04, 20.09, 19, 16, 1182, 4042, 19941, 117, 317MB, 88975MB

[soulomoon]

any shake restart is done asynchronously through a worker thread

@crtschin consider this, notification A then request B. How do we ensure B run after A's session restart in this PR ?

[crtschin]

@crtschin consider this, notification A then request B. How do we ensure B run after A's session restart in this PR ?

Good question, I'll walk you through an example timeline.

1. Notification A adds a lock identifying itself here.
2. Notification A gets executed.
3. Notification A queues up a shakeRestart via one of the set*Modified helpers.
4. Notification A spawns a thread waiting on the restart being finished and fills the lock it created in step (1) when the shake restart is finished.
5. Notification A finishes
6. Request B starts being processed asynchronously
7. Request B waits on all notification locks that were submitted prior.
8. Worker thread finishes the shake restart submitted by notification A.
9. Request B proceeds and gets handled.

Critically it's the locks that are added by notifications, which are waited on by follow-up requests, that ensures the ordering.

[soulomoon]

@crtschin Good work and thanks for the explaination.
There is a problem about ioActionBetweenShakeSession.

[soulomoon]

why do we not need to waitForLastRestart for unqueueForEvaluation as well ?

[crtschin]

Good one, I was thinking waiting was only needed for the session setup, but it makes sense for the eval itself as well.

[soulomoon]

ioActionBetweenShakeSession should be run after all the shake rules that rely on these states fully stop.
If it happpens here, there is chance that the newer state being picked up by an old step rule run and result in inconsistancy of the shake database. It is used to be an old bug stay in hls for years until it was fixed.

[crtschin]

Hmm, I tried this, but some tests failed. I'll try this again, as some of the failures may have been eval-related.

[soulomoon]

Thanks, some tests failed probably means something is out of sync. We need to be very careful and find out all of the possible out of sync parts.

[crtschin]

I think I got where the failure I encountered came from, it was TH interpreter related. A new interpreter is created on each init of a HscEnv in a session for each module. If a restart occurs concurrently with a load, I believe the loader could observe different TH splice declarations in dependencies, which are unloaded in a different interpreter they are loaded in on.

The fix in the last commit is to keep a MVar that keeps the first interpreter that's ever instantiated and shares it across all HscEnv initializations. There are already explicit calls to unload in the interpreter, so I think it's safe to keep a single instance around permanently.

[soulomoon]

The TH interpreter is out of my area, I know nothing about it.
perhaps @fendor can review the TH interpreter part ?

[soulomoon]

atomically $ updatePositionMapping ide (VersionedTextDocumentIdentifier _uri _version) [] in places like mkPluginNotificationHandler LSP.SMethod_TextDocumentDidOpen might need to go into setFileModified too for cases it is called

---

In fact I am not very sure about this, but worth thinking about this moving part. On the one hand, we need it to be sync with the internal shake state, on the other hand, we need it to sync with the actual file state. We are bound to have a close/open world gap here. Perhaps the better idea is to halt the shake session immediatly when a restart is needed. But that was a whole lot of changes.

---

See lastValueIO, we would attach the version of the mapping to the build result using the this.

[crtschin]

I think I see what you meant now with the position mapping. I didn't quite get it at first. So the 2 approaches are:

1. Previously: Update position mapping in the notification handler.
2. Proposed: Update position mapping when the shake restart is processed, in-between the shake session being up.

Consider the following sequence of events:

a. Request $R$ arrives
b. Notification $N$ arrives
c. Request $R$ executes its actions.
d. Notification $N$ restart is processed.

With approach (1), the position mapping is updated between steps (b) and (c). This means that $R$ sees the most up-to-date position mapping.

With approach (2), the position mapping is updated after step (d). This means that $R$ saw the same position mapping as was valid at the time it started being processed.

Thinking about this, I think both might be wrong. With approach (1) it might be possible for an in-progress request action to see multiple different position mappings for a single rule. Approach (2) is wrong in that the request would be working with a position mapping that doesn't reflect the file.

I think the right solution would be for the processing of a request to be atomic relative to a position mapping. This would require useWithStaleFast (and lastValueIO) to be in STM. This would be a pretty huge refactor though, as useWithStaleFast is a 6-year old function that's used in a lot of places.

---

Converting back to draft, as this notes a pretty fundamental issue with the PR.