Updates Benchmark macros to use linear! with range#13897
Updates Benchmark macros to use linear! with range#13897
Conversation
sam0x17
left a comment
sam0x17
left a comment
There was a problem hiding this comment.
looking great, like the linear! syntax as a solution to the ranges not being valid in type position issue, would love to hear @ggwpez's thoughts on this
I do think we still need some sort of static assertion to make sure start and end are Into
|
|
||
| // compile-time assertions that each referenced param type implements ParamRange | ||
| #( | ||
| #home::assert_impl_all!(#param_types: #home::ParamRange); |
There was a problem hiding this comment.
so let's replace this with a compile-time assertion that start and end are both bounded by Into<u32>
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
The CI pipeline was cancelled due to failure one of the required jobs. |
| // trie and PoV size. | ||
| #[benchmark(extra)] | ||
| fn transfer_increasing_users(u: Linear<0, 1_000>) { | ||
| fn transfer_increasing_users(u: linear![0..1_000]) { |
There was a problem hiding this comment.
Why not keep Linear<0..1_000>?
Since const generics cannot accept ranges or something?
There was a problem hiding this comment.
I think the advantage of having a type is that people can click on it in the language server and it explains what it does.
There was a problem hiding this comment.
the reason for the fake macro is rust isn't able to parse the enclosing module as an ItemMod if we do Linear<0..10> because the 0..10 can't appear in type position, so we would either have to do some kind of pre-pass where we remove these before parsing as an ItemMod (hard) or enclose the range in something that works in any position like a macro call
There was a problem hiding this comment.
So based on those trade-offs, I like this
There was a problem hiding this comment.
The whole root of this issue is if we are supporting things that are Into<u32> instead of just u32 literals, this is no longer really a type, it's an expression, because the info we need to collect from the programmer is an expression for the start of the range and an expression for the end of the range. Before we were abusing const u32 type params to do this, but we can't do that with things that are merely Into the same way
There was a problem hiding this comment.
maybe something with a tuple though......? This was just the best idea that I had syntactically
There was a problem hiding this comment.
Well, we can use Into<u32> but only with the Linear<start, end> syntax and not ….
I dont see anything wrong with Linear<start, end> besides the fact that it is "less" rust.
On the other hand the start…end is also not quite right in the rust semantic, since we just skip specific values depending on the step size. So it is not actually a range in the rust sense.
There was a problem hiding this comment.
@gupnik does ItemMod still parse if you do Linear<something, something_else> where something is (non-const) expr? I was under the impression it doesn't let you but I forget now
My whole complaint @ggwpez is that Linear<something.something(), 33> is no longer a type anyway so why not make it look like a range
There was a problem hiding this comment.
But it is still a valid rust type; just needs brackets: Linear<{ something.something() }, 33>. Everything we do here must be compile time anyway, but that also applies to the V1 syntax.
This works just fine:
It needs GenericArgument though, see diff.
There was a problem hiding this comment.
Interesting! Thanks @ggwpez. Let me take this up.
|
Ooh I didn’t know about the brace thing so that’s what those are
…On Thu, Apr 13, 2023 at 12:16 PM Oliver Tale-Yazdi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In frame/balances/src/benchmarking.rs
<#13897 (comment)>
:
> - fn transfer_increasing_users(u: Linear<0, 1_000>) {
+ fn transfer_increasing_users(u: linear![0..1_000]) {
But it is still a valid rust type; just needs brackets: Linear<{
something.something() }, 33>. Everything we do here must be compile time
anyway, but that also applies to the V1 syntax.
This works just fine:
https://github.com/paritytech/substrate/blob/oty-demo-bench-syntax/frame/examples/basic/src/benchmarking.rs#L105
—
Reply to this email directly, view it on GitHub
<#13897 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOE4LNBXQYVIVEI2LBEKYDXBARFJANCNFSM6AAAAAAW4SRMCA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
5 participants
As discussed in #13845