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

introduce SerializedAccountMetadata #32644

Merged

Conversation

alessandrod
Copy link
Contributor

@alessandrod alessandrod commented Jul 27, 2023

Instead of passing original_account_lengths around as Vec<usize>, introduce an explicit type that includes the length and soon more. Doesn't change anything in the existing logic, just the type passed around.

Extracted from #31986. This is preparation of passing around more data from serialization down to CPI.

@alessandrod alessandrod requested a review from Lichtso July 27, 2023 17:00
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #32644 (07257aa) into master (6f88587) will increase coverage by 0.0%.
The diff coverage is 88.5%.

@@           Coverage Diff           @@
##           master   #32644   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         785      785           
  Lines      211023   211036   +13     
=======================================
+ Hits       173094   173124   +30     
+ Misses      37929    37912   -17     

@alessandrod alessandrod force-pushed the cpi-serialized-account-metadata branch from 2dfc0e5 to 0133a1a Compare July 28, 2023 06:57
…paramters_(aligned|unaligned)

This is in preparation of returning more than just the original length
…f a slice

This is in preparation of extracting account lenghts from a larger
context
Instead of passing original_account_lengths around as Vec<usize>,
introduce an explicit type that includes the length and soon more.
@alessandrod alessandrod force-pushed the cpi-serialized-account-metadata branch 2 times, most recently from 3337ebd to 07257aa Compare July 28, 2023 07:37
@alessandrod alessandrod merged commit e3f253d into solana-labs:master Jul 28, 2023
transaction_context: &TransactionContext,
instruction_context: &InstructionContext,
copy_account_data: bool,
buffer: &[u8],
account_lengths: &[usize],
account_lengths: I,
Copy link
Member

Choose a reason for hiding this comment

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

nit: i prefer existential types for these cases for one less type parameter (slight readability improvement, imo):

    account_lengths: impl IntoIterator<Item = usize>,

.map(|instruction_account_index| {
if let Some(index) = instruction_context
.is_instruction_account_duplicate(instruction_account_index)
.unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

(note to self) seems this and below new .unwrap()s are safe. i.e. these are always infallible before/after this pr.

Comment on lines +215 to +218
// fun fact: jemalloc is good at caching tiny allocations like this one,
// so collecting here is actually faster than passing the iterator
// around, since the iterator does the work to produce its items each
// time it's iterated on.
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@ryoqun ryoqun self-requested a review August 1, 2023 13:54
Copy link
Member

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

lgtm with a nit

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.

3 participants