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.

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

change(state): Set iterator read bounds where possible in DiskDb #7731

Merged
merged 18 commits into from
Oct 20, 2023
Merged

change(state): Set iterator read bounds where possible in DiskDb #7731

merged 18 commits into from
Oct 20, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Oct 11, 2023 •
edited by teor2345
Loading

Motivation

Unbounded RocksDB iterators are slow in column families with many delete tombstones.

This PR updates DiskDb to bound iterators when reading a range of keys.

Close #7664.

Solution

  • Set lower and upper bounds on rocksdb iterators where possible in DiskDb methods

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

  • Refactor address_utxo_locations()

@arya2 arya2 added C-bug Category: This is a bug P-High 🔥 C-security Category: Security issues I-slow Problems with performance or responsiveness A-state Area: State / database changes C-tech-debt Category: Code maintainability issues I-remote-trigger Remote nodes can make Zebra do something bad labels Oct 11, 2023
@arya2 arya2 self-assigned this Oct 11, 2023
@arya2 arya2 marked this pull request as ready for review October 11, 2023 23:51
@arya2 arya2 requested a review from a team as a code owner October 11, 2023 23:51
@arya2 arya2 requested review from oxarbitrage and removed request for a team October 11, 2023 23:51
teor2345

This comment was marked as resolved.

Sign in to view

@arya2 arya2 requested a review from teor2345 October 12, 2023 03:26
teor2345
teor2345 reviewed Oct 12, 2023
View reviewed changes
Copy link
Collaborator

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This looks great!

But there's one edge case we still need to cover: variable-length keys.

We don't have any at the moment, but PR #7392 changes the size of a key, and its validation runs an iterator with one key size on a column family which has been changed to a different key size.

@teor2345 teor2345 dismissed their stale review October 12, 2023 20:29

Calculation was fixed

@upbqdn upbqdn added the do-not-merge Tells Mergify not to merge this PR label Oct 13, 2023
@mergify mergify bot mentioned this pull request Oct 19, 2023
Closed
mergify bot added a commit that referenced this pull request Oct 19, 2023
@mergify
Merge of #7731
c5e4014
@mergify mergify bot mentioned this pull request Oct 19, 2023
Closed
@arya2 arya2 requested a review from teor2345 October 19, 2023 18:03
teor2345
teor2345 approved these changes Oct 19, 2023
View reviewed changes
Copy link
Collaborator

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks!

mergify bot added a commit that referenced this pull request Oct 19, 2023
@mergify
Merge of #7731
1cbeffa
@mergify mergify bot mentioned this pull request Oct 19, 2023
Closed
@mergify mergify bot merged commit 56e6305 into main Oct 20, 2023
105 checks passed
@mergify mergify bot deleted the read-bounds branch October 20, 2023 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

@oxarbitrage oxarbitrage Awaiting requested review from oxarbitrage oxarbitrage is a code owner automatically assigned from ZcashFoundation/general-rust-reviewers

Assignees

@arya2 arya2

Labels
A-state Area: State / database changes C-bug Category: This is a bug C-security Category: Security issues C-tech-debt Category: Code maintainability issues I-remote-trigger Remote nodes can make Zebra do something bad I-slow Problems with performance or responsiveness
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set iterator read bounds on deleting column families to avoid very slow syncs
4 participants
@arya2 @gustavovalverde @teor2345 @upbqdn