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

Add ProofSizeExt to benchmarks #5257

Merged
merged 4 commits into from
Aug 7, 2024

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Aug 6, 2024

I propose to have ProofSizeExt available during benchmarking so we can improve the accuracy for extensions using it.

Another thing we could do is to also enable recording for the timing benchmark here:

Parachains will need to have recording enabled during import for reclaim, so we could enable it here and provide a flag --disable-proof-recording for scenarios where one does not want it. Happy to hear opinions about this.

@skunert skunert added the T12-benchmarks This PR/Issue is related to benchmarking and weights. label Aug 6, 2024
@skunert skunert requested review from ggwpez and a team August 6, 2024 11:32
@ggwpez
Copy link
Member

ggwpez commented Aug 6, 2024

Another thing we could do is to also enable recording for the timing benchmark here:

I think it is both needed since it uses the proof recorder from the BenchmarkingState? And that is None otherwise?
If the there is no big slowdown in timings then there is no issue.

@skunert
Copy link
Contributor Author

skunert commented Aug 6, 2024

I think it is both needed since it uses the proof recorder from the BenchmarkingState? And that is None otherwise?
If the there is no big slowdown in timings then there is no issue.

Yes. This is my impression as well. I added a CLI flag to revert to the old behaviour. By running benchmarks a few times on my machine I found that timing weight impact is around 3-9%. For parachains this is correct, but unnecessary for relay chains and solochains.

@skunert skunert enabled auto-merge August 7, 2024 07:19
@skunert skunert added this pull request to the merge queue Aug 7, 2024
Merged via the queue into master with commit dd48544 Aug 7, 2024
158 of 161 checks passed
@skunert skunert deleted the skunert/proof-size-ext-in-benchmarking branch August 7, 2024 08:22
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Aug 28, 2024
I propose to have `ProofSizeExt` available during benchmarking so we can
improve the accuracy for extensions using it.

Another thing we could do is to also enable recording for the timing
benchmark here:

https://github.com/paritytech/polkadot-sdk/blob/035211d707d0a74a2a768fd658160721f09d5b44/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs#L232

Parachains will need to have recording enabled during import for
reclaim, so we could enable it here and provide a flag
`--disable-proof-recording` for scenarios where one does not want it.
Happy to hear opinions about this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T12-benchmarks This PR/Issue is related to benchmarking and weights.
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

4 participants