Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

benches: disable caching per default #12232

Merged
merged 4 commits into from
Sep 13, 2022
Merged

benches: disable caching per default #12232

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
385a23d
Disable cache for storage benches
ggwpez Sep 9, 2022
0d27f49
Disable caching per default
ggwpez Sep 9, 2022
f85a82e
Update utils/frame/benchmarking-cli/src/storage/cmd.rs
ggwpez Sep 12, 2022
1653c22
Add --enable-trie-cache to 'storage' command
ggwpez Sep 12, 2022
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
14 changes: 14 additions & 0 deletions utils/frame/benchmarking-cli/src/block/cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ pub struct BlockCmd {
#[allow(missing_docs)]
#[clap(flatten)]
pub params: BenchmarkParams,

/// Enable the Trie cache.
///
/// This should only be used for performance analysis and not for final results.
#[clap(long)]
pub enable_trie_cache: bool,
}

impl BlockCmd {
Expand Down Expand Up @@ -98,4 +104,12 @@ impl CliConfiguration for BlockCmd {
fn import_params(&self) -> Option<&ImportParams> {
Some(&self.import_params)
}

fn trie_cache_maximum_size(&self) -> Result<Option<usize>> {
if self.enable_trie_cache {
Ok(self.import_params().map(|x| x.trie_cache_maximum_size()).unwrap_or_default())
} else {
Ok(None)
}
}
}
14 changes: 14 additions & 0 deletions utils/frame/benchmarking-cli/src/extrinsic/cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ pub struct ExtrinsicParams {
/// Extrinsic to benchmark.
#[clap(long, value_name = "EXTRINSIC", required_unless_present = "list")]
pub extrinsic: Option<String>,

/// Enable the Trie cache.
///
/// This should only be used for performance analysis and not for final results.
#[clap(long)]
pub enable_trie_cache: bool,
}

impl ExtrinsicCmd {
Expand Down Expand Up @@ -132,4 +138,12 @@ impl CliConfiguration for ExtrinsicCmd {
fn import_params(&self) -> Option<&ImportParams> {
Some(&self.import_params)
}

fn trie_cache_maximum_size(&self) -> Result<Option<usize>> {
if self.params.enable_trie_cache {
Ok(self.import_params().map(|x| x.trie_cache_maximum_size()).unwrap_or_default())
} else {
Ok(None)
}
}
}
14 changes: 14 additions & 0 deletions utils/frame/benchmarking-cli/src/overhead/cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ pub struct OverheadParams {
/// Good for adding LICENSE headers.
#[clap(long, value_name = "PATH")]
pub header: Option<PathBuf>,

/// Enable the Trie cache.
///
/// This should only be used for performance analysis and not for final results.
#[clap(long)]
pub enable_trie_cache: bool,
}

/// Type of a benchmark.
Expand Down Expand Up @@ -156,4 +162,12 @@ impl CliConfiguration for OverheadCmd {
fn import_params(&self) -> Option<&ImportParams> {
Some(&self.import_params)
}

fn trie_cache_maximum_size(&self) -> Result<Option<usize>> {
if self.params.enable_trie_cache {
Ok(self.import_params().map(|x| x.trie_cache_maximum_size()).unwrap_or_default())
} else {
Ok(None)
}
}
}
2 changes: 1 addition & 1 deletion utils/frame/benchmarking-cli/src/storage/cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub struct StorageParams {
/// Trie cache size in bytes.
///
/// Providing `0` will disable the cache.
#[clap(long, default_value = "1024")]
#[clap(long, default_value = "0")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[clap(long, default_value = "0")]
#[clap(long, default_value = "1024")]

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise it would not get enabled when you set enable_trie_cache.

Copy link
Member Author

@ggwpez ggwpez Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

After thinking about this more. I'm not sure how it worked before?

I am not sure either. Maybe the state-cache just did not have such a large influence as the Trie cache now does.
But yea, the former results should already be wrong due to state-caching. I did not notice this earlier, as --state-cache-size did not make a noticeable difference.

And when we disable the cache, wouldn't this hurt the other benchmarks which rely on the cache working?

Well, we need to measure worst case performance… so a second iteration of a benchmark should not rely on a former iteration. None of the benches should actually rely on a cache.

Otherwise it would not get enabled when you set enable_trie_cache.

I did not put a --enable-trie-cache for the storage command, since the trie-cache-size was not inherited from the import commands. But I will add it for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait I think this is bytes, will change to 67108864.

Copy link
Member

Choose a reason for hiding this comment

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

Well, we need to measure worst case performance… so a second iteration of a benchmark should not rely on a former iteration. None of the benches should actually rely on a cache.

Makes sense, but how do we then handle values which we assume of already being present? Aka the whitelisted values? Do we just somehow remove them from the formular that gives us the weight?

Copy link
Member Author

@ggwpez ggwpez Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we just somehow remove them from the formular that gives us the weight?

For pallet weights: Yes.

substrate/client/db/src/bench.rs

Lines 524 to 531 in 7de3d78

fn read_write_count(&self) -> (u32, u32, u32, u32) {
let mut reads = 0;
let mut repeat_reads = 0;
let mut writes = 0;
let mut repeat_writes = 0;
self.all_trackers().iter().for_each(|tracker| {
if !tracker.whitelisted {

That could be written a bit nicer with a .filter 🙈

For storage weights: Does not matter, since we need the average read/write weight for a key, regardless of whitelist, since whitelisted values are also read once.
For BlockExecutionWeight and ExtrinsicBaseWeight: No, since this is where we want to account for the cost of whitelisted keys, so they are included here.

pub trie_cache_size: usize,

/// Include child trees in benchmark.
Expand Down