-
Notifications
You must be signed in to change notification settings - Fork 36
readTQueueN #91
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
base: master
Are you sure you want to change the base?
readTQueueN #91
Conversation
If we want this function at all, it should come with some clearly documented caveats.
I'm tempted to say let's not do this, given the pitfalls. |
I need this functionality whether I like it or not, and I could implement it on top of the existing package, but I'd end up re-implementing lots of existing logic and I'd have worse performance issues. In my case N is usually 3 and would never exceed about 5, so I'm not really bothered about complexity in N. I'm got rid of case 2 cos I realised that it's superfluous. Now I just retry instead, letting the items pile up on the write side.
I agree that your first point should be documented but it's hardly a show stopper - I can't think why people would have two threads consuming the same queue in different chunk sizes. If they did, then surely it would be obvious that the bigger chunk would rarely be obtained. This code does tolerate a mixture of Moving to your second point, I could avoid calling It's not outlandish to want this functionality, and if the module doesn't provide it, then people have to do it themselves. If they don't fork the repo (if only to export the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with the documented caveats. Can we have a test please?
Control/Concurrent/STM/TQueue.hs
Outdated
-- | 0 < read < N | retry | retry | case 4 | | ||
-- +--------------+-----------+--------------- +-----------------+ | ||
-- | read >= N | . . . . . . . case 1 . . . . . . . . . | | ||
-- +----=--------------------------------------------------------+ | ||
|
||
-- case 1a: More than N: splitAt N read -> put suffix in read and return prefix | ||
-- case 1b: Exactly N: Reverse write into read, and return all of the old read | ||
-- case 2: Move reverse write to read, retry | ||
-- case 2: No longer exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renumber the cases please
Control/Concurrent/STM/TQueue.hs
Outdated
-- |Reads N values, blocking until enough are available | ||
-- |Reads N values, blocking until enough are available. | ||
-- This is likely never to return if another thread is | ||
-- blocking on readTQueue. It has quadratic complexity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: markup identifiers e.g. 'readTQueue'
Control/Concurrent/STM/TQueue.hs
Outdated
-- |Reads N values, blocking until enough are available. | ||
-- This is likely never to return if another thread is | ||
-- blocking on readTQueue. It has quadratic complexity | ||
-- in n due to each write triggering readTQueueN to calculate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: N
not n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a test in Stm066.hs. I made the other changes.
This function retries until N items are available in a TQueue without removing them, and then returns them all at once. In some cases a small
<>
happens, but if the calling code is left to do this the performance cost is higher. It also becomes quite messy and is almost tantamount to reimplementing a queue in front of the queue.