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

Cleanup String::from_utf8 #3446

Merged
merged 10 commits into from
Feb 26, 2024
52 changes: 30 additions & 22 deletions substrate/utils/frame/benchmarking-cli/src/pallet/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,16 +298,22 @@ impl PalletCmd {
// Convert `Vec<u8>` to `String` for better readability.
let benchmarks_to_run: Vec<_> = benchmarks_to_run
.into_iter()
.map(|b| {
.map(|(pallet, extrinsic, components, pov_modes)| {
let pallet_str = String::from_utf8(pallet.clone()).expect("Encoded from String; qed");
let extrinsic_str = String::from_utf8(extrinsic.clone()).expect("Encoded from String; qed");
let pov_modes_str = pov_modes.into_iter()
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick. In general we don't use Hungarian notation or any variation of it. So in this case I'd rename these variables to just pallet_name, extrinsic_name and pov_modes (as done some files below - writer.rs?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, sure thing. I've updated the variable names for pallet_str, extrinsic_str and removed pov_modes_str as pov_modes was already being passed in

.map(|(p, s)| {
(String::from_utf8(p).unwrap(), String::from_utf8(s).unwrap())
})
.collect();

(
b.0,
b.1,
b.2,
b.3.into_iter()
.map(|(p, s)| {
(String::from_utf8(p).unwrap(), String::from_utf8(s).unwrap())
})
.collect(),
pallet,
extrinsic,
components,
pov_modes_str,
pallet_str,
extrinsic_str,
)
})
.collect();
Expand All @@ -329,12 +335,12 @@ impl PalletCmd {
let mut component_ranges = HashMap::<(Vec<u8>, Vec<u8>), Vec<ComponentRange>>::new();
let pov_modes = Self::parse_pov_modes(&benchmarks_to_run)?;

for (pallet, extrinsic, components, _) in benchmarks_to_run.clone() {
for (pallet, extrinsic, components, _, pallet_str, extrinsic_str) in benchmarks_to_run.clone() {
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
for (pallet, extrinsic, components, _, pallet_str, extrinsic_str) in benchmarks_to_run.clone() {
for (pallet, extrinsic, components, _, pallet, extrinsic) in benchmarks_to_run.clone() {

Types dont need to be in the var name.

log::info!(
target: LOG_TARGET,
"Starting benchmark: {}::{}",
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
"Starting benchmark: {}::{}",
"Starting benchmark: {pallet_str}::{extrinsic_str}",

String::from_utf8(pallet.clone()).expect("Encoded from String; qed"),
String::from_utf8(extrinsic.clone()).expect("Encoded from String; qed"),
pallet_str,
extrinsic_str,
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
pallet_str,
extrinsic_str,

);
let all_components = if components.is_empty() {
vec![Default::default()]
Expand Down Expand Up @@ -417,8 +423,8 @@ impl PalletCmd {
.map_err(|e| {
format!(
"Benchmark {}::{} failed: {}",
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
"Benchmark {}::{} failed: {}",
"Benchmark {pallet_str}::{extrinsic_str} failed: {e}",

String::from_utf8_lossy(&pallet),
String::from_utf8_lossy(&extrinsic),
pallet_str,
extrinsic_str,
e
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
pallet_str,
extrinsic_str,
e

)
})?;
Expand Down Expand Up @@ -495,10 +501,8 @@ impl PalletCmd {
log::info!(
target: LOG_TARGET,
"Running benchmark: {}.{}({} args) {}/{} {}/{}",
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
"Running benchmark: {}.{}({} args) {}/{} {}/{}",
"Running benchmark: {pallet_str}.{extrinsic_str}({} args) {}/{} {}/{}",

String::from_utf8(pallet.clone())
.expect("Encoded from String; qed"),
String::from_utf8(extrinsic.clone())
.expect("Encoded from String; qed"),
pallet_str,
extrinsic_str,
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
pallet_str,
extrinsic_str,

components.len(),
s + 1, // s starts at 0.
all_components.len(),
Expand Down Expand Up @@ -707,12 +711,14 @@ impl PalletCmd {
Vec<u8>,
Vec<(BenchmarkParameter, u32, u32)>,
Vec<(String, String)>,
String,
String,
)>,
) -> Result<PovModesMap> {
use std::collections::hash_map::Entry;
let mut parsed = PovModesMap::new();

for (pallet, call, _components, pov_modes) in benchmarks {
for (pallet, call, _components, pov_modes, _, _) in benchmarks {
for (pallet_storage, mode) in pov_modes {
let mode = PovEstimationMode::from_str(&mode)?;
let splits = pallet_storage.split("::").collect::<Vec<_>>();
Expand Down Expand Up @@ -766,18 +772,20 @@ fn list_benchmark(
Vec<u8>,
Vec<(BenchmarkParameter, u32, u32)>,
Vec<(String, String)>,
String,
String,
)>,
list_output: ListOutput,
no_csv_header: bool,
) {
let mut benchmarks = BTreeMap::new();

// Sort and de-dub by pallet and function name.
benchmarks_to_run.iter().for_each(|(pallet, extrinsic, _, _)| {
benchmarks_to_run.iter().for_each(|(_, _, _, _, pallet_str, extrinsic_str)| {
benchmarks
.entry(String::from_utf8_lossy(pallet).to_string())
.entry(pallet_str)
.or_insert_with(BTreeSet::new)
.insert(String::from_utf8_lossy(extrinsic).to_string());
.insert(extrinsic_str);
});

match list_output {
Expand Down
39 changes: 17 additions & 22 deletions substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -571,19 +571,22 @@ pub(crate) fn process_storage_results(

let mut prefix_result = result.clone();
let key_info = storage_info_map.get(&prefix);
let pallet_name = match key_info {
Some(k) => String::from_utf8(k.pallet_name.clone()).expect("encoded from string"),
None => "".to_string(),
};
let storage_name = match key_info {
Some(k) => String::from_utf8(k.storage_name.clone()).expect("encoded from string"),
None => "".to_string(),
};
let max_size = key_info.and_then(|k| k.max_size);

let override_pov_mode = match key_info {
Some(StorageInfo { pallet_name, storage_name, .. }) => {
let pallet_name =
String::from_utf8(pallet_name.clone()).expect("encoded from string");
let storage_name =
String::from_utf8(storage_name.clone()).expect("encoded from string");

Some(_) => {
// Is there an override for the storage key?
pov_modes.get(&(pallet_name.clone(), storage_name)).or(
pov_modes.get(&(pallet_name.clone(), storage_name.clone())).or(
// .. or for the storage prefix?
pov_modes.get(&(pallet_name, "ALL".to_string())).or(
pov_modes.get(&(pallet_name.clone(), "ALL".to_string())).or(
// .. or for the benchmark?
pov_modes.get(&("ALL".to_string(), "ALL".to_string())),
),
Expand Down Expand Up @@ -662,13 +665,11 @@ pub(crate) fn process_storage_results(
// writes.
if !is_prefix_identified {
match key_info {
Some(key_info) => {
Some(_) => {
let comment = format!(
"Storage: `{}::{}` (r:{} w:{})",
String::from_utf8(key_info.pallet_name.clone())
.expect("encoded from string"),
String::from_utf8(key_info.storage_name.clone())
.expect("encoded from string"),
pallet_name,
storage_name,
reads,
writes,
);
Expand Down Expand Up @@ -699,10 +700,8 @@ pub(crate) fn process_storage_results(
Some(new_pov) => {
let comment = format!(
"Proof: `{}::{}` (`max_values`: {:?}, `max_size`: {:?}, added: {}, mode: `{:?}`)",
String::from_utf8(key_info.pallet_name.clone())
.expect("encoded from string"),
String::from_utf8(key_info.storage_name.clone())
.expect("encoded from string"),
pallet_name,
storage_name,
key_info.max_values,
key_info.max_size,
new_pov,
Expand All @@ -711,13 +710,9 @@ pub(crate) fn process_storage_results(
comments.push(comment)
},
None => {
let pallet = String::from_utf8(key_info.pallet_name.clone())
.expect("encoded from string");
let item = String::from_utf8(key_info.storage_name.clone())
.expect("encoded from string");
let comment = format!(
"Proof: `{}::{}` (`max_values`: {:?}, `max_size`: {:?}, mode: `{:?}`)",
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
"Proof: `{}::{}` (`max_values`: {:?}, `max_size`: {:?}, mode: `{:?}`)",
"Proof: `{pallet_name}::{storage_name}` (`max_values`: {:?}, `max_size`: {:?}, mode: `{:?}`)",

pallet, item, key_info.max_values, key_info.max_size,
pallet_name, storage_name, key_info.max_values, key_info.max_size,
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
pallet_name, storage_name, key_info.max_values, key_info.max_size,
key_info.max_values, key_info.max_size,

used_pov_mode,
);
comments.push(comment);
Expand Down
Loading