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

TransactionCost - hold transaction reference #3162

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

apfitzge
Copy link

Problem

  • TransactionCost allocates a vector to store writable account keys
  • This allocation is costly and unecessary if we just keep a reference to the transaction

Summary of Changes

  • TransactionCost holds a reference to the transaction

Fixes #

pub enum TransactionCost {
SimpleVote { writable_accounts: Vec<Pubkey> },
Transaction(UsageCostDetails),
pub enum TransactionCost<'a, Tx: SVMMessage> {
Copy link
Author

Choose a reason for hiding this comment

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

Was simpler for tests to just make this generic now as well.
See WritableKeysTransaction below, which is used in several tests and benches.

@apfitzge
Copy link
Author

apfitzge commented Oct 14, 2024

This change is not strictly better everywhere, but I believe overall the change is significantly better as it avoid an allocation which, in a real running validator, can be incredibly costly. However, it adds some overhead when we iterate over writable keys in cost-tracking.

I believe the following benches demonstrate that even in reasonably predictable allocation patterns the new approach will be better overall.

Bench on actual cost calculation:

  • expectation is that it will be improved because we are not allocating.
master:
test bench_cost_model                       ... bench:     685,593.22 ns/iter (+/- 6,067.51)
test bench_cost_model_requested_write_locks ... bench:     731,525.00 ns/iter (+/- 5,070.71)

branch:
test bench_cost_model                       ... bench:     476,283.30 ns/iter (+/- 23,550.96)
test bench_cost_model_requested_write_locks ... bench:     470,575.00 ns/iter (+/- 12,402.49)

Bench on cost_tracker:

  • expectation is slight reduction in speed due to overhead of AccountKeys iteration & is_writable filtering
master:
test bench_cost_tracker_contentious_transaction     ... bench:   1,605,051.05 ns/iter (+/- 12,555.84)
test bench_cost_tracker_non_contentious_transaction ... bench:   1,970,991.70 ns/iter (+/- 25,844.68)

branch:
test bench_cost_tracker_contentious_transaction     ... bench:   1,662,604.20 ns/iter (+/- 47,244.18)
test bench_cost_tracker_non_contentious_transaction ... bench:   2,040,983.30 ns/iter (+/- 25,184.97)

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.

1 participant