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

PrioGraphScheduler::try_schedule_transaction remove TransactionAccountLocks allocation #1760

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

apfitzge
Copy link

Problem

  • get_account_locks_unchecked does some allocations for locks
  • try_lock_accounts takes arbitrary iterators (clone-able) so we do not need to allocate

Summary of Changes

  • Make AccountKeys::key_segment_iter return clonable iterator. (No change to impl, just exposes clone)
  • Make AccountKeys::iter return clonable iterator. (No change to impl, just exposes clone)
  • Iterate over write/read accounts, do not allocate.

Fixes #

@apfitzge
Copy link
Author

apfitzge commented Jun 17, 2024

Small part of larger effort to reduce allocations. Don't expect significant impact from this alone.

Banking Bench
master:
{'name': 'banking_bench_total', 'median': '51532.09'}
{'name': 'banking_bench_tx_total', 'median': '53217.47'}
{'name': 'banking_bench_success_tx_total', 'median': '50988.14'}

with change:
{'name': 'banking_bench_total', 'median': '51722.87'}
{'name': 'banking_bench_tx_total', 'median': '53396.77'}
{'name': 'banking_bench_success_tx_total', 'median': '51339.26'}
bench-tps
master:
[2024-06-17T14:04:13.835628171Z INFO  solana_bench_tps::bench] 
    Average max TPS: 58954.28, 0 nodes had 0 TPS
[2024-06-17T14:04:13.835631979Z INFO  solana_bench_tps::bench] 
    Highest TPS: 58954.28 sampling period 1s max transactions: 3346725 clients: 1 drop rate: 0.58
[2024-06-17T14:04:13.835635625Z INFO  solana_bench_tps::bench] 	Average TPS: 36731.09

with change:
[2024-06-17T13:59:15.938553191Z INFO  solana_bench_tps::bench] 
    Average max TPS: 61463.46, 0 nodes had 0 TPS
[2024-06-17T13:59:15.938564112Z INFO  solana_bench_tps::bench] 
    Highest TPS: 61463.46 sampling period 1s max transactions: 3123034 clients: 1 drop rate: 0.62
[2024-06-17T13:59:15.938578429Z INFO  solana_bench_tps::bench] 	Average TPS: 34279.95
image

left is with change, right without.

@apfitzge apfitzge force-pushed the scheduler_account_locks_2 branch from 39d43e6 to 96866fe Compare June 17, 2024 18:54
@apfitzge apfitzge marked this pull request as ready for review June 20, 2024 11:52
@apfitzge apfitzge requested review from ryoqun and t-nelson June 20, 2024 11:52
@apfitzge
Copy link
Author

@t-nelson - want to make sure I'm not an idiot with the SDK changes.

As far as I know, this is not a breaking change since I am only adding an additional trait-bound on the return type. It's not actually changing the return type, just exposing to calling-code that it is also Clone-able.

@apfitzge apfitzge requested a review from tao-stones June 21, 2024 13:51
@ryoqun
Copy link
Member

ryoqun commented Jun 21, 2024

As far as I know, this is not a breaking change since I am only adding an additional trait-bound on the return type. It's not actually changing the return type, just exposing to calling-code that it is also Clone-able.

(yeah, i think this is correct)

@ryoqun
Copy link
Member

ryoqun commented Jun 21, 2024

(yeah, i think this is correct)

oh, one little caveat is that we're now committed to returning cloneable iter thereafter. but, i don't think this will be a significant burden.

Copy link
Member

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

lgtm

@apfitzge apfitzge merged commit 8f8f87b into anza-xyz:master Jul 3, 2024
50 checks passed
@apfitzge apfitzge deleted the scheduler_account_locks_2 branch July 3, 2024 15:48
samkim-crypto pushed a commit to samkim-crypto/agave that referenced this pull request Jul 31, 2024
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.

2 participants