Benchmark storage arg to read fraction of kv pairs. #1233

adelarja · 2023-08-29T00:58:03Z

The purpose of this pull request is to introduce an argument to the benchmarking storage CLI, enabling the selection of the percentage of database keys for benchmark execution.

The argument is named db-fraction and accepts values between 1 and 100 (1% to 100%).

Example usage (using 10% of keys):

./target/release/substrate benchmark storage --dev --state-version=0 --db=paritydb --db-fraction 10

Closes #146.

cla-bot-2021 · 2023-08-29T00:58:10Z

User @adelarja, please sign the CLA here.

…kv-pairs

…github.com:adelarja/polkadot-sdk into benchmark-storage-arg-to-read-fraction-of-kv-pairs

paritytech-cicd-pr · 2023-08-29T01:34:51Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3463878

substrate/utils/frame/benchmarking-cli/src/storage/read.rs

substrate/utils/frame/benchmarking-cli/src/storage/write.rs

ggwpez · 2023-09-03T12:59:04Z

substrate/utils/frame/benchmarking-cli/src/storage/read.rs


+		info!("Reading {} keys of {} keys", number_of_keys, keys.len());
+		let keys = &keys[0..number_of_keys];
 		let mut child_nodes = Vec::new();


I dont know if we should also apply this to the child nodes. Otherwise there could be cases where one large top key pulls in a huge child tree.

WDYT @crystalin?

I guess we should also do it for the child_nodes below.

I agree. Capping the vec size to db_fraction can at most double the size of potential key reads (root + child) but I presume that's still a major improvement from what it is right now.

@nbaztec @ggwpez I applied it to the child nodes here and here. WDYT?

ggwpez · 2023-09-03T13:00:24Z

substrate/utils/frame/benchmarking-cli/src/storage/read.rs

