account_saver: Remove nested options #2724

apfitzge · 2024-08-23T19:17:20Z

Problem

Nested options here is confusing in the accounts_db code.
In reality, they are either all Some or the outer is None

Summary of Changes

Remove nested options
Get rid of unneccessary Boxes

Fixes #

apfitzge · 2024-08-23T19:20:38Z

accounts-db/src/accounts_db.rs

-        let (account_infos, cached_accounts) = txn_iter
-            .enumerate()
-            .map(|(i, txn)| {
+        let (account_infos, cached_accounts) = (0..accounts_and_meta_to_store.len())


if the txn_iterator is empty, we still want to iterate over the accounts to store.

Might make sense to just have some sort of iteration over accounts_and_meta_to_store?

That sounds nice to me

Hm sounded good in theory, but when I just took a stab it I ran into a problem immediately.
We implement StorableAccounts on various tuple types. Rust does not allow us to implement Iterator for arbitrary types that we didn't define.

So afaict we'd need to wrap these in some type or have an iter fn - which doesn't seem much better imo.

apfitzge · 2024-08-23T19:23:06Z

accounts-db/src/accounts_db.rs

-            } else {
-                Box::new(std::iter::empty())
-            };
+        let mut write_version_producer = if self.accounts_update_notifier.is_some() {


we either iterate over a range or have an empty. 0..0 should be equivalent, but match the Range type so we can avoid doing a Box allocation

How about just using a u64 to track the latest write version and increment it whenever we notify?

apfitzge · 2024-08-23T19:26:59Z

accounts-db/src/accounts_db.rs

+                        assert_eq!(transactions.len(), accounts.len());
+                        transactions.iter().copied()
+                    }
+                    None => [].iter().copied(),


behavior change here - instead of creating a box for different types of iterators, we either have an iterator over the txns vs an empty iterator.

At the next level down we just use None if the iterator has no next element.

How about we pass Option<&[&SanitizedTransaction]> directly?

This suggestion from jstarry makes sense to me.

apfitzge · 2024-08-26T15:26:20Z

@jstarry & @LucasSte as a follow-up to this I plan to move this module out of SVM.
This functionality, as far as I can tell, is not actually used inside the svm crate - it is really needed by our runtime.

The reason I'd like to move it is to make the transition to the new transaction type not depend on the geyser interface changing, since I am seeing push-back on that front.
Instead, an extension-trait of SVMTransaction will be provided to create a SanitizedTransaction from the abstracted tx type, which will then be fed to geyser.
That behavior really has no place being part of SVMTransaction imo, so this module living here makes the abstraction impossible.

Instead of this collecting whatever transaction-type we pass in, it will (until geyser transitions) return SanitizedTransaction regardless of input type.
Once geyser transitions it will instead return the transaction interface type.
Additionally, these transactions will only be collected if geyser is enabled - otherwise we do not want to waste time for the conversion.

jstarry · 2024-08-27T06:06:51Z

Wherever you move account_saver, I think account_loader should be moved as well I think. Wouldn't it be strange if account loading was part of the SVM but account saving was not?

jstarry

Implementation looks fine as is but had some suggestions

jstarry · 2024-08-27T08:57:29Z

accounts-db/src/accounts_db.rs

-            } else {
-                Box::new(std::iter::empty())
-            };
+        let mut write_version_producer = if self.accounts_update_notifier.is_some() {


How about just using a u64 to track the latest write version and increment it whenever we notify?

jstarry · 2024-08-27T08:58:22Z

accounts-db/src/accounts_db.rs

-        let (account_infos, cached_accounts) = txn_iter
-            .enumerate()
-            .map(|(i, txn)| {
+        let (account_infos, cached_accounts) = (0..accounts_and_meta_to_store.len())


That sounds nice to me

jstarry · 2024-08-27T08:59:21Z

accounts-db/src/accounts_db.rs

+                        assert_eq!(transactions.len(), accounts.len());
+                        transactions.iter().copied()
+                    }
+                    None => [].iter().copied(),


How about we pass Option<&[&SanitizedTransaction]> directly?

brooksprumo

Looks fine to me. The changes are mainly localized to if there's an account_update_notifier (aka geyser) plugin. The rest of the changes are refactors.

brooksprumo · 2024-08-27T12:56:17Z

accounts-db/src/accounts_db.rs

+                        assert_eq!(transactions.len(), accounts.len());
+                        transactions.iter().copied()
+                    }
+                    None => [].iter().copied(),


This suggestion from jstarry makes sense to me.

apfitzge · 2024-08-27T13:25:06Z

Wouldn't it be strange if account loading was part of the SVM but account saving was not?

I don't really think it'd be that strange - this is not actually where we "save" accounts anyways. We're just unrolling the TransactionProcessingResult into a more convenient form to then be saved (by accounts-db).

That said, I think I could probably work around the conversion issue I mentioned previously and just put the conversion trait in runtime - so we could leave this here.

Think @buffalojoec has some opinions on what should/should not be in SVM, so he may want to weigh in here.

jstarry

lgtm

jstarry · 2024-08-28T01:05:19Z

accounts-db/src/accounts_db.rs

-            } else {
-                Box::new(std::iter::empty())
-            };
+        let mut current_write_version = self


