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

Unify critical session running in hls #4256

Merged
merged 38 commits into from
Jun 8, 2024

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented May 27, 2024

see #4251
What's done

  1. add Development.IDE.Core.Thread to provide common api to run a woker thread
  2. rewrite runWithDb using the Development.IDE.Core.Thread
  3. add session loading thread and session restart thread using Development.IDE.Core.Thread
  4. send session loading runs to session loading thread
  5. send session restart runs to session restart thread

The above modification ensure the shutdown handler can cancel the session loading and session restart. And hence we would have a more correct shutdown behaviour. Potentially make the test less likely to hang.

@soulomoon soulomoon marked this pull request as ready for review May 29, 2024 10:48
@soulomoon soulomoon requested review from wz1000 and fendor as code owners May 29, 2024 10:48
@soulomoon soulomoon added the performance Issues about memory consumption, responsiveness, etc. label Jun 4, 2024
ghcide/src/Development/IDE/Core/Thread.hs Outdated Show resolved Hide resolved
@@ -0,0 +1,67 @@
module Development.IDE.Core.Thread
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a more typical name for something like this would be a "work queue" or "job queue".

ghcide/src/Development/IDE/Core/Thread.hs Outdated Show resolved Hide resolved
-- Return ContT to run the action
runWithThread :: ThreadRun input workerResource resource arg -> input -> ContT () IO (resource, TQueue arg)
runWithThread ThreadRun{..} ip = ContT $ \f -> do
tRunWithResource ip $ \w r -> do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this kind of looks like two separate things to me:

  1. A domain-specific withResource which creates the resource and passes it to a continuation.
  2. A standard "worker queue"

My instinct is that we could focus on defining the worker queue, and just leave the creation of the resource, where necessary, to the call site. Indeed, many of the call sites don't create a resource at all! I think we could then quite naturally write:

withMyResource $ \resource -> withWorkerQueue (\arg -> doSomething resource arg) $ \queue -> ...

or something?

Copy link
Collaborator Author

@soulomoon soulomoon Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, let's implement withWorkerQueue instead to see if that would make better sense

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better, makes sense to me.

I do wonder: if we are using blockRunInThread, then it looks like it's pretty much identical to having a resource that's guarded by a lock, and then having the waiting threads block on acquiring the lock. If we put work into the queue and the move on so it can be executed asynchronously, then it is different. But if we immediately block and wait then there's no real effect of it happening in another thread!

ghcide/src/Development/IDE/Core/WorkerThread.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Core/WorkerThread.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Core/WorkerThread.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Core/WorkerThread.hs Outdated Show resolved Hide resolved
@soulomoon
Copy link
Collaborator Author

soulomoon commented Jun 8, 2024

it's pretty much identical to having a resource that's guarded by a lock

yes, with a bit different, enqueued actions would not be cancelled even if the thread start it is cancelled.

It reminds me of something, we might probably want to limit some of the queue size to 1, so the waiting to enqueue action can be cancelled when we are doing shakeRestart. Then less likely a duplication actions would be run for the sessionLoadQueue
But maybe in another PR, we might need to come up with some design, such as we might need a bit wrapping to do over TQueue and TMVar if we do it this way.

@soulomoon soulomoon enabled auto-merge (squash) June 8, 2024 17:10
@soulomoon soulomoon disabled auto-merge June 8, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues about memory consumption, responsiveness, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants