-
Notifications
You must be signed in to change notification settings - Fork 375
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 ] Add atomically function #3380
Conversation
…y test atomicModifyIORef functionality.
libs/base/Data/IORef.idr
Outdated
|
||
||| This function atomically modifies the contents of an IORef. | ||
||| This function is useful for using IORef in a safe way in a multithreaded program. | ||
||| If you only have one IORef, then using atomicModifyIORef to access and modify it will prevent race conditions. | ||
||| Any backend other than the default (chez) will perform modifyIORef (non-atomic). | ||
export | ||
atomicModifyIORef : HasIO io => IORef a -> (a -> a) -> io () | ||
atomicModifyIORef ref f | ||
= case codegen of | ||
"chez" => do mutex <- makeMutex | ||
mutexAcquire mutex | ||
val <- readIORef ref | ||
writeIORef ref (f val) | ||
mutexRelease mutex | ||
_ => modifyIORef ref f |
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.
This cannot work. Two threads that compete for an atomic reference must be using the same mutex. Here, you create a new mutex for every new call to atomicModifyIORef
which is quite pointless. So, the mutex must either be an argument to atomicModifyIORef
(see my example on discord) or wrapped up in IORef
itself, but that would probably slow down everything else.
tests/chez/chez003/IORef.idr
Outdated
t1 <- fork $ do | ||
atomicModifyIORef z (* 2) | ||
t2 <- fork $ | ||
atomicModifyIORef z (* 2) |
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.
In order to proof atomicity, the two competing threads must be long running (hundreds of milliseconds) and both access the mutable reference a lot (thousands or better millions of accesses). Preferably, one is considerably slower than the other. Only then will you create enough contention to truly show atomicity. Again, see my example on discord.
In this example, creating and acquiring the mutex (which you currently do on every call to atomicModifyIORef
) is so slow (mutexes are slow) compared to the simple update action, that you will almost always get the illusion of atomicity. Try running your version of atomicModifyIORef
with the example code I gave on discord and you will see that it is not atomic at all.
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.
Ah I see, the code I added in chez003
never actually created any true contention thus never tested the atomicity of atomicModifyIORef
. I added some of the code from discord you provided for chez003
, thank you for providing this, and thanks for helping me see/understand this!
One more thought on this: I think in your implementation of Again, there are several possibilities to approach this if the current version does not work:
|
…client decide this. Also update test to ensure enough contention to test for true atomicity (thanks to @stefan-hoeck for help with both of these).
Thank you for the feedback and the possible solutions. I have decided to just let the client make the decision of when to utilize Might be cool to add some dummy primitives for the javascript backend in the future though. |
Co-authored-by: G. Allais <guillaume.allais@ens-lyon.org>
…dating tests/chez/chez003/IORef.idr to reflect new function.
Not sure what to do about the failing checks (pretty much the same issue for all of them):
|
RE failing checks, it may just be that GitHub broke home-directory resolution in their paths for artifacts. I am testing this theory out now. [EDIT] I was wrong. But I did spot a suspicious looking "include hidden files: false" setting that might be causing this problem. |
Thank you for looking into this! |
Testing the hidden file theory now. Very promising given the recency of actions/upload-artifact#602. [EDIT] Looks good. Fix is up for PR. |
You're good to merge |
Sounds good, just merged main in! |
writeIORef1 : HasLinearIO io => IORef a -> (1 val : a) -> io () | ||
writeIORef1 (MkRef m) val = primIO1 (prim__writeIORef m val) |
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.
Unrelated but this seems unsafe: by writing a linear value into a reference, we gain the ability to freely duplicate it!
This PR adds a new function,
atomically
toData.IORef
(only for thechez
backend).This function atomically runs its argument according to the provided mutex.
It can for instance be used to modify the contents of an IORef
ref
with a functionf
in a safe way in a multithreaded program by usingatomically lock (modifyIORef ref f)
provided that other threads also rely on the samelock
to modifyref
.Credit to @stefan-hoeck for providing code to test this addition in
chez003.
Thanks to @gallais for the generalized
atomically
implementation.