Cleanup String::from_utf8 by philoniare · Pull Request #3446 · paritytech/polkadot-sdk

philoniare · 2024-02-22T15:55:56Z

Description

*Refactors String::from_utf8 usage in the pallet benchmarking

Fixes #389

bkchr · 2024-02-25T21:58:04Z

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

+		for (pallet, extrinsic, components, _, pallet_str, extrinsic_str) in benchmarks_to_run.clone() {
 			log::info!(
 				target: LOG_TARGET,
 				"Starting benchmark: {}::{}",


Suggested change

"Starting benchmark: {}::{}",

"Starting benchmark: {pallet_str}::{extrinsic_str}",

bkchr · 2024-02-25T21:58:16Z

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

+				pallet_str,
+				extrinsic_str,


Suggested change

pallet_str,

extrinsic_str,

bkchr · 2024-02-25T21:58:43Z

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

@@ -417,8 +423,8 @@ impl PalletCmd {
 						.map_err(|e| {
 							format!(
 								"Benchmark {}::{} failed: {}",


Suggested change

"Benchmark {}::{} failed: {}",

"Benchmark {pallet_str}::{extrinsic_str} failed: {e}",

bkchr · 2024-02-25T21:58:56Z

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

+								pallet_str,
+								extrinsic_str,
 								e


Suggested change

pallet_str,

extrinsic_str,

e

bkchr · 2024-02-25T21:59:30Z

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

@@ -495,10 +501,8 @@ impl PalletCmd {
 							log::info!(
 								target: LOG_TARGET,
 								"Running  benchmark: {}.{}({} args) {}/{} {}/{}",


Suggested change

"Running benchmark: {}.{}({} args) {}/{} {}/{}",

"Running benchmark: {pallet_str}.{extrinsic_str}({} args) {}/{} {}/{}",

bkchr · 2024-02-25T21:59:40Z

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

+								pallet_str,
+								extrinsic_str,


Suggested change

pallet_str,

extrinsic_str,

bkchr · 2024-02-25T22:01:08Z

substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs

-								let item = String::from_utf8(key_info.storage_name.clone())
-									.expect("encoded from string");
 								let comment = format!(
 									"Proof: `{}::{}` (`max_values`: {:?}, `max_size`: {:?}, mode: `{:?}`)",


Suggested change

"Proof: `{}::{}` (`max_values`: {:?}, `max_size`: {:?}, mode: `{:?}`)",

"Proof: `{pallet_name}::{storage_name}` (`max_values`: {:?}, `max_size`: {:?}, mode: `{:?}`)",

bkchr · 2024-02-25T22:01:11Z

substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs

 								let comment = format!(
 									"Proof: `{}::{}` (`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,


Suggested change

pallet_name, storage_name, key_info.max_values, key_info.max_size,

key_info.max_values, key_info.max_size,

davxy · 2024-02-26T06:57:51Z

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

+				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()


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?)

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

philoniare · 2024-02-26T10:15:34Z

Thanks for the review @bkchr, all of the comments have been addressed

ggwpez · 2024-02-26T11:50:13Z

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

 		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() {


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.

ggwpez

Thanks!

ggwpez · 2024-02-26T11:50:56Z

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

-									.expect("Encoded from String; qed"),
-								String::from_utf8(extrinsic.clone())
-									.expect("Encoded from String; qed"),
+								"Running  benchmark: {pallet_str}.{extrinsic_str}({} args) {}/{} {}/{}",


Suggested change

"Running benchmark: {pallet_str}.{extrinsic_str}({} args) {}/{} {}/{}",

"Running benchmark: {pallet_str}::{extrinsic_str}({} args) {}/{} {}/{}",

Noticed recently that it should be consistent with the print above.

ggwpez · 2024-02-26T11:52:14Z

substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs

-		let pallet_string = String::from_utf8(batch.pallet.clone()).unwrap();
-		let instance_string = String::from_utf8(batch.instance.clone()).unwrap();
+		let pallet_name = String::from_utf8(batch.pallet.clone()).unwrap();
+		let instance_name = String::from_utf8(batch.instance.clone()).unwrap();


Originally i thought that already the types in BenchmarkBatchSplitResults and BenchmarkBatch should be converted, but it would mean to duplicate or generitize the types.
Guess this is a good start for now.

ggwpez · 2024-02-26T14:18:13Z

bot fmt

command-bot · 2024-02-26T14:18:18Z

@ggwpez https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5348374 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-44b231b3-8b81-4a68-a363-f82dc97114cb to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-02-26T14:20:33Z

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5348374 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5348374/artifacts/download.

…head-data * origin/master: (51 commits) Runtime Upgrade ref docs and Single Block Migration example pallet (#1554) Collator overseer builder unification (#3335) Introduce storage attr macro `#[disable_try_decode_storage]` and set it on `System::Events` and `ParachainSystem::HostConfiguration` (#3454) Add Polkadotters bootnoders per IBP application (#3423) Add documentation around FRAME Origin (#3362) Bridge zombienet tests: Check amount received at destination (#3490) Snowbridge benchmark tests fix (#3424) fix(zombienet): increase timeout in download artifacts (#3376) Cleanup String::from_utf8 (#3446) [prdoc] Validate crate names (#3467) Limit max execution time for `test-linux-stable` CI jobs (#3483) Introduce Notification block pinning limit (#2935) frame-support: Improve error reporting when having too many pallets (#3478) add Encointer as trusted teleporter for Westend (#3411) [pallet-xcm] Adjust benchmarks (teleport_assets/reserve_transfer_assets) not relying on ED (#3464) Add more debug logs to understand if statement-distribution misbehaving (#3419) Remove redundant parachains assigner pallet. (#3457) Use generic hash for runtime wasm in resolve_state_version_from_wasm (#3447) Runtime: allow backing multiple candidates of same parachain on different cores (#3231) Bridge zombienet tests: move all "framework" files under one folder (#3462) ...

# Description *Refactors `String::from_utf8` usage in the pallet benchmarking Fixes paritytech#389 --------- Co-authored-by: command-bot <>

philoniare added 4 commits February 22, 2024 23:53

feat: cleanup String::from_utf8

bda08b5

Merge branch 'master' into cleanup-from_utf8

c4ada60

feat: refactor new updates

1b945d1

Merge branch 'master' into cleanup-from_utf8

cebbaa6

philoniare marked this pull request as ready for review February 24, 2024 02:01

bkchr approved these changes Feb 25, 2024

View reviewed changes

bkchr added the R0-no-crate-publish-required The change does not require any crates to be re-published. label Feb 25, 2024

bkchr requested a review from ggwpez February 25, 2024 22:02

davxy approved these changes Feb 26, 2024

View reviewed changes

philoniare added 2 commits February 26, 2024 18:13

feat: refactor due to review

96ec635

Merge branch 'master' into cleanup-from_utf8

48c0cc6

Merge branch 'master' into cleanup-from_utf8

6256d3b

ggwpez reviewed Feb 26, 2024

View reviewed changes

ggwpez approved these changes Feb 26, 2024

View reviewed changes

philoniare added 2 commits February 26, 2024 20:27

feat: rename vars to exclude type

3e9899a

Merge branch 'master' into cleanup-from_utf8

ac489d4

".git/.scripts/commands/fmt/fmt.sh"

855e81f

ggwpez added this pull request to the merge queue Feb 26, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 26, 2024

ggwpez added this pull request to the merge queue Feb 26, 2024

Merged via the queue into paritytech:master with commit 81d447a Feb 26, 2024

	"Starting benchmark: {}::{}",
	"Starting benchmark: {pallet_str}::{extrinsic_str}",

	"Benchmark {}::{} failed: {}",
	"Benchmark {pallet_str}::{extrinsic_str} failed: {e}",

	"Running benchmark: {}.{}({} args) {}/{} {}/{}",
	"Running benchmark: {pallet_str}.{extrinsic_str}({} args) {}/{} {}/{}",

	"Proof: `{}::{}` (`max_values`: {:?}, `max_size`: {:?}, mode: `{:?}`)",
	"Proof: `{pallet_name}::{storage_name}` (`max_values`: {:?}, `max_size`: {:?}, mode: `{:?}`)",

	pallet_name, storage_name, key_info.max_values, key_info.max_size,
	key_info.max_values, key_info.max_size,

	for (pallet, extrinsic, components, _, pallet_str, extrinsic_str) in benchmarks_to_run.clone() {
	for (pallet, extrinsic, components, _, pallet, extrinsic) in benchmarks_to_run.clone() {

	"Running benchmark: {pallet_str}.{extrinsic_str}({} args) {}/{} {}/{}",
	"Running benchmark: {pallet_str}::{extrinsic_str}({} args) {}/{} {}/{}",

Conversation

philoniare commented Feb 22, 2024

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philoniare commented Feb 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez commented Feb 26, 2024

Uh oh!

command-bot bot commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Feb 26, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

command-bot bot commented Feb 26, 2024 •

edited

Loading