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

acerone85 · 2024-10-04T16:06:52Z

[Link to related issue(s) here, if any]

Closes #681

[Short description of the changes.]

When loading the contract code into the stack, we use the new semantics of StorageRead from #863 to only fetch the portion of a contract starting from the specified offset. We also read the contract bytes directly into the stack.

A similar optimization is done for code_copy.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

netrome

Looks alright to me. The TODO comment needs to be resolved before I can approve. There's also some code duplication that I think we could factor out, but that's not blocking from my end.

fuel-vm/src/interpreter/blockchain.rs

netrome

Nice stuff!

netrome · 2024-10-22T12:46:36Z

Poke me when you've fixed the clippy errors and I'll re-approve

MitchTurner

I'm not a VM guy, so maybe I could get you to walk me through these changes.

MitchTurner · 2024-10-22T16:28:05Z

CHANGELOG.md

@@ -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.


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?

MitchTurner · 2024-10-22T16:29:07Z

fuel-vm/src/interpreter/blockchain.rs

+        .ok_or(PanicReason::ContractNotFound)?
+        .map_err(RuntimeError::Storage)?;
+
+    if contract_buffer.len() != contract_len {


If it's already allocated to to contracct_len size, will the len call ever not be contract_len?

Yes, this is a leftover from refactoring, most probably this check should not be there.
(Btw, I noticed that we have a slightly different implementation of the same function:

fuel-vm/fuel-vm/src/interpreter/flow.rs

Line 631 in a6abe47

fn read_contract<S>(

, so maybe we should factor out the common code and have only one function instead)

xgreenx · 2024-10-22T17:13:20Z

fuel-vm/src/interpreter/blockchain.rs

-        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(


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.

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

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.

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

acerone85 · 2024-10-23T09:45:39Z

I'm not a VM guy, so maybe I could get you to walk me through these changes.

Happy to do it once I address the comments left by @xgreenx :)

…pecified offset

acerone85 · 2024-11-12T15:20:32Z

fuel-vm/src/interpreter/blockchain.rs

-        let contract_bytes = contract.as_ref().as_ref();
-        let contract_len = contract_bytes.len();
+        let contract_bytes = self.memory.write(self.owner, dst_addr, length)?;
+        let contract_len = contract_size(&self.storage, &contract_id)?;
        let charge_len = core::cmp::max(contract_len as u64, length);


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

That would be a breaking change, and should be done separately.

I also prefer to keep the current charging strategy, even if we overcharge user=) The length in most cases should be not more than contract_len.

acerone85 · 2024-11-12T15:23:20Z

With the new version the function code_root still uses the old StorageInspect::get<RawContractCode> function. This seems to be okay to me, as the function will return a borrowed reference to the whole contract, which will then be moved into chunks into a MerkleTree structure to compute the code root. In this case, using StorageRead::read<RawContractCode> would require allocating more space to store the contract code, which can be avoided. Do you agree with this?

xgreenx · 2024-11-27T22:35:55Z

fuel-vm/src/interpreter/blockchain.rs

+            // to invoke `self.storage.read_contract`. Furthermore,
+            // the call to `self.storage.read_contract` cannot fail with
+            // error `StorageError::`
+            #[allow(clippy::cast_possible_truncation)]


I think it is better to remove it and contract_len.into() above, and convert contract_offset into the u32 at the begin of the function explicitly returning an error. In this case you don't need to have a comment

xgreenx · 2024-11-27T22:44:15Z

fuel-vm/src/interpreter/blockchain.rs

+            // error `StorageError::`
+            #[allow(clippy::cast_possible_truncation)]
+            self.storage
+                .read_contract(&contract_id, contract_offset as usize, contract_buffer)


I think the fuel-vm should be responsible for extracting subsection from contract_buffer and passing it to the read_contract function.

Plus, it should be responsible for filling contract_buffer[(length - (contract_len - contract_offset))..].fill(0); as well.

Because it is requirement from the specification and we don't want to leak business logic to the storage.

The same is related to another place in the PR.

Also updated teh blobs logic to use the `read` function as well.

xgreenx

@Dentosal Could you also review this PR please?=)

acerone85 requested review from xgreenx, Dentosal and MitchTurner as code owners October 4, 2024 16:06

acerone85 marked this pull request as draft October 4, 2024 16:06

acerone85 force-pushed the 681_remove_storage_read branch from 318b0b3 to 9f4117d Compare October 4, 2024 16:20

Copy directly into stack when loading contract code

ab0cc7b

acerone85 force-pushed the 681_remove_storage_read branch from 9f4117d to ab0cc7b Compare October 4, 2024 16:30

acerone85 added 5 commits October 4, 2024 19:05

Update gas costs

816390f

Merge branch 'master' into 681_remove_storage_read

994008e

Update changelog

ded2cff

Merge branch 'master' into 681_remove_storage_read

ccfbb61

Revert optimization on LDC

ef32062

acerone85 self-assigned this Oct 14, 2024

acerone85 marked this pull request as ready for review October 14, 2024 12:47

acerone85 requested a review from Voxelot as a code owner October 14, 2024 12:47

Merge branch 'master' into 681_remove_storage_read

0280827

netrome reviewed Oct 22, 2024

View reviewed changes

fuel-vm/src/interpreter/blockchain.rs Outdated Show resolved Hide resolved

fuel-vm/src/interpreter/blockchain.rs Outdated Show resolved Hide resolved

acerone85 added 3 commits October 22, 2024 13:19

Merge branch 'master' into 681_remove_storage_read

83a97ba

Remove TODO comment

8c6ced6

Isolate load contract code to helper function

bd00e21

netrome previously approved these changes Oct 22, 2024

View reviewed changes

Address clippy's errors

010dd6f

acerone85 dismissed netrome’s stale review via 010dd6f October 22, 2024 13:59

netrome previously approved these changes Oct 22, 2024

View reviewed changes

MitchTurner reviewed Oct 22, 2024

View reviewed changes

xgreenx reviewed Oct 22, 2024

View reviewed changes

WIP: Change Storage Read trait

d67dd77

xgreenx mentioned this pull request Oct 30, 2024

Add the possibility to specify an offset for the read value in StorageRead::read #863

Merged

10 tasks

Merge branch 'master' into 681_remove_storage_read

470cd31

acerone85 dismissed netrome’s stale review via 470cd31 November 12, 2024 13:13

acerone85 added 3 commits November 12, 2024 15:04

Reset to master

226330a

Optimize load_contract and code_copy to read contract starting from s…

c6ae75e

…pecified offset

Changelog

5cfd978

acerone85 commented Nov 12, 2024

View reviewed changes

acerone85 changed the title ~~Remove storage read~~ Optimize interpreter::blockchain::{load_contract_code, code_copy to read contract starting from offset Nov 13, 2024

xgreenx requested changes Nov 27, 2024

View reviewed changes

xgreenx added 3 commits December 2, 2024 17:12

Merge branch 'refs/heads/master' into 681_remove_storage_read

b64929f

Apply suggestion from the review.

4e228be

Also updated teh blobs logic to use the `read` function as well.

Return right error in the case of non existing source

ba2d17a

xgreenx approved these changes Dec 3, 2024

View reviewed changes

Merge branch 'master' into 681_remove_storage_read

26e6cd7

Dentosal approved these changes Dec 3, 2024

View reviewed changes

Dentosal changed the title ~~Optimize interpreter::blockchain::{load_contract_code, code_copy to read contract starting from offset~~ Optimize interpreter::blockchain::{load_contract_code, code_copy} to read contract starting from offset Dec 3, 2024

xgreenx added this pull request to the merge queue Dec 3, 2024

Merged via the queue into master with commit 917f3fa Dec 3, 2024
40 checks passed

xgreenx deleted the 681_remove_storage_read branch December 3, 2024 02:18

xgreenx mentioned this pull request Dec 4, 2024

Release v0.59.0 #877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

acerone85 commented Oct 4, 2024 •

edited

Loading

netrome left a comment

netrome left a comment

netrome commented Oct 22, 2024

MitchTurner left a comment

MitchTurner Oct 22, 2024

MitchTurner Oct 22, 2024

acerone85 Oct 22, 2024

xgreenx Oct 22, 2024

acerone85 Oct 22, 2024

xgreenx Oct 22, 2024

acerone85 Oct 24, 2024

acerone85 commented Oct 23, 2024 •

edited

Loading

acerone85 Nov 12, 2024

Dentosal Dec 3, 2024

xgreenx Dec 3, 2024

acerone85 commented Nov 12, 2024

xgreenx Nov 27, 2024

xgreenx Nov 27, 2024

xgreenx left a comment

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

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

Conversation

acerone85 commented Oct 4, 2024 • edited Loading

Checklist

Before requesting review

After merging, notify other teams

netrome left a comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

netrome commented Oct 22, 2024

MitchTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerone85 commented Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerone85 commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

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

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

acerone85 commented Oct 4, 2024 •

edited

Loading

acerone85 commented Oct 23, 2024 •

edited

Loading