Make tickets reliable #86
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* All the `unsafeCoerce` tricks to try to block up inlining are quite unreliable, especially these days. We can get much more reliable results by making `Ticket` a datatype, and using `lazy` when peeking into it. Do that. * Remove the `Eq Ticket` instance. It was never sensible or lawful. * Add strictness annotations to suppress warnings. * Add a `peekTicketA` function to extract the value from a ticket in an `Applicative` context, so we can unbox the ticket even if the value within will be used lazily. * Make `atomicModifyIORefCAS` and `atomicModifyIORefCAS_` strictness as consistent as possible in the fall-back case where we use `atomicModifyIORef`. As I wrote it, the code is only as strict as it should be for `base >= 4.13` / `GHC >= 8.8.1`. The previous promise never to install a thunk in the `IORef` simply can't be kept in the fall-back case; adjust the documentation to match reality. Fixes rrnewton#5. Fixes rrnewton#69.
treeowl
force-pushed
the
reliable-tickets
branch
from
26e7e07 to
e6f6ac4
Compare
|
@jberryman Do you think you still have what you need to see if this happens to fix #77? That one looks pretty involved. |
|
hey thanks for working on this. it should be easy to delete some of the few other tests in this file to get something totally self-contained that just depends on atomic-primops, if you want to try it on your branch: https://github.com/jberryman/unagi-chan/blob/master/tests/Atomics.hs#L94 |
|
Hi, friendly ping. This seems like an important PR with some critical bug fixes for some years-old tickets. Just curious, what's the status -- is this repo still maintained? (Would be great to throw up an "unmaintained" banner on Hackage if not!) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All the
unsafeCoercetricks to try to block up inlining are quite unreliable, especially these days. We can get much more reliable results by makingTicketa datatype, and usinglazywhen peeking into it. Do that.Remove the
Eq Ticketinstance. It was never sensible or lawful.Add strictness annotations to suppress warnings.
Add a
peekTicketAfunction to extract the value from a ticket in anApplicativecontext, so we can unbox the ticket even if the value within will be used lazily.Make
atomicModifyIORefCASandatomicModifyIORefCAS_strictness as consistent as possible in the fall-back case where we useatomicModifyIORef. As I wrote it, the code is only as strict as it should be forbase >= 4.13/GHC >= 8.8.1. The previous promise never to install a thunk in theIORefsimply can't be kept in the fall-back case; adjust the documentation to match reality.Fixes #5.
Fixes #69.