-
Notifications
You must be signed in to change notification settings - Fork 525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(state_table): pre-compute column id mapping #3201
Conversation
Signed-off-by: Alex Chi <iskyzh@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #3201 +/- ##
==========================================
- Coverage 73.56% 73.55% -0.01%
==========================================
Files 745 745
Lines 102223 102242 +19
==========================================
+ Hits 75199 75208 +9
- Misses 27024 27034 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@@ -88,7 +116,7 @@ impl CellBasedRowDeserializer { | |||
pub fn take(&mut self) -> Option<(Vec<u8>, Row)> { | |||
let cur_pk_bytes = self.pk_bytes.take(); | |||
cur_pk_bytes.map(|bytes| { | |||
let ret = self.data.iter_mut().map(Option::take).collect::<Vec<_>>(); | |||
let ret = std::mem::replace(&mut self.data, vec![None; self.columns.len()]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this lead to extra allocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe no. Previously, .collect_vec()
will lead to one allocation. Now only vec![None; self.columns.len()]
has one allocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense.
#[allow(dead_code)] | ||
pub async fn get<'a, 'b>(&'a mut self, key: &'b K) -> Option<&'a HashValueType<S>> { | ||
let state = self.inner.get(key); | ||
// TODO: we should probably implement a entry function for `LruCache` | ||
match state { | ||
Some(_) => self.inner.get(key), | ||
None => { | ||
let remote_state = self.fetch_cached_state(key).await.unwrap(); | ||
remote_state.map(|rv| { | ||
self.inner.put(key.clone(), rv); | ||
self.inner.get(key).unwrap() | ||
}) | ||
} | ||
} | ||
} | ||
// #[allow(dead_code)] | ||
// pub async fn get<'a, 'b>(&'a mut self, key: &'b K) -> Option<&'a HashValueType<S>> { | ||
// let state = self.inner.get(key); | ||
// // TODO: we should probably implement a entry function for `LruCache` | ||
// match state { | ||
// Some(_) => self.inner.get(key), | ||
// None => { | ||
// let remote_state = self.fetch_cached_state(key).await.unwrap(); | ||
// remote_state.map(|rv| { | ||
// self.inner.put(key.clone(), rv); | ||
// self.inner.get(key).unwrap() | ||
// }) | ||
// } | ||
// } | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not used. And it triggers Rust compiler's false negative very often. So I commented it out. Anyone needs this can simply revert the comment.
Signed-off-by: Alex Chi iskyzh@gmail.com
What's changed and what's your intention?
As title
Checklist
./risedev check
(or alias,./risedev c
)Refer to a related PR or issue link (optional)