wip: limit use of self ref struct in accounts index

[jeffwashington]

Problem

self-ref in accounts index is non-ideal.

Confirmed this gets rid of all but the rare, contentious creation of the self referencing struct.

Note the need to be removed.

Summary of Changes

Fixes #
#34786

[HaoranYi]

Nice catch! We should avoid the clone as much as possible!

[Lichtso]

Can confirm that this PR eliminates all allocations through ouroboros in my small test ledger.

[HaoranYi]

There are two other places that will cause the self-ref Arc to be cloned.

• AccountsDb::calculate_accounts_hash_from_index
• AccountsDb::get_append_vec_id

solana/accounts-db/src/accounts_db.rs

Line 9755 in 0d92254

pub fn get_append_vec_id(&self, pubkey: &Pubkey, slot: Slot) -> Option<AppendVecId> {

solana/core/src/accounts_hash_verifier.rs

Line 437 in 0d92254

.calculate_accounts_hash_from_index(slot, &calculate_accounts_hash_config);

But one of them, AccountsDb::get_append_vec_id seems to be deadcode.
And the other one AccountsDb::calculate_accounts_hash_from_index is only used in hash verification.

Both of them are not big deal.

But we may want to add a comment for fn AccountsIndex::get() to warn the caller that this fn makes a copy of the self-ref Arc.

[HaoranYi]

Another place that we will clone the Arc unnecessarily, which can be optimized too!

• AccountsIndex::get_account_read_entry_with_lock
  • AccountsIndex::get_account_read_entry
    • AccountsDb::do_shrink_slot_store

       for pubkey in shrink_collect.unrefed_pubkeys {
                if let Some(locked_entry) = self.accounts_index.get_account_read_entry(pubkey) {
                    // pubkeys in `unrefed_pubkeys` were unref'd in `shrink_collect` above under the assumption that we would shrink everything.
                    // Since shrink is not occurring, we need to addref the pubkeys to get the system back to the prior state since the account still exists at this slot.
                    locked_entry.addref();
                }
            }

[HaoranYi]

Three more places that can be optimized too!

• accounts_db::retry_to_get_account_accessor
• snapshot_minimizer::get_minimized_slot_set
• snapshot_minimizer::filter_storage

[HaoranYi]

Maybe we should audit all the places this in_mem_accoutns_index::get fn is called too. This fn also clone the Arc.

solana/accounts-db/src/in_mem_accounts_index.rs

Line 322 in 3004eaa

pub fn get(&self, pubkey: &K) -> Option<AccountMapEntry<T>> {

[HaoranYi]

Another idea is that we can change ReadAccountMapEntry to not hold Arc but hold &Arc, then we don't need to call Arc::clone when we get AccountMapEntry from the index.

#[self_referencing]
pub struct ReadAccountMapEntryRef<'a, T: IndexValue> {
    owned_entry: &'a AccountMapEntry<T>,
    #[borrows(owned_entry)]
    #[covariant]
    slot_list_guard: RwLockReadGuard<'this, SlotList<T>>,
}

A quick prototype and a simple test shows that this approach , i.e. ReadAccountMapEntryRef is
https://github.com/solana-labs/solana/compare/master...HaoranYi:jan_24_2024_self_ref_ref?expand=1

[Update]
However, it doesn't work.

   /// lookup 'pubkey' in index (in mem or on disk)
    pub fn get_ref<'a, 'b>(&'a self, pubkey: &K) -> Option<&'b AccountMapEntry<T>>
    where
        'a: 'b,
    {
        self.get_internal(pubkey, |entry| (true, entry))
    }

It looks like that we can't return entry, Option<&Arc<..>> out of get_ref. entry lifetime is not long enough to cover the return value.