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

Idea: Reflections for WeightInfo functions #382

Open
ggwpez opened this issue Oct 29, 2022 · 3 comments
Open

Idea: Reflections for WeightInfo functions #382

ggwpez opened this issue Oct 29, 2022 · 3 comments
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request.

Comments

@ggwpez
Copy link
Member

ggwpez commented Oct 29, 2022

Currently we have the WeightInfo trait which exposes several weight functions in the form of:

pub trait WeightInfo {
	fn call_a() -> Weight;
	fn call_b() -> Weight;}

Sometimes you want to iterate over all weight functions and do something weight their name or result (eg in paritytech/substrate#12485).

One way to do this would be to have another type; WeightMetaInfo whose implementation is auto-generated as well:

pub trait WeighMetatInfo<W: WeightInfo> {
	/// Executes a callback for each weight function by passing in its name and return value.
	fn visit_weight_functions<F: Fn(&'static str, Weight) -> R, R>(f: F) -> Vec<R>;
}

This would allow us to do:

  • Mocked WeigtInfo implementations such that the weight of a call can be dynamically set in a test (Is already possible, but not in a spell-checked way without ugly macros).
  • Checking that all weights of a pallet are in a specific bound as part of the runtime-integrity check (eg. no Zero or stray todo!()).

I am not entirely convinced that this is the best design, but having this in some way would be nice.

@tifecool
Copy link

tifecool commented Nov 5, 2022

I'm interested in picking this up, but not sure I quite get the proposal.

From my understanding, something like this is what's being asked for:

/// Struct containing names of weight functions and their results
pub struct WeightMetaInfo{
	names: Vec<&str>,
	results: Vec<Weight>
}

impl WeightInfo{
	
	/// Gets `WeightMetaInfo`
	fn get_weight_meta_info() -> WeightMetaInfo{
		//code
	}
} 

Or a struct containing a hashmap

@xlc
Copy link
Contributor

xlc commented Nov 5, 2022

One way could refactor out all the code generation logic. Instead of generate codes that hard to be consumed by other tools, it should generate data.

e.g.

trait GetWeightInfoData {
	fn weight_data(call: Call) -> WeightInfoData;
	fn weight<const S: usize>(call: Call, args: [u32: S]) -> Weight {
		// do the math
	}
}

struct WeightData {
	weight: Weight,
	reads: u64,
	writes: u64,
}

struct WeightInfoData {
	base: WeightData,
	args: Vec<WeightData>,
}

enum Call {
	AddRegistrar,
	SetIdentity,
}

impl GetWeightInfoData for Call {
	fn weight_data(call: Call) -> WeightInfoData {
		match call {
			Call::AddRegistrar => WeightInfoData {
				base: WeightData {
					weight: Weight::from_ref_time(16_649_000 as u64),
					reads: 1,
					writes: 1,
				},
				args: vec![
					WeightData {
						weight: Weight::from_ref_time(241_000 as u64),
						reads: 0,
						writes: 0,
					}
				],
			}
			Call::SetIdentity => WeightInfoData {
				base: WeightData {
					weight: Weight::from_ref_time(31_322_000 as u64),
					reads: 1,
					writes: 1,
				},
				args: vec![
					WeightData {
						weight: Weight::from_ref_time(252_000 as u64),
						reads: 0,
						writes: 0,
					},
					WeightData {
						weight: Weight::from_ref_time(312_000 as u64),
						reads: 0,
						writes: 0,
					}
				],
			}
		}
	}
}

In that way it is easy to work with WeightInfoData and do whatever check. Could do some derive on the enum Call to be able to iterate all the calls.

@ggwpez
Copy link
Member Author

ggwpez commented Nov 8, 2022

Thanks for the ideas @xlc. Using data here instead of math terms should make it easier downstream indeed.
Could be extended to take the correct component ranges into account, instead of just u32.
The only problem I see is that pallet Call does not correspond 1:1 to benchmark functions. Sometimes we use a .max in a pallet::weight annotation or something.
Also I would like to have some for_all_weight_functions! macro that can be used for downstream code generation per benchmark function. So that a MockedWeightInfo could be macro-generated.

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I5-enhancement An additional feature request. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. and removed J0-enhancement labels Aug 25, 2023
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
jonathanudd pushed a commit to jonathanudd/polkadot-sdk that referenced this issue Apr 10, 2024
* Change script to update versions.

* Bump versions.

* Address remainders.

* cargo fmt --all

* Fix tests.

* Whitelist BlueOak license

* Fix benchmarks?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request.
Projects
Development

No branches or pull requests

4 participants