293-offload-bloom-filters #301
Conversation
|
|
||
| pub(crate) async fn filter_memory_allocated(&self) -> usize { | ||
| let mut memory = 0; | ||
| for holder in self.groups.read().await.iter() { |
There was a problem hiding this comment.
Maybe it's better to use FuturesUnordered here, so requests will be performed in parallel?
There was a problem hiding this comment.
And looks like it's group, not holder
bob-backend/src/pearl/group.rs
Outdated
|
|
||
| pub(crate) async fn filter_memory_allocated(&self) -> usize { | ||
| let mut memory = 0; | ||
| for holder in self.holders().read().await.iter() { |
There was a problem hiding this comment.
Same, FuturesUnordered
bob-backend/src/pearl/stuff.rs
Outdated
| ) -> Vec<(SimpleHolder, usize)> { | ||
| let mut res = vec![]; | ||
| for dc in iter { | ||
| for group in dc.groups().read().await.iter() { |
There was a problem hiding this comment.
Again, this cycle's iterations don't seem to be related to each other, so you can process them in parallel
bob-backend/src/pearl/core.rs
Outdated
| @@ -196,4 +203,27 @@ impl BackendStorage for Pearl { | |||
| dc.close_unneeded_active_blobs(soft, hard).await; | |||
| } | |||
| } | |||
|
|
|||
| async fn offload_old_filters(&self, limit: usize) { | |||
| Stuff::offload_old_filters( | |||
There was a problem hiding this comment.
Stuff is a bad name. You should give a more descriptive name
There was a problem hiding this comment.
It's not my naming) I can rename it to Utils
There was a problem hiding this comment.
Utils is a little bit better, but still a bad name. I don't see the whole picture, so maybe @pyakushin or @idruzhitskiy can suggest a name
| memory | ||
| } | ||
|
|
||
| pub(crate) fn groups(&self) -> Arc<RwLock<Vec<Group>>> { |
There was a problem hiding this comment.
Do this need to be exposed as a public?
There was a problem hiding this comment.
It make it visible only in current crate. Needed for stuff.rs
| size_before.saturating_sub(size_after) | ||
| ); | ||
| self.allocated_size | ||
| .fetch_sub(size_before.saturating_sub(size_after), Ordering::Relaxed); |
There was a problem hiding this comment.
Could this just be self.allocated_size = size_after? What is the reason for this math?
There was a problem hiding this comment.
Sorry, but my suggestion is incorrect
bob-backend/src/pearl/stuff.rs
Outdated
| async fn collect_holders( | ||
| iter: impl IntoIterator<Item = &Arc<DiskController>>, | ||
| ) -> Vec<(SimpleHolder, usize)> { | ||
| iter.into_iter() |
There was a problem hiding this comment.
This approach violates SOLID principle. It is better to expose holder_iter() function on each level, that provides iterator on holders and uses holder_iter() on level below. With such approach, here you can simply call top level holder_iter() and convert its result into SimpleHolder
bob-backend/src/pearl/hooks.rs
Outdated
| size_after, | ||
| size_before.saturating_sub(size_after) | ||
| ); | ||
| self.allocated_size.store(size_after, Ordering::Relaxed); |
There was a problem hiding this comment.
I reread the code. My previous suggestion just to store the value is incorrect. size_after contains filter size of a single holder. You need to revert change and perform math: allocated_size = allocated_size - (size_before - size_after)
Closes #293