We used to only update the write_version atomic counter for if self.accounts_update_notifier.is_some(). I don't see anything wrong with updating it always. @brooksprumo any thoughts here?

That's just a mistake on my part - reverted: ae2284a

buffalojoec · 2024-08-28T07:22:30Z

@jstarry & @LucasSte as a follow-up to this I plan to move this module out of SVM.
This functionality, as far as I can tell, is not actually used inside the svm crate - it is really needed by our runtime.

You read my mind. I was going to look into this as well, but I didn't want to block anyone pushing bugfixes for now.

Wouldn't it be strange if account loading was part of the SVM but account saving was not?

I don't really think it'd be that strange - this is not actually where we "save" accounts anyways. We're just unrolling the TransactionProcessingResult into a more convenient form to then be saved (by accounts-db).

That said, I think I could probably work around the conversion issue I mentioned previously and just put the conversion trait in runtime - so we could leave this here.

Think @buffalojoec has some opinions on what should/should not be in SVM, so he may want to weigh in here.

I'm fully on board with this approach. I just did something similar with rent evaluation/collection in #2753 and may also do something similar with other pieces like fees. Are you going to take it on for account saver @apfitzge ? If not I can do it!

apfitzge added 2 commits August 23, 2024 14:10

Remove nested options

5861430

remove box

a3b52fe

apfitzge commented Aug 23, 2024

View reviewed changes

Option<Vec>

2ba61d0

apfitzge marked this pull request as ready for review August 26, 2024 15:27

apfitzge requested review from jstarry and brooksprumo August 26, 2024 15:27

jstarry previously approved these changes Aug 27, 2024

View reviewed changes

brooksprumo previously approved these changes Aug 27, 2024

View reviewed changes

apfitzge added 2 commits August 27, 2024 08:10

use u64 to track write-version, manual iteration

2557ee5

pass option slice directly

de664e8

apfitzge dismissed stale reviews from brooksprumo and jstarry via de664e8 August 27, 2024 13:19

jstarry previously approved these changes Aug 28, 2024

View reviewed changes

only update write_version if accounts notifier is some

ae2284a

apfitzge dismissed jstarry’s stale review via ae2284a August 28, 2024 12:59

jstarry approved these changes Aug 28, 2024

View reviewed changes

apfitzge merged commit d651409 into anza-xyz:master Aug 28, 2024
40 checks passed

apfitzge deleted the account_saver_refactor branch August 28, 2024 18:15

ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024

account_saver: Remove nested options (anza-xyz#2724)

d44bf43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

account_saver: Remove nested options #2724

account_saver: Remove nested options #2724

apfitzge commented Aug 23, 2024

apfitzge Aug 23, 2024

jstarry Aug 27, 2024

apfitzge Aug 27, 2024

apfitzge Aug 23, 2024

jstarry Aug 27, 2024

apfitzge Aug 27, 2024

apfitzge Aug 23, 2024

jstarry Aug 27, 2024

brooksprumo Aug 27, 2024

apfitzge Aug 27, 2024

apfitzge commented Aug 26, 2024 •

edited

Loading

jstarry commented Aug 27, 2024

jstarry left a comment

jstarry Aug 27, 2024

jstarry Aug 27, 2024

jstarry Aug 27, 2024

brooksprumo left a comment

brooksprumo Aug 27, 2024

apfitzge commented Aug 27, 2024

jstarry left a comment

jstarry Aug 28, 2024

apfitzge Aug 28, 2024

buffalojoec commented Aug 28, 2024

account_saver: Remove nested options #2724

account_saver: Remove nested options #2724

Conversation

apfitzge commented Aug 23, 2024

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge commented Aug 26, 2024 • edited Loading

jstarry commented Aug 27, 2024

jstarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brooksprumo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge commented Aug 27, 2024

jstarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buffalojoec commented Aug 28, 2024

apfitzge commented Aug 26, 2024 •

edited

Loading