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

Fixed benchmarks for the 0.25.3 #1870

Merged
merged 37 commits into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
2852f7a
Fixed benchmarks to follow new memory rule
xgreenx Apr 20, 2024
57a9619
WIP: refactor commit_changes_with_height_update
Dentosal Apr 24, 2024
2a503bd
Make it actually work
Dentosal Apr 24, 2024
3e24828
Merge branch 'master' into dento/regenesis-migrate-FuelBlockIdsToHeights
Dentosal Apr 24, 2024
46d6c8a
Revert changes to commit
Dentosal Apr 24, 2024
ba36f01
Metadata using RwLock
Dentosal Apr 25, 2024
562bca3
Add genesis flag to db handler
Dentosal Apr 25, 2024
336378a
More docs
Dentosal Apr 25, 2024
9c0fd52
Add changelog entry
Dentosal Apr 25, 2024
1c91490
Merge branch 'master' into dento/regenesis-migrate-FuelBlockIdsToHeights
Dentosal Apr 25, 2024
d50083e
Cleanup
Dentosal Apr 25, 2024
8c861e0
Simplify locking
Dentosal Apr 25, 2024
ee3c832
Merge branch 'master' into feature/benchmarks-0.25.2
xgreenx Apr 25, 2024
8e91767
Fixed csiz benchmark and updated VM initialization benchmark
xgreenx Apr 25, 2024
d8fd3dd
Fixed something
xgreenx Apr 26, 2024
7fcd31d
Temporary fixed benchmarks
xgreenx Apr 28, 2024
df13a4a
Use UncheckedDatabase type for genesis imports
Dentosal Apr 29, 2024
451f8be
Restore TransactableStorage::commit height parameter
Dentosal Apr 29, 2024
28de830
Make Database use UncheckedDatabase internally
Dentosal Apr 29, 2024
8ba1fc2
Restore height fetch on db creation
Dentosal Apr 29, 2024
97d681c
Used stages to reuse most of the functionality
xgreenx Apr 29, 2024
2136866
Merge branch 'dento/regenesis-migrate-FuelBlockIdsToHeights' into fea…
xgreenx Apr 29, 2024
2bf802f
Fixed benchmarks with a proper fix from https://github.com/FuelLabs/f…
xgreenx Apr 29, 2024
56859ad
Merge branch 'master' into feature/benchmarks-0.25.2
xgreenx Apr 29, 2024
f7946bc
Reset some unnecessary changes
xgreenx Apr 29, 2024
9978427
Merge branch 'master' into feature/benchmarks-0.25.2
xgreenx Apr 29, 2024
59fc1a1
Make CI happy
xgreenx Apr 29, 2024
d9c2d8f
Merge branch 'master' into feature/benchmarks-0.25.2
xgreenx Apr 29, 2024
7738ffa
Merge branch 'master' into feature/benchmarks-0.25.2
xgreenx Apr 29, 2024
298685d
Merge branch 'master' into feature/benchmarks-0.25.2
xgreenx Apr 29, 2024
43756c4
Applied `master`
xgreenx Apr 29, 2024
ba11d13
Update CHANGELOG.md
xgreenx Apr 29, 2024
b9eb596
Improved redeability of the vm initialization
xgreenx Apr 30, 2024
6397e25
Merge remote-tracking branch 'origin/feature/benchmarks-0.25.2' into …
xgreenx Apr 30, 2024
9891916
Merge branch 'master' into feature/benchmarks-0.25.2
xgreenx Apr 30, 2024
31fdbb0
Hope
xgreenx Apr 30, 2024
7be5108
Merge remote-tracking branch 'origin/feature/benchmarks-0.25.2' into …
xgreenx Apr 30, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- [#1866](https://github.com/FuelLabs/fuel-core/pull/1866): Fixed a runtime panic that occurred when restarting a node. The panic happens when the relayer database is already populated, and the relayer attempts an empty commit during start up. This invalid commit is removed in this PR.
- [#1871](https://github.com/FuelLabs/fuel-core/pull/1871): Fixed `block` endpoint to return fetch the blocks from both databases after regenesis.
- [#1856](https://github.com/FuelLabs/fuel-core/pull/1856): Replaced instances of `Union` with `Enum` for GraphQL definitions of `ConsensusParametersVersion` and related types. This is needed because `Union` does not support multiple `Version`s inside discriminants or empty variants.
- [#1870](https://github.com/FuelLabs/fuel-core/pull/1870): Fixed benchmarks for the `0.25.3`.
- [#1870](https://github.com/FuelLabs/fuel-core/pull/1870): Improves the performance of getting the size of the contract from the `InMemoryTransaction`.
- [#1851](https://github.com/FuelLabs/fuel-core/pull/1851/): Provided migration capabilities (enabled addition of new column families) to RocksDB instance.

### Added
Expand Down
2 changes: 2 additions & 0 deletions benches/benches/block_target_gas_set/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ pub fn run_crypto(group: &mut BenchmarkGroup<WallTime>) {
op::movi(0x11, 32),
op::aloc(0x11),
op::movi(0x10, i),
op::cfe(0x10),
op::s256(RegId::HP, RegId::ZERO, 0x10),
op::jmpb(RegId::ZERO, 0),
]
Expand All @@ -161,6 +162,7 @@ pub fn run_crypto(group: &mut BenchmarkGroup<WallTime>) {
op::movi(0x11, 32),
op::aloc(0x11),
op::movi(0x10, i),
op::cfe(0x10),
op::k256(RegId::HP, RegId::ZERO, 0x10),
op::jmpb(RegId::ZERO, 0),
]
Expand Down
11 changes: 11 additions & 0 deletions benches/benches/vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,14 @@ fn vm(c: &mut Criterion) {

criterion_group!(benches, vm);
criterion_main!(benches);

// If you want to debug the benchmarks, you can run them with code below:
// But first you need to comment `criterion_group` and `criterion_main` macros above.
//
// fn main() {
// let mut criterio = Criterion::default();
// blockchain::run(&mut criterio);
// }
//
// #[test]
// fn dummy() {}
63 changes: 51 additions & 12 deletions benches/benches/vm_initialization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use criterion::{
Criterion,
Throughput,
};
use fuel_core_storage::InterpreterStorage;
use fuel_core_types::{
fuel_asm::{
op,
Expand All @@ -22,7 +23,10 @@ use fuel_core_types::{
checked_transaction::{
Checked,
IntoChecked,
Ready,
},
constraints::reg_key::Reg,
consts::VM_MAX_RAM,
interpreter::NotSupportedEcal,
Interpreter,
},
Expand Down Expand Up @@ -75,36 +79,71 @@ fn transaction<R: Rng>(

pub fn vm_initialization(c: &mut Criterion) {
let mut rng = StdRng::seed_from_u64(8586);

let consensus_params = ConsensusParameters::default();
let mut group = c.benchmark_group("vm_initialization");

let consensus_params = ConsensusParameters::default();
let mut i = 5usize;
loop {
// Increase the size of the script to measure the performance of the VM initialization
// with a large script. THe largest allowed script is 64 KB = 2^16 bytes.
const TX_SIZE_POWER_OF_TWO: usize = 16;

for i in 5..=TX_SIZE_POWER_OF_TWO {
let size = 8 * (1 << i);
if size as u64 > consensus_params.script_params().max_script_data_length() {
break
}

let script = vec![op::ret(1); size / Instruction::SIZE]
.into_iter()
.collect();
let script_data = vec![255; size];
let tx = transaction(&mut rng, script, script_data, &consensus_params);
let tx_size = tx.transaction().size();
let tx = tx.test_into_ready();

let name = format!("vm_initialization_with_tx_size_{}", tx_size);
group.throughput(Throughput::Bytes(tx_size as u64));
group.bench_function(name, |b| {
b.iter(|| {
let mut vm = black_box(
let vm = black_box(
Interpreter::<_, Script, NotSupportedEcal>::with_memory_storage(),
);
let ready_tx = tx.clone().test_into_ready();
black_box(vm.init_script(ready_tx))
.expect("Should be able to execute transaction");

// Initialize the VM and require the allocation of the whole memory
// to charge for the worst possible case.
black_box(initialize_vm(black_box(tx.clone()), vm));
Copy link
Member

Choose a reason for hiding this comment

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

This is even more confusing from before. I'm not sure where the extra black_box came from either.

Can we just have a method that says what we want, and then hide the implementation behind it? We probably can get rid of the comment in that case too.

unoptimized_and_fully_saturated_vm or something like that. Then have the internal code create the vm and wrap the vm and the tx in black_box?

})
});
i += 1;
}

group.finish();
}

fn initialize_vm<S>(
ready_tx: Ready<Script>,
mut vm: Interpreter<S, Script>,
) -> Interpreter<S, Script>
where
S: InterpreterStorage,
{
vm.init_script(ready_tx)
.expect("Should be able to execute transaction");

const POWER_OF_TWO_OF_HALF_VM: u64 = 25;
const VM_MEM_HALF: u64 = 1 << POWER_OF_TWO_OF_HALF_VM;
assert_eq!(VM_MEM_HALF, VM_MAX_RAM / 2);

for i in 0..=POWER_OF_TWO_OF_HALF_VM {
let stack = 1 << i;
let heap = VM_MAX_RAM - stack;

vm.memory_mut()
.grow_stack(stack)
.expect("Should be able to grow stack");
vm.memory_mut()
.grow_heap(Reg::new(&0), heap)
.expect("Should be able to grow heap");
}

vm.memory_mut()
.grow_heap(Reg::new(&0), 0)
.expect("Should be able to grow heap");

vm
}
3 changes: 3 additions & 0 deletions benches/benches/vm_set/blockchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,8 @@ pub fn run(c: &mut Criterion) {
op::addi(0x11, 0x11, WORD_SIZE.try_into().unwrap()),
op::addi(0x11, 0x11, AssetId::LEN.try_into().unwrap()),
op::movi(0x12, i as u32),
// Allocate space for storage values in the memory
op::cfei(i as u32 * Bytes32::LEN as u32),
];
let mut bench = VmBench::contract_using_db(
rng,
Expand Down Expand Up @@ -393,6 +395,7 @@ pub fn run(c: &mut Criterion) {
op::addi(0x11, 0x11, WORD_SIZE.try_into().unwrap()),
op::movi(0x12, 100_000),
op::movi(0x13, i.try_into().unwrap()),
op::cfe(0x13),
op::movi(0x14, i.try_into().unwrap()),
op::movi(0x15, i.try_into().unwrap()),
op::add(0x15, 0x15, 0x15),
Expand Down
14 changes: 12 additions & 2 deletions benches/benches/vm_set/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,12 @@ pub fn run(c: &mut Criterion) {
&mut bench_k256,
format!("{i}"),
VmBench::new(op::k256(RegId::HP, RegId::ZERO, 0x10)).with_prepare_script(
vec![op::movi(0x11, 32), op::aloc(0x11), op::movi(0x10, *i)],
vec![
op::movi(0x11, 32),
op::aloc(0x11),
op::movi(0x10, *i),
op::cfe(0x10),
],
),
);
}
Expand All @@ -95,7 +100,12 @@ pub fn run(c: &mut Criterion) {
&mut bench_s256,
format!("{i}"),
VmBench::new(op::s256(RegId::HP, RegId::ZERO, 0x10)).with_prepare_script(
vec![op::movi(0x11, 32), op::aloc(0x11), op::movi(0x10, *i)],
vec![
op::movi(0x11, 32),
op::aloc(0x11),
op::movi(0x10, *i),
op::cfe(0x10),
],
),
);
}
Expand Down
6 changes: 3 additions & 3 deletions benches/benches/vm_set/flow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub fn run(c: &mut Criterion) {
format!("{i}"),
VmBench::contract(rng, op::retd(RegId::ONE, 0x10))
.unwrap()
.with_post_call(vec![op::movi(0x10, *i)])
.with_post_call(vec![op::movi(0x10, *i), op::cfe(0x10)])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the significance of adding cfe here or why it uses these specific registers. Can you elaborate on why we need this change? Is this just to make sure our benchmarks account for the worst case scenario?

Copy link
Collaborator Author

@xgreenx xgreenx Apr 29, 2024

Choose a reason for hiding this comment

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

Because of the new VM rules: the memory that we are accessing should be initialized. The cfe opcode in the benchmarks expands the stack to allocate the memory that will later be used by the instruction that we are benchmarking.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand better. In this example, we have retd(RegId::ONE, 0x10), where we return the memory in the range from *ONE to *0x10. So we need to extend the call stack by the value pointed by 0x10.

.with_call_receipts(receipts_ctx.clone()),
);
}
Expand All @@ -84,7 +84,7 @@ pub fn run(c: &mut Criterion) {
format!("{i}"),
VmBench::contract(rng, op::retd(RegId::ONE, 0x10))
.unwrap()
.with_post_call(vec![op::movi(0x10, *i)])
.with_post_call(vec![op::movi(0x10, *i), op::cfe(0x10)])
.with_call_receipts(receipts_ctx.clone()),
);
}
Expand Down Expand Up @@ -118,7 +118,7 @@ pub fn run(c: &mut Criterion) {
&mut logd,
format!("{i}"),
VmBench::new(op::logd(0x10, 0x11, RegId::ZERO, 0x13))
.with_prepare_script(vec![op::movi(0x13, *i)])
.with_prepare_script(vec![op::movi(0x13, *i), op::cfe(0x13)])
.with_call_receipts(receipts_ctx.clone()),
);
}
Expand Down
3 changes: 3 additions & 0 deletions benches/benches/vm_set/mem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ pub fn run(c: &mut Criterion) {
VmBench::new(op::mcp(0x10, RegId::ZERO, 0x11)).with_prepare_script(vec![
op::movi(0x11, *i),
op::aloc(0x11),
op::cfe(0x11),
op::move_(0x10, RegId::HP),
]),
);
Expand All @@ -117,6 +118,7 @@ pub fn run(c: &mut Criterion) {
vec![
op::movi(0x11, *i),
op::aloc(0x11),
op::cfe(0x11),
op::move_(0x10, RegId::HP),
],
),
Expand All @@ -132,6 +134,7 @@ pub fn run(c: &mut Criterion) {
let mut prepare_script =
vec![op::move_(0x11, RegId::ZERO), op::move_(0x12, RegId::ZERO)];
prepare_script.extend(set_full_word(0x13, i));
prepare_script.extend(vec![op::cfe(0x13)]);

run_group_ref(
&mut mem_meq,
Expand Down
23 changes: 16 additions & 7 deletions benches/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
pub mod default_gas_costs;
pub mod import;

pub use fuel_core_storage::vm_storage::VmStorage;
use fuel_core::database::GenesisDatabase;
use fuel_core_storage::transactional::{
IntoTransaction,
StorageTransaction,
};
use fuel_core_types::{
fuel_asm::{
op,
Expand Down Expand Up @@ -29,11 +30,16 @@ use fuel_core_types::{
*,
},
};
use std::{
iter,
mem,
};

use fuel_core::database::GenesisDatabase;
use fuel_core_storage::transactional::StorageTransaction;
pub mod default_gas_costs;
pub mod import;

pub use fuel_core_storage::vm_storage::VmStorage;
pub use rand::Rng;
use std::iter;

const LARGE_GAS_LIMIT: u64 = u64::MAX - 1001;

Expand Down Expand Up @@ -424,6 +430,9 @@ impl TryFrom<VmBench> for VmBenchPrepared {

db.deploy_contract_with_id(&[], &Contract::default(), &contract_id)?;
}
let transaction = mem::take(db.database_mut());
let database = transaction.commit().expect("Failed to commit transaction");
*db.database_mut() = database.into_transaction();

inputs.into_iter().for_each(|i| {
tx.add_input(i);
Expand Down
Binary file not shown.
Binary file modified bin/fuel-core/chainspec/testnet/state_transition_bytecode.wasm
Binary file not shown.
2 changes: 1 addition & 1 deletion crates/fuel-core/src/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ where
// all next commits should be linked(the height should increase each time by one).
}
(Some(prev_height), None) => {
// In production, we shouldn't have cases where we call `commit_chagnes` with intermediate changes.
// In production, we shouldn't have cases where we call `commit_changes` with intermediate changes.
// The commit always should contain all data for the corresponding height.
return Err(DatabaseError::NewHeightIsNotSet {
prev_height: prev_height.as_u64(),
Expand Down
71 changes: 65 additions & 6 deletions crates/storage/src/transactional.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,20 +227,54 @@ impl<Storage> Modifiable for InMemoryTransaction<Storage> {
}
}

impl<Column, S> InMemoryTransaction<S>
where
Column: StorageColumn,
S: KeyValueInspect<Column = Column>,
{
fn get_from_changes(&self, key: &[u8], column: Column) -> Option<&WriteOperation> {
let k = key.to_vec();
self.changes
.get(&column.id())
.and_then(|btree| btree.get(&k))
}
}

impl<Column, S> KeyValueInspect for InMemoryTransaction<S>
where
Column: StorageColumn,
S: KeyValueInspect<Column = Column>,
{
type Column = Column;

fn exists(&self, key: &[u8], column: Self::Column) -> StorageResult<bool> {
if let Some(operation) = self.get_from_changes(key, column) {
match operation {
WriteOperation::Insert(_) => Ok(true),
WriteOperation::Remove => Ok(false),
}
} else {
self.storage.exists(key, column)
}
}

fn size_of_value(
&self,
key: &[u8],
column: Self::Column,
) -> StorageResult<Option<usize>> {
if let Some(operation) = self.get_from_changes(key, column) {
match operation {
WriteOperation::Insert(value) => Ok(Some(value.len())),
WriteOperation::Remove => Ok(None),
}
} else {
self.storage.size_of_value(key, column)
}
}

fn get(&self, key: &[u8], column: Self::Column) -> StorageResult<Option<Value>> {
let k = key.to_vec();
if let Some(operation) = self
.changes
.get(&column.id())
.and_then(|btree| btree.get(&k))
{
if let Some(operation) = self.get_from_changes(key, column) {
match operation {
WriteOperation::Insert(value) => Ok(Some(value.clone())),
WriteOperation::Remove => Ok(None),
Expand All @@ -249,6 +283,31 @@ where
self.storage.get(key, column)
}
}

fn read(
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. Did we not have implementations for these methods before on InMemoryTransaction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We had a default implementation that was calling a get method which is less optimal in the case of RocksDB.

image

&self,
key: &[u8],
column: Self::Column,
buf: &mut [u8],
) -> StorageResult<Option<usize>> {
if let Some(operation) = self.get_from_changes(key, column) {
match operation {
WriteOperation::Insert(value) => {
let read = value.len();
if read != buf.len() {
return Err(crate::Error::Other(anyhow::anyhow!(
"Buffer size is not equal to the value size"
)));
}
buf.copy_from_slice(value.as_ref());
Ok(Some(read))
}
WriteOperation::Remove => Ok(None),
}
} else {
self.storage.read(key, column, buf)
}
}
}

impl<Column, S> KeyValueMutate for InMemoryTransaction<S>
Expand Down
1 change: 1 addition & 0 deletions crates/types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ pub mod fuel_vm {
#[doc(no_inline)]
pub use fuel_vm_private::{
checked_transaction,
constraints,
consts,
crypto,
double_key,
Expand Down
Loading