Read only cown #43
Read only cown #43
Conversation
vishalgupta97
commented
Jul 15, 2024
vishalgupta97
commented
- Adds support for running function at thread termination (4a9a3ce)
- Adds support for read-only cowns (0e1708e)
- Add a hash-table nano benchmark (22ba896)
| @@ -0,0 +1,26 @@ | |||
|
|
|||
There was a problem hiding this comment.
I would sooner this be done in CMake, so it is more cross platform.
| @@ -223,6 +225,7 @@ namespace verona::rt | |||
| } | |||
|
|
|||
| Systematic::finished_thread(); | |||
| run_at_thread_termination(); | |||
|
|
||
| using namespace verona::cpp; | ||
|
|
||
| long num_buckets = 1; |
There was a problem hiding this comment.
Could you add a high-level comment about what is being tested by this test.
| union | ||
| { | ||
| std::atomic<uint16_t> state; | ||
| struct | ||
| { | ||
| uint8_t blocked; | ||
| uint8_t next_slot_type; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Slightly concerned about the memory model on this. You have concurrent accesses at both points in the union type. Could you create a wrapper type that explicitly manages the union type. So everything is through the std::atomic.
| }; | ||
| }; | ||
|
|
||
| #define STATE_TRUE_NONE 0x0001 // blocked = True, next_slot_type = None |
There was a problem hiding this comment.
Prefer static constexpr to C preprocessor macros.
| Slot* get_next_slot() | ||
| { | ||
| return next_slot; | ||
| } |
There was a problem hiding this comment.
This is never called. You directly access next_slot.
| #define STATE_TRUE_NONE 0x0001 // blocked = True, next_slot_type = None | ||
| #define STATE_TRUE_READER 0x0101 // blocked = False, next_slot_type = Reader | ||
|
|
||
| Slot* next_slot; |
There was a problem hiding this comment.
I think next_slot is accessed by multiple threads, so this needs to be a std::atomic.
| @@ -78,7 +78,63 @@ namespace verona::rt | |||
| */ | |||
| std::atomic<uintptr_t> status; | |||
There was a problem hiding this comment.
You have added next_slot. That is what status was previously doing.
| return; | ||
| } | ||
| // If we failed, then the another thread is extending the chain | ||
| while (next_slot == nullptr) |
There was a problem hiding this comment.
Spinning on this state can be optimised to almost anything, as the state is non-atomic.
| auto prev = | ||
| cown->last_slot.exchange(last_slot, std::memory_order_acq_rel); | ||
| auto curr_slot = last_slot; | ||
| curr_slot->set_behaviour(first_body); |
There was a problem hiding this comment.
I don't think this is correct. It is pointing at the first behaviour in the chain, but if there are multiple atomically scheduled behaviours, then this looks like it will be quite messed up.
Add support for read-only cowns Add hash table nano benchmark Remove needless template parameter from Behaviour::make. (microsoft#41) * Remove needless template parameter from Behaviour::make. Signed-off-by: Alona Enraght-Moony <[email protected]> * Always use `Be` for behaviour template param (instead of `T`). Signed-off-by: Alona Enraght-Moony <[email protected]> --------- Signed-off-by: Alona Enraght-Moony <[email protected]> Co-authored-by: Matthew Parkinson <[email protected]>
vishalgupta97
force-pushed
the
read-only-cown
branch
from
420d63d
to
adda733
Compare
