New AsyncMutex implementation
#3562
Conversation
| "preserve waiters order (fifo) on a non-race cancellation" in ticked { implicit ticker => | ||
| val numbers = List.range(1, 10) | ||
| val p = mutex.flatMap { m => | ||
| IO.ref(List.empty[Int]).flatMap { ref => | ||
| for { | ||
| f1 <- m.lock.allocated | ||
| (_, f1Release) = f1 | ||
| f2 <- m.lock.use_.start | ||
| _ <- IO.sleep(1.millis) | ||
| t <- numbers.parTraverse_ { i => | ||
| IO.sleep(i.millis) >> | ||
| m.lock.surround(ref.update(acc => i :: acc)) | ||
| }.start | ||
| _ <- IO.sleep(100.millis) | ||
| _ <- f2.cancel | ||
| _ <- f1Release | ||
| _ <- t.join | ||
| r <- ref.get | ||
| } yield r.reverse | ||
| } | ||
| } | ||
|
|
||
| p must completeAs(numbers) | ||
| } |
There was a problem hiding this comment.
|
@BalmungSan I've set this as "draft" due to your "do not merge" comment. I've also left another comment. |
|
As expected, this PR version of |
|
So, the "original" |
BalmungSan
changed the title
AsyncMutex implementationAsyncMutex implementation
|
I just updated the PR description with the benchmark results for current, previous (deferred) & new. And it seems the performance of all versions is very similar. Thus, I think the best is to just use the deferred one and remove the semaphore one. |
…nt cancels during release
|
Yeah this is super-duper close. It definitely feels like it's best to pick the simplest one with the most reasonable semantics. |
BalmungSan
force-pushed
the
revert-mutex-impl
branch
from
2ce85c3 to
3cb4833
Compare
|
Okay, so I reverted and unified both the Final version benchmark results
c.c. @djspiewak @armanbilge @durban |
I was half-following the recent discussions about the
Mutexissue and on an internal conversation with Arman, we both concluded that the current version ofAsyncMutexwas hard to follow, whereas the two previous versions were "easier" (it still requires a lot of comments and some mental force but still it is more straightforward IMHO). There is also the point that the previous versions always preserved fairness (FIFO semantics), contrary to current.Thus, considering that the main rationale for the implementation change was performance (especially on the happy path) I was curious and decided to re-run the benchmarks. Since the version Arman introduced on #3409 was heavily modified, I expected some changes.
For clarification, during the lifetime of this PR, we have had the following versions:
series/3.xMutex&AtomicCell#3346 (this version has been discarded since a new test shows it has a serious bug which would allow multiple acquires of theMutex)Mutex&AtomicCell#3346 (this version only requiresConcurrentso it can also replace semaphore implementation)Async)Right now we (Arman & Luis) propose to use either deferred or new, since both pass all the current tests (including the FIFO order one) as well as being easier to understand (in our humble opinion).
Benchmark results
Current
Previous / Deferred
New
For context, here are the results of the semaphore implementation for all runs.
Current
Previous / Deferred
New