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

to_account_metas()macro with Vec::with_capacity(#length) #2399

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

Conversation

Eliascm17
Copy link

Problem

  • Up until this point, the to_account_metas() macro would use .extend() to append account metas to an already allocated vec which would make for unnecessary memory allocations which would also waste compute units.

Solution

  • Introduce ToAccountMeta trait that allowed us to:
    • introduce to_account_meta() method that returns a single account meta for the same number of ToAccountMetas implentations
    • update to_account_metas() method implementations that return a vec of to_account_meta()
  • update to_account_metas() macro to:
    • take into account the number of accounts in Context<T> and use that as an arg to pass into Vec::with_capacity(#length)
    • use .push() instead of .extend() when adding account metas to the vec which rids of the unnecessary memory allocations

Previously:
Screenshot 2023-02-15 at 9 39 51 PM
Now:
Screenshot 2023-02-15 at 9 40 06 PM

cc: shoutout @cavemanloverboy & @nickgarfield for doing all of the heavy lifting. Just trying to get this PR submission across the finish line.

@vercel
Copy link

vercel bot commented Feb 16, 2023

@Eliascm17 is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@cavemanloverboy
Copy link
Contributor

cavemanloverboy commented Feb 16, 2023

The changes here are rather impactful. The snippet shown above does upwards of 16 memory allocations to construct a single Vec<AccountMeta>.

Copy link
Contributor

@Aursen Aursen left a comment

Choose a reason for hiding this comment

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

Please can you remove cpi_account, cpi_state, loader and state from the accounts module?

@Henry-E
Copy link

Henry-E commented Feb 20, 2023

Going to run through this now but once we merge #2386 , the macros will have to be added to the new account types as well.

@Eliascm17 For some reason the InitSpace macro isn't being recognised and this is causing cargo build to fail. Are you sure you rebased to the latest master?

Overall I like the idea the of switching to using a macro because extending vectors like this is super wasteful but I will have to read through this in more detail to understand whether it correctly covers the case of composite accounts.

} else {
account_metas.push(AccountMeta::new_readonly(crate::ID, false));
}
}
} else {
quote! {
account_metas.extend(self.#name.to_account_metas(#is_signer));
account_metas.push(self.#name.to_account_meta(#is_signer));
Copy link

Choose a reason for hiding this comment

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

yeh, this basically breaks how composite accounts work. Since you're calling to_account_metas on the another account context type which has itself used the derive(Accounts) macro.

Copy link
Contributor

Choose a reason for hiding this comment

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

There may be a way to handle this by checking if the type is a composite account.

Alternatively, would be nice if the derive(Accounts) macro also implemented Iterator and iterated through the accounts in order. This could allow you to call to_account_meta on each element of a composite account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants