Skip to content
This repository has been archived by the owner on May 27, 2022. It is now read-only.

Simplify thread management with a bounded thread count #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

edahlgren
Copy link
Contributor

Simpler thread management:

  • set n worker threads
  • serve all requests to worker threads via a shared mvar queue
  • handle a request on a worker thread with a callback through the zmq Router
  • eliminate pipelining

Benefits:

  • less gc-ing of threads due to a fixed thread count

Potentially costs:

  • same requests will now all go to riak, all deserializing responses, potentially elevating gc

@@ -55,8 +56,7 @@ runZmqRpcWithContext ctx binda call = do
send s "" [SndMore]
send s m []
else do -- call
_ <- forkIO $ call m (zmqRpcReply ctx inproc zid)
return ()
Copy link
Contributor

Choose a reason for hiding this comment

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

@jamwt can you comment on why you originally designed this function to return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm curious as to why you think that change matters. @jamwt's version is semantically equivalent to void $ forkIO. Perhaps the first version of montage was trying to avoid "last statement in a 'do' block must be an expression" issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your new version doesn't forkIO though. It seems unnecessary to do it (since we're calling forkIO at the top level) but I was curious why it was done originally.

Copy link
Contributor

Choose a reason for hiding this comment

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

I now understand (from IRC).

@wmoss
Copy link
Contributor

wmoss commented May 10, 2013

This looks reasonable (except I don't understand the change I commented on).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants