Skip to content
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

improve the performance of trimming shape run cache #298

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dzhou121
Copy link

When shape run cache was trimmed with a large keep_ages, it's expensive to scan the whole cache.

This PR is trying to save the cache keys to a Vec of age registries, and when doing trim, pop the Vec according to keep_ages and those are the keys that needed to be removed from the cache.

src/shape_run_cache.rs Outdated Show resolved Hide resolved
@jackpot51
Copy link
Member

Sorry for the delay in reviewing. I have one item I would like fixed.

jackpot51
jackpot51 previously approved these changes Sep 1, 2024
@jackpot51
Copy link
Member

Looks like Rust/build CI is failing

Comment on lines +75 to +67
// insert a new registry to the front of the Vec
// to keep keys for the current age
self.age_registries.insert(0, HashSet::default());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can potentially reuse a hashset popped earlier.

@dzhou121
Copy link
Author

dzhou121 commented Sep 5, 2024

Can you please re-run the CI?

src/shape_run_cache.rs Outdated Show resolved Hide resolved
@jackpot51 jackpot51 requested a review from UkoeHB September 6, 2024 13:31
@jackpot51
Copy link
Member

This looks good to me. Given that @UkoeHB also provided feedback, I'd like a re-review from them as well.

Comment on lines +43 to +50
self.age_registries[index].remove(key);

// update age
*age = self.age;
// register the key to the new age registry
if let Some(keys) = self.age_registries.first_mut() {
keys.insert(key.clone());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.age_registries[index].remove(key);
// update age
*age = self.age;
// register the key to the new age registry
if let Some(keys) = self.age_registries.first_mut() {
keys.insert(key.clone());
}
let prev_copy = self.age_registries[index].take(key);
// update age
*age = self.age;
// register the key to the new age registry
if let Some(keys) = self.age_registries.first_mut() {
// Note: This is only valid so long as the PartialEq impl on ShapeRunKey checks value
// equality.
keys.insert(prev_copy.expect("age_registries should have entry if cache has entry"));
}

Comment on lines +68 to +75
// and remove the keys from cache saved in the registries
while self.age_registries.len() as u64 > keep_ages {
if let Some(keys) = self.age_registries.pop() {
for key in keys {
self.cache.remove(&key);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// and remove the keys from cache saved in the registries
while self.age_registries.len() as u64 > keep_ages {
if let Some(keys) = self.age_registries.pop() {
for key in keys {
self.cache.remove(&key);
}
}
}
// and remove the keys from cache saved in the registries
let mut recovered_keys: Option<HashSet<ShapeRunKey>> = None;
while self.age_registries.len() as u64 > keep_ages {
if let Some(keys) = self.age_registries.pop() {
for key in keys.drain() {
self.cache.remove(&key);
}
if recovered_keys.is_none() {
recovered_keys = Some(keys);
}
}
}

// Increase age
self.age += 1;
// insert a new registry to the front of the Vec
// to keep keys for the current age
self.age_registries.insert(0, HashSet::default());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.age_registries.insert(0, HashSet::default());
self.age_registries.insert(0, recovered_keys.unwrap_or_default());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants