Storage Rewrite #2791
Storage Rewrite #2791
Conversation
There was a problem hiding this comment.
It feels like registry.rs is mostly about defining a type that behaves mostly like a Vec but never has to relocate its buffer, even as new elements are added. It has nothing to do with Id or epochs - those are the element type's concern. The question of how to ensure that concurrent modification is safe could be left to a higher-level module, with this new Vec-like type, with a safe API, simply serving to manage the backing storage.
| enum ElementStatus { | ||
| Vacant, | ||
| Occupied(Epoch), | ||
| Error(Epoch, Cow<'static, str>), |
There was a problem hiding this comment.
This variant is going to make ElementStatus six words long, when ideally it'd be two. Consider boxing.
|
|
||
| pub struct Storage<T> { | ||
| blocks: [UnsafeCell<Option<Box<StorageBlock<STORAGE_BLOCK_SIZE, T>>>>; 256], | ||
| max_index: AtomicU32, |
There was a problem hiding this comment.
This is actually one greater than the maximum index. Suggest index_limit or perhaps indices_allocated?
| const STORAGE_BLOCK_SIZE: usize = 32; | ||
|
|
||
| pub struct Storage<T> { | ||
| blocks: [UnsafeCell<Option<Box<StorageBlock<STORAGE_BLOCK_SIZE, T>>>>; 256], |
There was a problem hiding this comment.
You could also make this a [AtomicPtr<StorageBlock<T>>], and then use compare_exchange to put in new blocks. Atomic loads for indexing are ordinary loads on x86_64, and I don't think they're that bad on ARM.
256 should be a const too, shouldn't it?
There was a problem hiding this comment.
That would work, would be safer and would be easy to change back if it's an issue in arm.
Yeah both the values need to be constants and need to be different based on the resource. (we need to deal with 100,000 textures, but maybe not 100,000 devices)
There was a problem hiding this comment.
Great!
Somehow Storage feels a lot like ArrayVec<T, STORAGE_BLOCK_SIZE> for some appropriately fancy T. So maybe we can side-step this UnsafeCell somehow as well. Letting the original Storage's elements be enums wasn't a major perf problem, right?
There was a problem hiding this comment.
I don't think we can use ArrayVec directly because it needs a &mut to add an element and we need to add elements on &mut
As for the enum, the reason the storage isn't online is because the enum is going to become a fancy atomic at some point so we wouldn't need the lock at all. Even if we kept it as a lock, it's much easier to avoid UB nonsense where we don't need to go through a mutex to get unlocked access to something.
Additionally, I highly anticipate other things being stored SOA style in the future in the storage.
|
Just for posterity: this is still on my todolist, though obviously very out of date. I have some new ideas on how to design the datastructure to be more clearly correct and easier for ff and friends to trust, there are just currently higher priorities. |
|
For the reasons discussed here this PR can't land as is. |
Connections
Closes #2710
Description
This completely reworks how we do storage to be completely lock-free on the hot paths. There is still some work to do but I wanted to get a draft PR out.
Testing
Tests.