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

Optimize interpreter::blockchain::{load_contract_code, code_copy to read contract starting from offset #847

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

### Changed
- [#847](https://github.com/FuelLabs/fuel-vm/pull/847): Remove `contract_read`, and change `load_contract_code`, `code_copy` and `code_root` to explicitly load the contract code in a buffer. Also check for mismatches between contract size stored and actual size of contract in those functions.
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this. There is no function called contract_read that is removed in these changes. And you added a call to read_contract, which sounds like the same thing?


### Fixed
- [860](https://github.com/FuelLabs/fuel-vm/pull/860): Fixed missing fuzzing coverage report in CI.

Expand Down
58 changes: 44 additions & 14 deletions fuel-vm/src/interpreter/blockchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ use fuel_storage::{
use fuel_tx::{
consts::BALANCE_ENTRY_SIZE,
BlobId,
Contract,
ContractIdExt,
DependentCost,
Receipt,
Expand Down Expand Up @@ -636,8 +637,12 @@ where
self.gas_cost,
charge_len,
)?;
let contract = super::contract::contract(self.storage, &contract_id)?;
let contract_bytes = contract.as_ref().as_ref();

let contract_buffer: Vec<u8> = load_contract_code_from_storage(
Copy link
Collaborator

Choose a reason for hiding this comment

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

After reviewing this change, I see that we basically have the same code as before and allocate vectors in all cases.

So basically, this change doesn't make a lot of sense=D

The main problem is why we can't do the optimal implementation and copy directly into the memory - contract offset(and blob offset, since we have the same problem for blobs LDC with mode == 1 and BLDD opcode).

I think we need to extend the StorageRead trait if we want to fix this issue in a proper way. A new method will do the same as the read or change the read method itself to something like:

fn read(&self, key: &Type::Key, offset: usize, buf: &mut [u8])
        -> Result<Option<usize>, Self::Error>

Could you change this method to work with offset and update all code to not allocate vector, please?

Also, it would be nice if you do the same for blobs as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the original idea was to have only one allocation. The only actual improvement in this PR as is now is fixing <MemoryStorage as StorageRead>::read`, which was panicking before due to slices of been different size being copied.

Happy to extend the trait to read. I think it would be better to add a new function read_from_offset, and then change read to be a provided method with offset 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with the new method and with reworking the old one. I think changing the old one makes more sense because the compiler will help you identify places that also need to be updated in the code to avoid allocation of the Vec. Plus this read function only exists for load opcodes, so they mainly define their existence, usage and shape.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, just to make sure: if we opt for modifying the StorageRead::read function, then there will be several places where the implementation needs to be updated, both in fuel-vm and fuel-core; also it is not clear to me that we can read from an arbitrary offset easily in all the implementations of the StorageRead trait. But I will try and see how far can I go before getting into trouble

self.storage,
contract_len as usize,
&contract_id,
)?;

let new_sp = ssp.saturating_add(length);
self.memory.grow_stack(new_sp)?;
Expand All @@ -654,7 +659,7 @@ where
copy_from_slice_zero_fill(
self.memory,
owner,
contract_bytes,
&contract_buffer,
region_start,
contract_offset,
length,
Expand Down Expand Up @@ -870,6 +875,28 @@ where
}
}

fn load_contract_code_from_storage<S>(
storage: &S,
contract_len: usize,
contract_id: &ContractId,
) -> Result<Vec<u8>, RuntimeError<<S as InterpreterStorage>::DataError>>
where
S: InterpreterStorage,
{
let mut contract_buffer: Vec<u8> = alloc::vec![0u8; contract_len as usize];
storage
.read_contract(&contract_id, &mut contract_buffer)
.transpose()
.ok_or(PanicReason::ContractNotFound)?
.map_err(RuntimeError::Storage)?;

if contract_buffer.len() != contract_len as usize {
Err(PanicReason::ContractMismatch)?
} else {
Ok(contract_buffer)
}
}

struct BurnCtx<'vm, S> {
storage: &'vm mut S,
context: &'vm Context,
Expand Down Expand Up @@ -994,9 +1021,14 @@ where
self.memory.write(self.owner, dst_addr, length)?;
self.input_contracts.check(&contract_id)?;

let contract = super::contract::contract(self.storage, &contract_id)?;
let contract_bytes = contract.as_ref().as_ref();
let contract_len = contract_bytes.len();
let contract_len = contract_size(self.storage, &contract_id)?;

let mut contract_buffer = load_contract_code_from_storage(
self.storage,
contract_len as usize,
&contract_id,
)?;

let charge_len = core::cmp::max(contract_len as u64, length);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We never read more than length bytes with the new version. Should charge_len be changed accordingly to reflect this?

let profiler = ProfileGas {
pc: self.pc.as_ref(),
Expand All @@ -1016,7 +1048,7 @@ where
copy_from_slice_zero_fill(
self.memory,
self.owner,
contract.as_ref().as_ref(),
&contract_buffer,
dst_addr,
contract_offset,
length,
Expand Down Expand Up @@ -1112,13 +1144,11 @@ impl<'vm, S> CodeRootCtx<'vm, S> {
self.gas_cost,
len as u64,
)?;
let root = self
.storage
.storage_contract(&contract_id)
.transpose()
.ok_or(PanicReason::ContractNotFound)?
.map_err(RuntimeError::Storage)?
.root();

let buf: Vec<u8> =
load_contract_code_from_storage(self.storage, len as usize, &contract_id)?;

let root = Contract::root_from_code(buf);

self.memory.write_bytes(self.owner, a, *root)?;

Expand Down
16 changes: 0 additions & 16 deletions fuel-vm/src/interpreter/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ use fuel_asm::{
};
use fuel_storage::StorageSize;
use fuel_tx::{
Contract,
Output,
Receipt,
};
Expand All @@ -55,8 +54,6 @@ use fuel_types::{
ContractId,
};

use alloc::borrow::Cow;

impl<M, S, Tx, Ecal> Interpreter<M, S, Tx, Ecal>
where
M: Memory,
Expand Down Expand Up @@ -179,19 +176,6 @@ where
}
}

pub(crate) fn contract<'s, S>(
storage: &'s S,
contract: &ContractId,
) -> IoResult<Cow<'s, Contract>, S::DataError>
where
S: InterpreterStorage,
{
storage
.storage_contract(contract)
.map_err(RuntimeError::Storage)?
.ok_or_else(|| PanicReason::ContractNotFound.into())
}

struct ContractBalanceCtx<'vm, S> {
storage: &'vm S,
memory: &'vm mut MemoryInstance,
Expand Down
9 changes: 0 additions & 9 deletions fuel-vm/src/storage/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,15 +145,6 @@ pub trait InterpreterStorage:
Ok(())
}

/// Fetch a previously inserted contract code from the chain state for a
/// given contract.
fn storage_contract(
&self,
id: &ContractId,
) -> Result<Option<Cow<'_, Contract>>, Self::DataError> {
StorageInspect::<ContractsRawCode>::get(self, id)
}

/// Fetch the size of a previously inserted contract code from the chain state for a
/// given contract.
fn storage_contract_size(
Expand Down
3 changes: 2 additions & 1 deletion fuel-vm/src/storage/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,8 @@ impl StorageRead<ContractsRawCode> for MemoryStorage {
) -> Result<Option<usize>, Self::Error> {
Ok(self.memory.contracts.get(key).map(|c| {
let len = buf.len().min(c.as_ref().len());
buf.copy_from_slice(&c.as_ref()[..len]);
buf[..len].copy_from_slice(&c.as_ref()[..len]);
buf[len..].fill(0);
len
}))
}
Expand Down
Loading