Begin of atomic append facilities #10
Conversation
This is a very early draft. method names and semantic may change.
…pare_capacity()' method
|
so, I leave this now for review, probably for some weeks. This is still not completely finished, i will add a Whatever is finished is open for some bikeshedding about method names and whenever the atomic_append feature should be default or not. I don't have ARM hardware where it would make a measurable difference in performance. On x86 atomics are pretty free/nil and differences between with/without atomics are below the noise floor. On my side there is no urge to merge this yet. If this needs to be finished ping me. |
|
I see, so essentially |
|
added reserve/reserve_exact/shrink_to/shrink_to_fit, reworked the resize_insert fn for that. The resize that can reservation of multiple elements is vital for adding slices efficiently (and i need it for other things). I see that i find time to add the slice things next when i find time. |
* introduces a helper union to fix simplify alignment calculations no need for cmp and phantom data anymore * add simple testcase that triggered the miri issue * change end_ptr_atomic_mut(), using the rewritten start_ptr()
|
So, this concludes my work for now. Eventually other std::Vec mehtods/Traits (like Extend) could be added. Currently i have no urgent need for those. Integration into cowstr is kindof later in the queue i'll see if i miss some things then. |
|
ping --- any news on my PR, do you intend to merge it? |
|
Sorry, I didn't realize it was out of draft. I will take a look at it later today. Thanks for the ping. |
|
I haven't forgotten, but yesterday was a bit busier than expected. I should be able to take care of this tonight. |
|
On Wed, 18 Dec 2024 07:39:37 -0800 Geordon Worley ***@***.***> wrote:
I haven't forgotten, but yesterday was a bit busier than expected. I
should be able to take care of this tonight.
NP. i did the PR in hindight that I want to use it sometime next,
but there is no urge as i am currently busy with other things
too. just wanted to ping you since i suspected it overseen it.
… |
| // correct the len | ||
| let len_again = self.len_atomic_add_release(slice.len()); | ||
| // in debug builds we check for races, the chance to catch these are still pretty minimal | ||
| #[cfg(debug_assertions)] |
There was a problem hiding this comment.
#[cfg(debug_assertions)] can be removed
may need #[allow(unused_variable)]
|
Okay, today I will be getting to this. I will start reviewing now, but I will have to finish later today as I have something else. |
| /// Before incrementing the length of the vector, you must ensure that new elements are | ||
| /// properly initialized. | ||
| #[inline(always)] | ||
| unsafe fn len_atomic_add_release(&self, n: usize) -> usize { |
There was a problem hiding this comment.
I am a bit confused by this method. I added the line about what it returns, but the Safety section is the same. From what it says, it is necessary for new elements to be initialized properly prior to this length being updated. Due to this, the only correct usage scenario where the original length could be different than the one you expected it to be is if two threads somehow coordinated the allocation of two new items in the vector, added their items, hit a barrier that blocks both of them from continuing until the items were added, then each subsequently atomically adds to the length in each thread. This is because if two threads are adding items, they must both synchronize the process of adding items to the vector, and therefore must already pre-agree upon what the length will ultimately be and what slot each thread will allocate into prior to incrementing the length to uphold the guarantee that looking inside the vector always results in valid initialized values. In this case there is no need for this function to return the size. In fact, it would make more sense for this to return nothing at all and instead be passed in the old size as an argument, panicking in the case that the returned value isn't equal to the expected old length.
My understanding might be incomplete though, but I am having a hard time understanding how this API would be used currently since the person adding the length must already be absolutely sure that all the items were added by all threads prior to it incrementing the length. Basically, two threads cant increment the length until some kind of synchronization has occurred to ensure the values were already added by the other threads into unique slots in the vector. The most likely use case would probably just be that there are N observers, but only 1 owner that can actually add items to the vector. In that case, I would suggest that we create an API to reflect that reality.
Let me know if I am misunderstanding how this is used.
Since we always point to a valid allocation we can use NonNull here. This will benefit from the niche optimization: size_of::<HeaderVec<H,T>>() == size_of::<Option<HeaderVec<H,T>>> Also adds a test to verfiy that HeaderVec are always lean and niche optimized.
|
Note: I am not happy with the name |
this is WIP I am still fleshing out some things. posting this early that you can see my intentions. Everything is still in flux.
You proposed 2 types one for non atomic and one for atomic access. Actually I need to do this in one type because i have some hybrid access. A non shared Vec can be mutated with &mut self, a shared Vec can be appended by a single thread from &self. When a &mut self is not available then .len() doing Relaxed ordering. This would be racy vs another thread that appends to the Vec but always memory safe when the append follows the contracts (only once thread append, length must be set after appending data). Clone does acquire semantic, since clone is already a expensive operation the cost is negligible and its the correct way when the atomic append is used.
A push_atomic/push_atomic_slice comes next.
Note: no rush: i said i want to use this in cowstr but that transition is a bit down in my queue.