feat: supports chains of StoreRevMut
#175
Conversation
firewood/src/storage/mod.rs
Outdated
| pub struct StoreRevMut { | ||
| base_space: Arc<dyn MemStoreR>, | ||
| deltas: Arc<RwLock<StoreRevMutDelta>>, | ||
| prev_deltas: Arc<StoreRevMutDelta>, |
There was a problem hiding this comment.
If you change this to Arc<RwLock<StoreRevMutDelta>> this means you won't have to clone anything, just increasing reference counts. You'll have to get a few more read locks, but I think that's a very small price to pay.
I think this would allow you to remove a lot of the clone implementations too.
| prev_deltas: Arc<StoreRevMutDelta>, | |
| prev_deltas: Arc<RwLock<StoreRevMutDelta>>, |
There was a problem hiding this comment.
Yeah, discussed this offline. The reason I did a clone is in take_delta StoreDelta actually takes the ownership of the pages and the child StoreRevMut will no longer have reference to it. I tried a bit to share the references to the pages and give up, but let me try again. :)
There was a problem hiding this comment.
Ok, after debugged together, @rkuris posted the commit to defer the clone during take_delta(). This has to be fixed as it is expensive to do clone in commit, but we are trying to push down the clone so that we can complete remove that later. A new issue will be created for follow up.
firewood/src/storage/mod.rs
Outdated
| Self { | ||
| base_space: other.base_space.clone(), | ||
| deltas: Default::default(), | ||
| prev_deltas: Arc::new(other.deltas.read().clone()), |
There was a problem hiding this comment.
This gets a lot easier with the type change above:
| prev_deltas: Arc::new(other.deltas.read().clone()), | |
| prev_deltas: other.deltas.clone(), |
| let wal_cfg = WalConfig::builder().build(); | ||
| let disk_requester = init_buffer(buf_cfg, wal_cfg); | ||
|
|
||
| // TODO: Run the test in a separate standalone directory for concurrency reasons |
There was a problem hiding this comment.
Looks like it's doing this already?
nit: I think we have a method somewhere that creates a directory under target, which means cargo clean will remove it automatically.
There was a problem hiding this comment.
Yes, you're right, I will remove the comment in a follow up PR.
There is still a bug to fix here in that the *guard assignment should not be needed; will attempt to fix that today.
|
|
||
| // TODO: Run the test in a separate standalone directory for concurrency reasons | ||
| let tmp_dir = TempDir::new("firewood").unwrap(); | ||
| let path = get_file_path(tmp_dir.path(), file!(), line!()); |
This commits extends current implementation of
StoreRevMutto support basing on top of anotherStoreRevMut, by chainingprev_deltaswith currentdeltasfrom itself. This would be an essential step forProposalsupport.