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

perf(AllTransactions-iter): do not clone all transactions by default #13187

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

hai-rise
Copy link
Contributor

@hai-rise hai-rise commented Dec 6, 2024

We're spamming the mempool so hard it can take over 1s for TxPool::on_canonical_state_change to complete. This needs much improvement for chains with 200ms-1s block time.

Earlier profiling shows that ~10% is spent on cloning all transactions (via AllTransactions::transactions_iter) to count the transaction type in update_transaction_type_metrics.
image

And another 4% is spent on dropping the cloned Arcs.
image

We only need a reference for tx.transaction.tx_type(), so this PR makes transactions_iter return references by default and only clone on demand like in pooled_transactions.

@hai-rise hai-rise requested a review from mattsse as a code owner December 6, 2024 18:22
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice,

cool, will take a look what else we can optimize here

Comment on lines +1101 to +1102
) -> impl Iterator<Item = &Arc<ValidPoolTransaction<T>>> + '_ {
self.by_hash.values()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, makes sense

@mattsse mattsse added the C-perf A change motivated by improving speed, memory usage or disk footprint label Dec 6, 2024
@mattsse mattsse merged commit c608679 into paradigmxyz:main Dec 6, 2024
42 checks passed
@hai-rise hai-rise deleted the minimize-clone branch December 7, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-perf A change motivated by improving speed, memory usage or disk footprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants