Skip to content

CHIA-1808 Speedup requesting removals by using a cache #18895

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

CHIA-1808 Speedup requesting removals by using a cache #18895

wants to merge 1 commit into from

Conversation

AmineKhaldi
Copy link
Contributor

Purpose:

Keep a LRU cache of removals for heights, to speedup future requests for those removals.

Current Behavior:

We always fetch these removals from the DB.

New Behavior:

We keep a LRU cache of the removals we already fetched.

@AmineKhaldi AmineKhaldi added Added Required label for PR that categorizes merge commit message as "Added" for changelog Cleanup Code cleanup labels Nov 19, 2024
@AmineKhaldi AmineKhaldi self-assigned this Nov 19, 2024
@AmineKhaldi AmineKhaldi marked this pull request as ready for review November 20, 2024 10:32
@AmineKhaldi AmineKhaldi requested a review from a team as a code owner November 20, 2024 10:32
@arvidn
Copy link
Contributor

arvidn commented Nov 20, 2024

Do you have a reason to believe the cache hit rate would be high in this cache?
I can't think of a reason why it would be

@AmineKhaldi
Copy link
Contributor Author

Do you have a reason to believe the cache hit rate would be high in this cache? I can't think of a reason why it would be

Examining the direct and indirect paths this function is involved in (along with get_coins_added_at_height) gave me a reason to believe so. Did you examine those paths and come to the opposite conclusion?

arvidn
arvidn requested changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

I don't think we should add a cache, using more memory, without any measurements or rationale that it's an improvement

@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arvidn arvidn arvidn requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@AmineKhaldi AmineKhaldi

Labels
Added Required label for PR that categorizes merge commit message as "Added" for changelog Cleanup Code cleanup merge_conflict Branch has conflicts that prevent merge to main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AmineKhaldi @arvidn