Add blocking #32
Add blocking #32
Conversation
polytypic
force-pushed
the
add-blocking
branch
5 times, most recently
from
9e6ea54
to
8ba2122
Compare
polytypic
force-pushed
the
add-blocking
branch
6 times, most recently
from
00fe8f8
to
1d03f71
Compare
polytypic
force-pushed
the
main
branch
2 times, most recently
from
1f32020
to
790267c
Compare
polytypic
force-pushed
the
add-blocking
branch
4 times, most recently
from
c2c780c
to
3d8521b
Compare
polytypic
force-pushed
the
add-blocking
branch
8 times, most recently
from
3e9287e
to
0dce076
Compare
polytypic
force-pushed
the
add-blocking
branch
6 times, most recently
from
9c4951f
to
a13b8d7
Compare
|
|
| @@ -1504,7 +1602,7 @@ experiment where we abort the transaction in case we observe that the values of | |||
|
|
|||
| ```ocaml | |||
| # with_updater @@ fun () -> | |||
| for _ = 1 to 10_000 do | |||
| for _ = 1 to 100_000 do | |||
There was a problem hiding this comment.
This is inherently non-deterministic and I noticed that it occasionally failed on CI. I haven't noticed failures after increasing the number of attempts, but it can of course still happen.
| *) | ||
| (**) | ||
| let fenceless_get = Atomic.get | ||
| let fenceless_set = Atomic.set |
There was a problem hiding this comment.
I have another PR #46 that changes these aliases to actually perform fenceless operations. Having these as fenceless should be safe (because the fences would be redundant), seems to significantly improve performance on ARM (Apple M1), and is also completely internal to the library, so I personally feel that we should just make the optimization.
| state : 'a state; | ||
| lt : cass; | ||
| gt : cass; | ||
| mutable awaiters : awaiter list; |
There was a problem hiding this comment.
This is mutable so that during the determine phase the awaiters can be efficiently copied from the updated locations to be resumed during the release phase.
| awaiter | ||
| then add_awaiters awaiter casn gt | ||
| else stop | ||
| | CASN _ as stop -> stop) |
There was a problem hiding this comment.
add_awaiters signals that some location had already been updated by returning the CASN _ descriptor of that location. This way the subsequent call of remove_awaiters (to prevent space leaks) can stop early at that same descriptor.
| @@ -0,0 +1,57 @@ | |||
| open Kcas | |||
|
|
|||
| type 'a internal = 'a Magic_option.t Loc.t | |||
There was a problem hiding this comment.
The Magic_option avoids a level of indirection. I decided to go with this optimization to reduce space usage and because promise is already using some other OCaml magic (as does the Eio Promise implementation).
polytypic
force-pushed
the
add-blocking
branch
2 times, most recently
from
d1d972b
to
25c9d9e
Compare
There was a problem hiding this comment.
For this review, I :
- read and tested the examples in README
- read documentation
- had a look at the added code but I can not say I fully understand
kcasalgorithm - made some tests/tries on my own
- had a quick look at
kcas_datachanges
What I did not do :
- try the changes in
kcas_datadata structures - try/read
Kcas_data.Promiseimplementation
Overall, there is not a lot to say except documentation is (as usual) great and easy to read and obviously, this seems like a great addition to kcas and kcas_data !
src/kcas.mli
Outdated
| (** Exception that may be raised to signal that the operation should be | ||
| retried, at some point in the future, after the examined shared memory | ||
| location or locations have changed. | ||
|
|
||
| {b NOTE}: It is important to understand that "{i after}" may effectively | ||
| mean "{i immediately}", because it may be the case that the examined | ||
| shared memory locations have already changed. *) | ||
|
|
||
| val later : unit -> 'a | ||
| (** [later ()] is equivalent to [raise Later]. *) |
There was a problem hiding this comment.
Maybe give more information about where this exception is caught (at least a pointer to the corresponding functions ?) ?
There was a problem hiding this comment.
Yes.
BTW, it could be a nice addition to odoc to automatically attach a list of references to items. The list of operations like get_as, update, and commit that refer to Later would then be seen directly when looking at Later.
src/kcas.mli
Outdated
| (** In [lock_free] mode the algorithm makes sure that at least one domain will | ||
| be able to make progress by performing read-only operations as read-write | ||
| operations. *) |
There was a problem hiding this comment.
Which is a more expensive operation, right ? It may be good to emphasize this with something like
at least one domain will be able to make progress at the cost of performing read-only operations as read-write operations.
There was a problem hiding this comment.
I will adjust the text. The cost is actually non-trivial, but generally speaking mutating locations is more expensive unless avoiding the mutation leads to starvation, which is kind of the difference between the modes. (The commit mechanism is designed to avoid starvation due to the obstruction-free mode by switching to the lock-free mode in case interference is detected.)
README.md
Outdated
| let x = | ||
| x | ||
| |> Loc.get_as @@ fun x -> | ||
| Retry.unless (x <> 0); | ||
| x |
There was a problem hiding this comment.
Not convinced this is more readable than
let x = Loc.get_as (fun x -> Retry.unless (x<>0); x) x
There was a problem hiding this comment.
Hmm... I guess it is better to avoid unnecessary cute uses of operators in examples.
| val to_blocking : xt:'x t -> (xt:'x t -> 'a option) -> 'a | ||
| (** [to_blocking ~xt tx] converts the non-blocking transaction [tx] to a | ||
| blocking transaction by retrying on [None]. *) | ||
|
|
||
| val to_nonblocking : xt:'x t -> (xt:'x t -> 'a) -> 'a option | ||
| (** [to_nonblocking ~xt tx] converts the blocking transaction [tx] to a | ||
| non-blocking transaction by returning [None] on retry. *) |
| val attempt : ?mode:Mode.t -> 'a tx -> 'a | ||
| (** [attempt tx] attempts to atomically perform the transaction over shared | ||
| memory locations recorded by calling [tx] with a fresh explicit | ||
| transaction log. If used in {!Mode.obstruction_free} may raise | ||
| {!Mode.Interference}. Otherwise either raises [Exit] on failure to commit | ||
| the transaction or returns the result of the transaction. The default for | ||
| [attempt] is {!Mode.lock_free}. *) |
There was a problem hiding this comment.
Why remove this function ? Is it because commit can be aborted by an exception ?
There was a problem hiding this comment.
Good question! I usually try to comment on changes like this, but I apparently missed that.
I decided to remove attempt for two reasons:
-
attemptcan not implement the blocking mechanism by itself. Instead it would potentially raise theLaterexception for the user to handle and then it would mean that there should probably be some additional support to be able to await for changes to multiple locations. -
I initially provided
attemptas a means for users to write their own version ofcommit(e.g. with additions like timeouts). However, one should be able to achieve pretty much everything viacommitalready (including timeouts — e.g. setup a location that is written at timeout) and I don't see much use cases forattempt— I never used it myself except in tests.
So, rather than add even more (potentially unused) things to the API, I decided that it is better to just remove attempt. Something like it can be later added back if there are real use cases for it.
There was a problem hiding this comment.
I am interested to see how you add a timeout (this also seems like it could be a good example).
| let modify ?backoff loc f = update ?backoff loc f |> ignore [@@inline] | ||
| match f before with | ||
| | after -> | ||
| if before == after then before |
There was a problem hiding this comment.
resume_awaiters is not called here because the value has not changed, right ?
There was a problem hiding this comment.
Yes, this is an optimization to avoid unnecessarily updating locations. It wasn't implemented before, but with the addition of awaiters it potentially avoids waking up awaiters unnecessarily (as nothing logically changed), which could avoid a lot of unnecessary computation.
src/kcas.ml
Outdated
| resume_awaiters before state'.awaiters | ||
| else update_no_alloc await (Backoff.once backoff) loc state f | ||
| | exception Retry.Later -> | ||
| let state = new_state (Obj.magic ()) in |
There was a problem hiding this comment.
I am not extremely familiar with the use of Obj.magic but why not use before here instead ?
There was a problem hiding this comment.
Hmm... Both state.before and state.after will be overwritten, so using before would work here also. Sure, why not.
Thanks for the review! My plan now is to move the As has been discussed elsewhere, the idea with However, in the future we may end up using some different means to provide blocking support. This is not a problem for me and should not be a problem in general and we can then change kcas to use such a future mechanism when it is available. At that point we can deprecate It should also be mentioned that |
|
Sounds good to me.
The 1% will be the implementations of concurrency libraries such as Eio and Domainslib, and any blocking synchronization structures such as promises, kcas, rendezvous channels, etc. That said, by going ahead with the proposed interface for DLA, I'm hoping that we will gain experience by doing it rather than trying to come up with the perfect scheme now. |
This PR adds blocking support to kcas and basically turns kcas into a proper software transactional memory (STM) implementation.
For blocking this PR introduces an experimental domain local await mechanism. It is a first-order interface inspired by proposed "rendezvous" mechanism in a lockfree PR with support for cancellation. Ultimately some sort of unified interface for suspend functionality should be provided by some other library, possibly the
Stdlib.For the blocking support this PR also introduces a dedicated
Retry.Laterexception for retrying transactions and higher-order single location updates. Previously theStdlib.Exitexception was used for that purpose, but I feel that having a dedicated exception makes the intent clearer and freesStdlib.Exitfor other purposes such as aborting a transaction.This PR adds a
Promisemodule based on thePromisemodule of Eio to the kcas_data package. I also drafted some other Eio style primitives, but decided to move the development of those to another PR.Perhaps a good place to start review is to read the couple of sections added to the README:
The blocking support works by adding a list of awaiters to locations. When a location is modified, the awaiters are resumed. This adds a bit of overhead to all operations as locations take an extra word of memory and those words also need to be accessed on every write operation. Based on the benchmarks the overhead seems to result in the previous implementation being roughly about 1.05x faster (in some cases the overhead is less and in some cases more). After a blocking operation is resumed, the implementation eagerly removes any awaiters it attached to locations to avoid space leaks.
One internal change made in this PR is that uses of
Atomic.getandAtomic.setoperations that do not need use fences for correctness are made to use functionsfenceless_getandfenceless_set, which are just aliases forAtomic.getandAtomic.set. I have another PR #46 that changes those to actually use fenceless operations and introduces some additional optimizations.This PR also changes the description of the library to reflect the new capabilities by calling kcas "Software transactional memory based on lock-free multi-word compare-and-set". I believe multi-word compare-and-set is less familiar technical jargon to potential users.