Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions programs/bpf_loader/src/syscalls/cpi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1308,8 +1308,9 @@ fn update_caller_account(
// never points to an invalid address.
//
// Note that the capacity can be smaller than the original length only if the account is
// reallocated using the AccountSharedData API directly (deprecated). BorrowedAccount
// and CoW don't trigger this, see BorrowedAccount::make_data_mut.
// reallocated using the AccountSharedData API directly (deprecated) or using
// BorrowedAccount::set_data_from_slice(), which implements an optimization to avoid an
// extra allocation.
let min_capacity = caller_account.original_data_len;
if callee_account.capacity() < min_capacity {
callee_account
Expand Down
8 changes: 7 additions & 1 deletion sdk/src/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,13 @@ impl AccountSharedData {
}

pub fn reserve(&mut self, additional: usize) {
self.data_mut().reserve(additional)
if let Some(data) = Arc::get_mut(&mut self.data) {
data.reserve(additional)
} else {
let mut data = Vec::with_capacity(self.data.len().saturating_add(additional));
data.extend_from_slice(&self.data);
self.data = Arc::new(data);
}
}

pub fn capacity(&self) -> usize {
Expand Down
11 changes: 4 additions & 7 deletions sdk/src/transaction_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -869,13 +869,10 @@ impl<'a> BorrowedAccount<'a> {
self.can_data_be_changed()?;
self.touch()?;
self.update_accounts_resize_delta(data.len())?;
// Calling make_data_mut() here guarantees that set_data_from_slice()
// copies in places, extending the account capacity if necessary but
// never reducing it. This is required as the account migh be directly
// mapped into a MemoryRegion, and therefore reducing capacity would
// leave a hole in the vm address space. After CPI or upon program
// termination, the runtime will zero the extra capacity.
self.make_data_mut();
// Note that we intentionally don't call self.make_data_mut() here. make_data_mut() will
// allocate + memcpy the current data if self.account is shared. We don't need the memcpy
// here tho because account.set_data_from_slice(data) is going to replace the content
// anyway.
Comment on lines +872 to +875
Copy link
Copy Markdown

@seanyoung seanyoung May 7, 2024

Choose a reason for hiding this comment

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

Very nice catch finding that self.make_data_mut() is redundant. Totally makes sense.

Suggested change
// Note that we intentionally don't call self.make_data_mut() here. make_data_mut() will
// allocate + memcpy the current data if self.account is shared. We don't need the memcpy
// here tho because account.set_data_from_slice(data) is going to replace the content
// anyway.
// Note that we intentionally don't call self.make_data_mut() here, since we are replacing
// the contents transaction wide anyway with account.set_data_from_slice(data)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm +0 on the edit, so I think I'm going to keep my comment. I think it's worth being explicit about make_data_mut() doing a copy, since the method name itself doesn't immediately suggest that.

self.account.set_data_from_slice(data);

Ok(())
Expand Down