-
Notifications
You must be signed in to change notification settings - Fork 645
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
Changes from 4 commits
bda08b5
c4ada60
1b945d1
cebbaa6
96ec635
48c0cc6
6256d3b
3e9899a
ac489d4
855e81f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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() | ||||||||
.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(); | ||||||||
|
@@ -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() { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Types dont need to be in the var name. |
||||||||
log::info!( | ||||||||
target: LOG_TARGET, | ||||||||
"Starting benchmark: {}::{}", | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
String::from_utf8(pallet.clone()).expect("Encoded from String; qed"), | ||||||||
String::from_utf8(extrinsic.clone()).expect("Encoded from String; qed"), | ||||||||
pallet_str, | ||||||||
extrinsic_str, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
); | ||||||||
let all_components = if components.is_empty() { | ||||||||
vec![Default::default()] | ||||||||
|
@@ -417,8 +423,8 @@ impl PalletCmd { | |||||||
.map_err(|e| { | ||||||||
format!( | ||||||||
"Benchmark {}::{} failed: {}", | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
String::from_utf8_lossy(&pallet), | ||||||||
String::from_utf8_lossy(&extrinsic), | ||||||||
pallet_str, | ||||||||
extrinsic_str, | ||||||||
e | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
) | ||||||||
})?; | ||||||||
|
@@ -495,10 +501,8 @@ impl PalletCmd { | |||||||
log::info!( | ||||||||
target: LOG_TARGET, | ||||||||
"Running benchmark: {}.{}({} args) {}/{} {}/{}", | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
String::from_utf8(pallet.clone()) | ||||||||
.expect("Encoded from String; qed"), | ||||||||
String::from_utf8(extrinsic.clone()) | ||||||||
.expect("Encoded from String; qed"), | ||||||||
pallet_str, | ||||||||
extrinsic_str, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
components.len(), | ||||||||
s + 1, // s starts at 0. | ||||||||
all_components.len(), | ||||||||
|
@@ -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<_>>(); | ||||||||
|
@@ -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 { | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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())), | ||||||
), | ||||||
|
@@ -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, | ||||||
); | ||||||
|
@@ -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, | ||||||
|
@@ -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: `{:?}`)", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
pallet, item, key_info.max_values, key_info.max_size, | ||||||
pallet_name, storage_name, key_info.max_values, key_info.max_size, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
used_pov_mode, | ||||||
); | ||||||
comments.push(comment); | ||||||
|
There was a problem hiding this comment.
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
andpov_modes
(as done some files below - writer.rs?)There was a problem hiding this comment.
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 removedpov_modes_str
aspov_modes
was already being passed in