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

Create script to refactor benchmarks v1 to v2 #2141

Open
0xmovses opened this issue Nov 2, 2023 · 3 comments
Open

Create script to refactor benchmarks v1 to v2 #2141

0xmovses opened this issue Nov 2, 2023 · 3 comments
Assignees
Labels
I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T10-tests This PR/Issue is related to tests. T12-benchmarks This PR/Issue is related to benchmarking and weights.

Comments

@0xmovses
Copy link
Contributor

0xmovses commented Nov 2, 2023

This came up in the FRAME weekly call today. We should create a script that will update all the benchmark v1 syntax and structure to benchmark v2. This would alleviate manual work and help the wider community.

the script would most likely use the syn crate and the quote! macro to parse tokens.

Some further inspiration (not for logic or functionality, but its use of syn) : https://github.com/sam0x17/macro_magic/blob/main/core_macros/src/lib.rs.

The finished script should be pushed to its own repo so that the wider network can make use of it. Creating this issue to track the progress of it as we can use it to refactor the benchmarks.

Example refactor #1868

@0xmovses 0xmovses added I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T10-tests This PR/Issue is related to tests. T12-benchmarks This PR/Issue is related to benchmarking and weights. labels Nov 2, 2023
@0xmovses 0xmovses changed the title Create script to refactor benchmark v1 to v2 Create script to refactor benchmarks v1 to v2 Nov 2, 2023
@0xmovses 0xmovses self-assigned this Nov 3, 2023
@kianenigma
Copy link
Contributor

kianenigma commented Nov 3, 2023

cool idea, but I must admit I think this is not anywhere near the top of priorities in my head. I think the existence of weights itself, lack of education and automation around it is the main challenge. I hardly think creating a script for this niche use case would be a big unblocker for anyone or anything. Happy to be challenged about it though :)

@0xmovses
Copy link
Contributor Author

0xmovses commented Nov 3, 2023

So in the call I brought up a minor issue I ran into when working on this PR #1982. Without going into too much detail, the issue was that I was slowed down in several instances when working on the tests which uses the old benchmark v1 syntax. I was slowed down as we don't get help from the compiler when writing test code inside the macro. We have to build and run the code to see simple errors (slow).

It happened enough times to make me see the big dev-ex benefit of the v2 syntax for developers working with substrate. Initially I suggested just rewriting these pallet benchmarks to v2 (quite easy), @ggwpez however suggested that writing a script to do this on bulk would be more beneficial.

As a side note, documentation still primarily uses the v1 syntax (scroll down a bit) https://docs.substrate.io/reference/how-to-guides/weights/add-benchmarks/ . I strongly prefer v2 and feel we should only write documentation examples using v2.

If I was a new developer considering substrate, the v1 benchmark macro would put me off a little. The v2 however, great 👍 And I would know that I'd be spending a lot of my time writing and working within these tests.

There may be more higher priority issues I'm happy to switch on to, but I feel that anything relating to testing pallet code is important for dev-ex onboarding new devs etc.

@0xmovses
Copy link
Contributor Author

So I worked on this tool last week, it works with single benchmark tests, but when working with many tests the tests fail, I would need to spend more time debugging and adding some new logic. For now moving onto other things, anyone is free to PR to it in the meantime.

https://github.com/0xmovses/substrate-benchmark-upgrader

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T10-tests This PR/Issue is related to tests. T12-benchmarks This PR/Issue is related to benchmarking and weights.
Projects
None yet
Development

No branches or pull requests

2 participants