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

ResolvedTransactionView: array writable_cache #3775

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

Conversation

apfitzge
Copy link

Problem

  • ResolvedTransactionView uses Vec<bool> for storing the cached is_writable
  • This allocates for every transaction

Summary of Changes

  • Remove allocation (in simplest way possible) using statically sized (256) bool array

Fixes #

@apfitzge apfitzge self-assigned this Nov 25, 2024
@apfitzge apfitzge marked this pull request as ready for review November 25, 2024 15:36
@@ -30,7 +30,7 @@ pub struct ResolvedTransactionView<D: TransactionData> {
/// The resolved address lookups.
resolved_addresses: Option<LoadedAddresses>,
/// A cache for whether an address is writable.
writable_cache: Vec<bool>, // TODO: should this be a vec, bitset, or array[256].
writable_cache: [bool; 256],
Copy link
Author

Choose a reason for hiding this comment

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

plan to benchmark different approaches here;

  • bool array
  • bitset

however, just doing the simplest approach to remove the allocation-per-transaction for now, while getting code to use new tx-type into master. Easier to evaluate and compare approaches then once the allocation is gone.

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