Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Dont use benchmark range on constant functions #12456

Merged

Conversation

shawntabrizi
Copy link
Contributor

Trying to address the confusion here: https://substrate.stackexchange.com/questions/5249/why-would-benchmarking-a-simple-setter-has-a-linear-behaviour

These are constant functions, and should have constant weight.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Oct 9, 2022
@shawntabrizi shawntabrizi added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Oct 9, 2022
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Did you manually deploy this to the Substrate runtime to update the weights?

@shawntabrizi
Copy link
Contributor Author

Did you manually deploy this to the Substrate runtime to update the weights?

yes, did it manually, just to get a new file. The weight itself is not very important.

@ggwpez
Copy link
Member

ggwpez commented Oct 11, 2022

Did you manually deploy this to the Substrate runtime to update the weights?

yes, did it manually, just to get a new file. The weight itself is not very important.

We also never updated it. Could be that it never ran on reference hardware and therefore incorrectly detected a slope.

@shawntabrizi
Copy link
Contributor Author

Did you manually deploy this to the Substrate runtime to update the weights?

yes, did it manually, just to get a new file. The weight itself is not very important.

We also never updated it. Could be that it never ran on reference hardware and therefore incorrectly detected a slope.

I ran it locally, and it detected a very small slope of 1 ns.

@ggwpez ggwpez requested a review from koute October 12, 2022 10:08
Copy link
Contributor

@koute koute left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

It's probably not worth it to try and detect these kinds of constant functions and special-case them during weight generation; such tiny slopes should not make much difference in practice anyway, and there's always the possibility of getting it wrong.

Comment on lines 39 to 40
// This is the benchmark setup phase.
let value = 1000u32.into();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is supposed to be an example maybe it'd be a good idea to explicitly say that "the set_dummy always takes constant time hence we're hardcoding the value here" or something like that?

@shawntabrizi
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for 1600913

@ggwpez
Copy link
Member

ggwpez commented Oct 12, 2022

bot rebase

@paritytech-processbot
Copy link

Rebased

@ggwpez
Copy link
Member

ggwpez commented Oct 12, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit b324e51 into master Oct 12, 2022
@paritytech-processbot paritytech-processbot bot deleted the shawntabrizi-update-example-benchmarking branch October 12, 2022 16:32
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* dont use benchmark range on constant function

* update weights

* fix

* new weights

* Update frame/examples/basic/src/benchmarking.rs

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants