Skip to content

deduplication for COPY operations #1655

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.

Sign up for GitHub

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

Closed
harsh-ps-2003 wants to merge 9 commits into from
Closed

deduplication for COPY operations #1655

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9d196f3
add copy_cost for *COPY operations
harsh-ps-2003 Jul 24, 2024
6b15eee
seperate common logic of opcodes to a method
harsh-ps-2003 Jul 25, 2024
1b72873
fmt nit
harsh-ps-2003 Jul 25, 2024
0453a50
performance nit
harsh-ps-2003 Jul 26, 2024
9c685ad
fmt nit
harsh-ps-2003 Jul 26, 2024
dbc2d57
test nits
harsh-ps-2003 Jul 26, 2024
1474059
fmt nit
harsh-ps-2003 Jul 26, 2024
a1a488b
refactor
harsh-ps-2003 Aug 6, 2024
f92ff49
Merge branch 'bluealloy:main' into copycode
harsh-ps-2003 Aug 6, 2024
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
9 changes: 7 additions & 2 deletions crates/interpreter/src/gas/calc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ pub fn exp_cost(spec_id: SpecId, power: U256) -> Option<u64> {
/// `*COPY` opcodes cost calculation.
#[inline]
pub const fn verylowcopy_cost(len: u64) -> Option<u64> {
VERYLOW.checked_add(tri!(cost_per_word(len, COPY)))
copy_cost(VERYLOW, len, COPY)
}

/// `EXTCODECOPY` opcode cost calculation.
Expand All @@ -131,7 +131,12 @@ pub const fn extcodecopy_cost(spec_id: SpecId, len: u64, is_cold: bool) -> Optio
} else {
20
};
base_gas.checked_add(tri!(cost_per_word(len, COPY)))
copy_cost(base_gas, len, COPY)
}

#[inline]
pub const fn copy_cost(base_cost: u64, len: u64, copy_cost_per_word: u64) -> Option<u64> {
base_cost.checked_add(tri!(cost_per_word(len, copy_cost_per_word)))
}

/// `LOG` opcode cost calculation.
Expand Down
71 changes: 36 additions & 35 deletions crates/interpreter/src/instructions/system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,23 +38,17 @@ pub fn codesize<H: Host + ?Sized>(interpreter: &mut Interpreter, _host: &mut H)

pub fn codecopy<H: Host + ?Sized>(interpreter: &mut Interpreter, _host: &mut H) {
pop!(interpreter, memory_offset, code_offset, len);
let len = as_usize_or_fail!(interpreter, len);
gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64));
if len == 0 {
return;
}
let memory_offset = as_usize_or_fail!(interpreter, memory_offset);
let code_offset = as_usize_saturated!(code_offset);
resize_memory!(interpreter, memory_offset, len);

let len = as_usize_or_fail!(interpreter, len);
// Inform the optimizer that the bytecode cannot be EOF to remove a bounds check.
assume!(!interpreter.contract.bytecode.is_eof());
// Note: this can't panic because we resized memory to fit.
interpreter.shared_memory.set_data(
let source = interpreter.contract.bytecode.original_byte_slice();
copy_to_memory(
interpreter,
memory_offset,
code_offset,
len,
interpreter.contract.bytecode.original_byte_slice(),
source as *const [u8],
);
}

Expand Down Expand Up @@ -94,20 +88,14 @@ pub fn callvalue<H: Host + ?Sized>(interpreter: &mut Interpreter, _host: &mut H)
pub fn calldatacopy<H: Host + ?Sized>(interpreter: &mut Interpreter, _host: &mut H) {
pop!(interpreter, memory_offset, data_offset, len);
let len = as_usize_or_fail!(interpreter, len);
gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64));
if len == 0 {
return;
}
let memory_offset = as_usize_or_fail!(interpreter, memory_offset);
let data_offset = as_usize_saturated!(data_offset);
resize_memory!(interpreter, memory_offset, len);

// Note: this can't panic because we resized memory to fit.
interpreter.shared_memory.set_data(
let source = interpreter.contract.input.as_ref();
copy_to_memory(
interpreter,
memory_offset,
data_offset,
len,
&interpreter.contract.input,
source as *const [u8],
);
}

Expand All @@ -127,8 +115,6 @@ pub fn returndatacopy<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interprete
pop!(interpreter, memory_offset, offset, len);

let len = as_usize_or_fail!(interpreter, len);
gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64));

let data_offset = as_usize_saturated!(offset);
let data_end = data_offset.saturating_add(len);

Expand All @@ -139,21 +125,13 @@ pub fn returndatacopy<H: Host + ?Sized, SPEC: Spec>(interpreter: &mut Interprete
return;
}

// if len is zero memory is not resized.
if len == 0 {
return;
}

// resize memory
let memory_offset = as_usize_or_fail!(interpreter, memory_offset);
resize_memory!(interpreter, memory_offset, len);

// Note: this can't panic because we resized memory to fit.
interpreter.shared_memory.set_data(
let source = interpreter.return_data_buffer.as_ref();
copy_to_memory(
interpreter,
memory_offset,
data_offset,
len,
&interpreter.return_data_buffer,
source as *const [u8],
);
}

Expand All @@ -179,6 +157,29 @@ pub fn returndataload<H: Host + ?Sized>(interpreter: &mut Interpreter, _host: &m
*offset = B256::from(output).into();
}

// common logic for copying data from a source buffer to the EVM's memory
fn copy_to_memory(
interpreter: &mut Interpreter,
memory_offset: U256,
data_offset: usize,
len: usize,
source: *const [u8],
) {
Copy link
Member

@rakita rakita Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would return a Option<usize> here that represents memory_offset.
https://github.com/bluealloy/revm/pull/1655/files#r1694998333

Without return function silently fails, and can work only if it is in the last line of instruction.

gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64));
if len == 0 {
return;
}
let memory_offset = as_usize_or_fail!(interpreter, memory_offset);
resize_memory!(interpreter, memory_offset, len);

// Note: this can't panic because we resized memory to fit.
unsafe {
interpreter
.shared_memory
.set_data(memory_offset, data_offset, len, &*source);
}
}
Copy link
Member

@rakita rakita Jul 29, 2024

Choose a reason for hiding this comment

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

I would remove this, and change behaviour of function to do gas, len check, memory resize.

As memory_offset is reused, we can return that from this function as Option<usize>

Copy link
Author

@harsh-ps-2003 harsh-ps-2003 Jul 31, 2024

Choose a reason for hiding this comment

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

The problem with changing the functions behaviour as suggested by you is that we encountering is a classic case of Rust's borrow checker preventing simultaneous mutable and immutable borrows of the same data.
When we do :

let source = interpreter.contract.input.as_ref();
    let memory_offset = copy_to_memory(interpreter, memory_offset, data_offset, len);
    if let Some(memory_offset) = memory_offset {
        // Note: this can't panic because we resized memory to fit.
        interpreter.shared_memory.set_data(memory_offset, data_offset, len, source);
    };

We create an immutable borrow of interpreter.contract.input and then we try to mutably borrow interpreter in the copy_to_memory function call. Finally, we try to use the immutable borrow source again in the set_data call.
The Rust borrow checker doesn't allow this because it can't guarantee that the mutable borrow in copy_to_memory won't invalidate the source reference.

How do you suggest I tackle this! @rakita

Copy link
Member

@rakita rakita Aug 2, 2024

Choose a reason for hiding this comment

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

can this line: let source = interpreter.contract.input.as_ref(); be moved inside if let Some(memory_.. so you don't have a borrow?

Copy link
Author

@harsh-ps-2003 harsh-ps-2003 Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Now when I return Option<usize> I get expected enum std::option::Option<usize>
found unit type () from the gas! macro. Thats why the tests are failing. How do you suggest I tackle this! @rakita

Copy link
Member

@rakita rakita Aug 6, 2024

Choose a reason for hiding this comment

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

  • There is a gas! version that takes Return. gas!(10,None) something similar should be added for gas_or_fail!
  • Change as_usize_or_fail to as_usize_or_fail_ret!(interpreter, memory_offset,None);
  • Change resize_memory! to resize_memory!(interpreter, memory_offset, len, None);


pub fn gas<H: Host + ?Sized>(interpreter: &mut Interpreter, _host: &mut H) {
gas!(interpreter, gas::BASE);
push!(interpreter, U256::from(interpreter.gas.remaining()));
Expand Down