Add sleeping lock implementation #50878
Conversation
- Rename jl_mutex_t -> jl_spin_mutex_t - Rename jl_mutex_func->jl_spin_mutex_func
| if (safepoint) | ||
| gc_state = jl_gc_safe_enter(self->ptls); | ||
| uintptr_t hash = (uintptr_t)lock; | ||
| hash >>= SLEEP_IGNORED_BITS; |
There was a problem hiding this comment.
How safe is this? Do these hashes really not need to be unique?
There was a problem hiding this comment.
collisions just wake up the collision
There was a problem hiding this comment.
I would worry a bit about highly unpredictable performance phenomena due to this.
There was a problem hiding this comment.
There is a predictable number of threads, so even one sleep_lock would be fine, but more just helps avoid some percentage of spurious wakeups
|
What's the reason not to just use |
| if ((waiters & 1) == 0) { | ||
| // nobody else holds the lock, we can take it | ||
| // this just subtracts 1 from the waiters count and sets the bottom bit to 1 | ||
| jl_atomic_fetch_add(&lock->waiters, -1); |
There was a problem hiding this comment.
This appears to fail to atomically acquire the lock here (the comment is incorrect in the presence of threads)
There was a problem hiding this comment.
We are currently holding the corresponding sleep_lock, so while other threads may have incremented the waiter counts,
they can't currently acquire the lock.
@pchintalapudi correct me if I am wrong. The goal is to make it feasible to have cheaper locks, that are not spin-locks. Prem noticed in his work (see #49460) that the codegen lock was not ideal to be spin-lock since we can One downside is that these locks are still not scheduler aware and we might eventually need/want that. In #50880 we are for now using |
Takes the changes from #49460 and separates out the pure sleep lock implementation.
Long-term we may want to adopt WTF ParkingLot, but this get's us going in the right direction.