Fix #23: throw ScopeClosing if trying to fork in a closed scope#25
Fix #23: throw ScopeClosing if trying to fork in a closed scope#25mitchellwrosen merged 2 commits intomainfrom
Conversation
ki/src/Ki/Internal/Scope.hs
Outdated
| -- Sentinel value: -1 means the scope is closing. | ||
| -- Sentinel value: -2 means the scope is closed. |
There was a problem hiding this comment.
Why do you use sentinel values instead of a proper sum type?
There was a problem hiding this comment.
It'd be good to benchmark but my answer at the moment is efficiency. 🤷
There was a problem hiding this comment.
Hm, I think it'd be good to verify that! One sentinel value is liveable, but two starts getting very hairy! But then again, it's not my library 😆
There was a problem hiding this comment.
Hehe, good point. Well, I'd like to think of it not as "my" library either! I'll add benching that to the todo list.
There was a problem hiding this comment.
It would be interesting to know what is the overhead of using a sum type here. Out of curiosity I wrote this playground https://play.haskell.org/saved/r85araK7 , and it looks like doing n >= 0 produces more core than a pattern match. Though I can't tell if that's better or not :)
ki/src/Ki/Internal/Scope.hs
Outdated
| assert (n >= -2) do | ||
| if | ||
| | n >= 0 -> writeTVar startingVar $! n + 1 | ||
| | n == -1 -> throwSTM ScopeClosing | ||
| | otherwise -> throwSTM (ErrorCall "ki: scope closed") |
There was a problem hiding this comment.
If you had a sum type this would be a much cleaner case imo.
|
Ok, @tstat and I looked into the cmm and asm a bit here. We discovered that listing the data ScopeStatus
= Closing
| Closed
| Open {-# UNPACK #-} !Int(It's kind of lame that GHC doesn't take the user's match order into account). All else seemed equal, as far as just the price of pattern matching and reassembling one of these things goes. However, the sum type is less desirable when closing a scope because, while waiting for the number of starting threads to drop to 0, know that the constructor is So, I think a fair compromise is to just introduce a pattern synonym for readability. It's not as safe as a proper sum type of course. If anyone wants to put together an benchmark that actually measures the (probably negligible) performance hit of a few extra comparison instructions here and there, please go ahead. Because the little integer state machine isn't that hard to grok (0+ to -1 to -2), and looks decently comprehensible with the pattern synonyms, I'm declaring this good enough for now. |
| -- Record the child as having started. Not allowed to retry. | ||
| atomically do | ||
| n <- readTVar startingVar | ||
| writeTVar startingVar $! n - 1 -- it's actually ok to go from e.g. -1 to -2 here (very unlikely) |
There was a problem hiding this comment.
This comment was inaccurate; we know n is 1 or more here.
Fixes #23