@@ -44,12 +44,15 @@ impl StorageCmd {
 		let mut keys: Vec<_> = client.storage_keys(best_hash, None, None)?.collect();
 		let (mut rng, _) = new_rng(None);
 		keys.shuffle(&mut rng);
+		let number_of_keys = (keys.len() * self.params.db_fraction as usize) / 100;


It should probably error if the result is zero.

We could do something like:

let number_of_keys = if number_of_keys != 0 {number_of_keys} else {keys.len()};

If we have number_of_keys = 0, we use all the keys by default. Another option would be to set the minimum limit to 1. WDYT?

But all keys could also be empty afaik the check makes sense either way. Just returning an error is no problem.

If the number_of_keys is 0 that can mean either (or both) keys.len() or self.params.db_fraction was 0 , but self.params.db_fraction is restricted via clap arguments 1..=100 so it cannot be 0, in which case keys.iter().take(0) is equivalent to keys.iter() thereby keeping the old behavior in tact. I believe there's no special case to be handled here unless we're worried someone/something overriding db_fraction to 0 in which case that should be the thing being checked here.

What i am saying is that the old behaviour was probably also wrong.

substrate/utils/frame/benchmarking-cli/src/storage/cmd.rs

substrate/utils/frame/benchmarking-cli/src/storage/write.rs

ggwpez

Okay approving now. You can still add the !=0 check if you want, but its an edge-case anyway so i dont want to block it for that.

crystalin · 2023-09-06T10:36:49Z

I'd like to test it before getting it merged. One change that I could think of is to have the fraction be a per 1000th instead of percent. I'll verify it today

ggwpez · 2023-09-06T12:03:53Z

I'd like to test it before getting it merged. One change that I could think of is to have the fraction be a per 1000th instead of percent. I'll verify it today

Yes could be good for very large states. We can use a float percent or unit [0, 1].

crystalin · 2023-09-07T15:08:30Z

I tried on Moonbeam:

re-indexing took 20h (not sure why but also out of scope here)
storage benchmark, never finished (already 15h into it)

I'm not sure what is limiting it but it doesn't seem to use much CPU (2%). IOPS is at 1500 (6MB/s disk read) but nothing appears in the logs

ggwpez · 2023-09-12T09:51:10Z

Okay so its not working as intended. Probably because we first collect all keys with client.storage_keys(best_hash, None, None)?.collect().
I dont think its possible to request a random key from the DB @cheme, or? What we are trying to do is to run the benchmark only on x% of the KV pairs.

bkchr · 2024-01-22T12:10:42Z

Do we still want this pr @ggwpez?

ggwpez · 2024-01-22T12:47:40Z

It did not actually solve the issue, since the slow time comes from iterating through all keys to pick a random sub-set.
The DB backend does not allow to pick a random key, so its only marginally faster with this fix.

adelarja added 4 commits August 28, 2023 12:34

Added draft

5d3a3a3

Fixed log and variable name.

be60ff1

Added argument explanation to readme

3a46fc9

Set argument range.

4b417b0

adelarja and others added 4 commits August 28, 2023 22:04

Merge branch 'master' into benchmark-storage-arg-to-read-fraction-of-…

1b1f1c5

…kv-pairs

Corrected argument explanation.

c136491

Merge branch 'benchmark-storage-arg-to-read-fraction-of-kv-pairs' of …

d1b8666

…github.com:adelarja/polkadot-sdk into benchmark-storage-arg-to-read-fraction-of-kv-pairs

Removed unnecessary blank spaces

5977abc

Fixed issue with write method

ef7f296

ggwpez self-requested a review September 3, 2023 12:51

ggwpez reviewed Sep 3, 2023

View reviewed changes

adelarja added 2 commits September 3, 2023 16:47

removed unnecessary variable.

0839793

Added argument to test

1280ca7

ggwpez reviewed Sep 4, 2023

View reviewed changes

substrate/utils/frame/benchmarking-cli/src/storage/write.rs Outdated Show resolved Hide resolved

adelarja added 4 commits September 4, 2023 18:18

Replaced iter for into_iter in write method

b1876fc

Applied db-fraction to child-nodes too

96891c5

Fixed format

152be9d

Re-added a line that was accidentally removed

9e255eb

ggwpez approved these changes Sep 6, 2023

View reviewed changes

ggwpez closed this Jan 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Benchmark storage arg to read fraction of kv pairs. #1233

Benchmark storage arg to read fraction of kv pairs. #1233

adelarja commented Aug 29, 2023

cla-bot-2021 bot commented Aug 29, 2023

paritytech-cicd-pr commented Aug 29, 2023

ggwpez Sep 3, 2023

ggwpez Sep 5, 2023

nbaztec Sep 5, 2023

adelarja Sep 5, 2023

ggwpez Sep 3, 2023

adelarja Sep 3, 2023 •

edited

Loading

ggwpez Sep 3, 2023

nbaztec Sep 5, 2023

ggwpez Sep 5, 2023

ggwpez left a comment

crystalin commented Sep 6, 2023

ggwpez commented Sep 6, 2023 •

edited

Loading

crystalin commented Sep 7, 2023 •

edited

Loading

ggwpez commented Sep 12, 2023

bkchr commented Jan 22, 2024

ggwpez commented Jan 22, 2024

Benchmark storage arg to read fraction of kv pairs. #1233

Benchmark storage arg to read fraction of kv pairs. #1233

Conversation

adelarja commented Aug 29, 2023

cla-bot-2021 bot commented Aug 29, 2023

paritytech-cicd-pr commented Aug 29, 2023

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

adelarja Sep 3, 2023 • edited Loading

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

ggwpez left a comment

Choose a reason for hiding this comment

crystalin commented Sep 6, 2023

ggwpez commented Sep 6, 2023 • edited Loading

crystalin commented Sep 7, 2023 • edited Loading

ggwpez commented Sep 12, 2023

bkchr commented Jan 22, 2024

ggwpez commented Jan 22, 2024

adelarja Sep 3, 2023 •

edited

Loading

ggwpez commented Sep 6, 2023 •

edited

Loading

crystalin commented Sep 7, 2023 •

edited

Loading