Some update for JSegcache #452
Conversation
* add support for hash table entry verification * this is used to support opportunistic concurrency control with multiple readers and a single writer This PR allows us to use multiple readers each reader reads the cache without coordination, after copying/using the value, we verify hash table entry, if the hash table entry is updated, it means the object is evicted/updated, we may have read corrupt or stale value, and we should roll back.
…ache to avoid calling expire themselves
|
|
|
@JunchengYTwitter - this looks good overall. Just needs rustfmt. I'm okay with this remaining a single PR. |
|
updated |
| @@ -83,6 +89,99 @@ impl std::fmt::Debug for Item { | |||
| } | |||
| } | |||
|
|
|||
| /// Items are the base unit of data stored within the cache. | |||
| pub struct RichItem { | |||
There was a problem hiding this comment.
Is there a reason this can't be:
pub struct RichItem {
item: Item,
item_info: u64,
item_info_ptr: *const u64,
}
It seems like re-using the Item type and having the wrapper type contain the additional fields for "rich" functionality would be a better way to go here
There was a problem hiding this comment.
nope, your proposal is better, I will change it :)
| .field("cas", &self.cas()) | ||
| .field("raw", &self.raw) | ||
| .field("item_info", &self.item_info) | ||
| .field("item_info_ptr", &self.item_info_ptr) |
There was a problem hiding this comment.
Doesn't this print the ptr address? Is that really useful?
There was a problem hiding this comment.
yes, it is not useful, printing the value itself is not very useful as well. I can print all three (two value, one pointer) or we can remove the print. What do you think?
There was a problem hiding this comment.
It seems like printing if it's stale or not would maybe have value here?
src/storage/seg/src/hashtable/mod.rs
Outdated
| } else { | ||
| freq = (freq | 0x80) << FREQ_BIT_SHIFT; | ||
| } | ||
| // TODO: this needs to be atomic |
There was a problem hiding this comment.
I'm having trouble understanding why - we hold a mutable reference to the hashtable here - so there can be no other read or write references.
| // used to support multi readers and single writer | ||
| // return true, if the item is evicted/updated since being | ||
| // read from the hash table | ||
| pub fn is_not_changed(&self) -> bool { |
There was a problem hiding this comment.
i don't love the is_not_ here - can this just be is_current()?
This PR contains multiple things (let me know if you want to split them)
get_agein segments, then add a new fieldageinItemso that we can return age to caller.RichItemto returnitem_infoanditem_info_ptr, these two are used to support opportunistic concurrency control. Not sure adding a new struct is better than adding the fields toItem, but given there might be fields (like these) that we often do not need, creating a new struct might be better.expire.