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

frame/benchmarking: baseline weights independent of complexity params #400

Open
ggwpez opened this issue Jan 11, 2022 · 4 comments
Open
Assignees
Labels
D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T10-tests This PR/Issue is related to tests.

Comments

@ggwpez
Copy link
Member

ggwpez commented Jan 11, 2022

The baseline benchmarks addition, subtraction, multiplication and division all ignore their complexity parameter and have a constant weight.
The benchmarks should be fixed to either not accept a parameter or have a weight that depends on it.

@ggwpez ggwpez self-assigned this Jan 11, 2022
@dharjeezy
Copy link
Contributor

Hello @ggwpez I am interested in picking this issue up.
I understand when you say the benchmark should not accept a parameter, that means we should remove the i parameter since it's not being used yh?

But I don't understand what you mean by "should have a weight that depends on it"?

@ggwpez
Copy link
Member Author

ggwpez commented Jan 12, 2022

Hello @ggwpez I am interested in picking this issue up.

Wow nice @dharjeezy !

I understand when you say the benchmark should not accept a parameter, that means we should remove the i parameter since it's not being used yh?

Yes it could just be removed and run with a constant repetition, eg 1_000_000.

But I don't understand what you mean by "should have a weight that depends on it"?

That would just be an alternative approach; instead of removing it you could also fix it so that the weight output uses the i parameter. It currently looks like this:

fn addition(_i: u32, ) -> Weight {
	(284_000 as Weight)
}

but could look more like this:

fn addition(i: u32, ) -> Weight {
	(1_000 as Weight)
		.saturating_add((10_000 as Weight).saturating_mul(i as Weight))
}

IMHO it would be the best to just remove the parameter, makes it simpler to use and understand.
Thinking again: The second approach would guarantee that the compiler does not optimize out the operation. Would be difficult to otherwise verify this.
I talked about it with @shawntabrizi and I think he has no preference which approach to use.
So it would be up to you @dharjeezy which way to go.

@ggwpez ggwpez assigned dharjeezy and unassigned ggwpez Jan 12, 2022
@shawntabrizi
Copy link
Member

The benchmarks should be fixed to either not accept a parameter or have a weight that depends on it.

I dont think it is clear that we can "force the benchmark" to use the i parameter. The i parameter not being used is a result of the benchmarking results undergoing linear regression, and the regression not finding any noticeable slope.

But yeah, in any case, it is not actually that important what precisely we are benchmarking, just that the benchmarks are universally consistent, as to tell when changes to the rust compiler or hardware are making an impact to the timing of the weights.

@ggwpez
Copy link
Member Author

ggwpez commented Jan 12, 2022

I dont think it is clear that we can "force the benchmark" to use the i parameter. The i parameter not being used is a result of the benchmarking results undergoing linear regression, and the regression not finding any noticeable slope.

We could execute batches of operations, eventually it should show up.
So (0..i * 1000) instead of (0..i).

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added T10-tests This PR/Issue is related to tests. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. and removed I5-tests labels Aug 25, 2023
claravanstaden pushed a commit to Snowfork/polkadot-sdk that referenced this issue Dec 8, 2023
helin6 pushed a commit to boolnetwork/polkadot-sdk that referenced this issue Feb 5, 2024
Bumps [dns-packet](https://github.com/mafintosh/dns-packet) from 1.3.1 to 1.3.4.
- [Release notes](https://github.com/mafintosh/dns-packet/releases)
- [Changelog](https://github.com/mafintosh/dns-packet/blob/master/CHANGELOG.md)
- [Commits](mafintosh/dns-packet@v1.3.1...v1.3.4)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
* message lane metrics

* fmt and clippy
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
* message lane metrics

* fmt and clippy
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* message lane metrics

* fmt and clippy
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* message lane metrics

* fmt and clippy
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* message lane metrics

* fmt and clippy
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* message lane metrics

* fmt and clippy
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* message lane metrics

* fmt and clippy
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* message lane metrics

* fmt and clippy
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
* message lane metrics

* fmt and clippy
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
* message lane metrics

* fmt and clippy
jonathanudd pushed a commit to jonathanudd/polkadot-sdk that referenced this issue Apr 10, 2024
* message lane metrics

* fmt and clippy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T10-tests This PR/Issue is related to tests.
Projects
Development

No branches or pull requests

4 participants