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

cometindex: speedup by committing event changes in batches of 1000 #4854

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

cronokirby
Copy link
Contributor

Instead of creating one transaction for each event we need to index, we instead only close this transaction every 1000 events (or when when we've caught up to the database).

This gives about a 5x performance in catch up speed.

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    indexing only

Instead of creating one transaction for each event we need to index, we
instead only close this transaction every 1000 events (or when when
we've caught up to the database).

This gives about a 5x performance in catch up speed.
@@ -179,7 +180,6 @@ impl Indexer {
relevant_events += 1;

// Otherwise we have something to process. Make a dbtx
Copy link
Member

Choose a reason for hiding this comment

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

nit: comment is now out of date

dbtx.commit().await?;
// Only commit in batches of <= 1000 events, for about a 5x performance increase when
// catching up.
if scanned_events % 1000 == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

it seems possible to miss this if the while loop short circuits from the check at line 172, resulting in more than 1000 events being committed at once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to using the relevant_events as the metric, so that you commit every time you've processed 1000 relevant events, avoiding this issue.

@conorsch
Copy link
Contributor

Requested changes have been made, so I'm approving and merging.

@conorsch conorsch merged commit ae4ed05 into main Sep 17, 2024
13 checks passed
@conorsch conorsch deleted the cometindex-catchup-mode branch September 17, 2024 17:45
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