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

account_saver: collect SanitizedTransaction references #2820

Merged

Conversation

apfitzge
Copy link

@apfitzge apfitzge commented Sep 3, 2024

Problem

  • Geyser interface has not been updated to something that allows generic tx type (see agave-transaction-ffi #2125)
  • I made account_saver generic, but it should not actually be collecting the generic references until we have transitioned geyser.

Summary of Changes

  • Instead of a boolean indicating whether we should collect accounts. Use a vec of references.
  • This is less efficient than current until we change the transaction type, since we're doing an allocation
  • When using new transaction type, we will need to convert to SanitizedTransaction. For SanitizedTransaction we simply collect references

Fixes #

@apfitzge apfitzge changed the title account_saver_geyser_hack account_saver: collect SanitizedTransaction references Sep 3, 2024
@apfitzge apfitzge marked this pull request as ready for review September 3, 2024 17:57
@apfitzge apfitzge requested a review from brooksprumo September 3, 2024 17:57
pub fn collect_accounts_to_store<'a, T: SVMMessage>(
txs: &'a [T],
txs_refs: &'a Option<Vec<impl core::borrow::Borrow<SanitizedTransaction>>>,

Choose a reason for hiding this comment

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

nit: Can the Borrow type be imported in the top-of-file use statement?

More real though, should this be Borrow? Does a regular reference not work? Or AsRef instead?

Copy link
Author

Choose a reason for hiding this comment

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

It may be owned or borrowed. If we have SanitizedTransaction we need only copy references. If we have a ResolvedTransactionView we need to convert to an owned SanitizedTransaction.

Of course this just needs to exist until geyser has a sane interface, but since that's blocked for now and we dont want to wait on it, we're stuck with something similar to this.

If we wanted to eat the cost for simplicity, we could also just clone our SanitizedTransaction

Copy link
Author

Choose a reason for hiding this comment

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

'AsRef' unfortunately does not work, since it is not implemented for '&T'.

Comment on lines +3804 to +3805
// references in order to comply with that interface - until it
// is changed.

Choose a reason for hiding this comment

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

Who is owning changing the geyser interface? Should it be required that the interface is updated before releasing v2.1 so that the extra allocation doesn't actually get released to the wild?

How significant is this allocation in practice (assuming geyser is in use)?

Copy link
Author

Choose a reason for hiding this comment

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

I guess I am "owning" the actual changes of the geyser interface. But have not had luck in getting reviews of related code.

Copy link
Author

Choose a reason for hiding this comment

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

These per batch allocations should be minimal compared to the per ingested allocations of regular transactions. Jemalloc is relatively well suited for these regular allocations

@brooksprumo brooksprumo self-requested a review September 4, 2024 21:43
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

:shipit:

@apfitzge apfitzge merged commit 9a66594 into anza-xyz:master Sep 9, 2024
40 checks passed
@apfitzge apfitzge deleted the account_saver_sanitized_transaction_refs branch September 9, 2024 15:42
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 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