Bench for Move based transfer with Gas

[oxade]

Extending benchmark to measure Move based mass-transfers using the Gas module.
Now that we have two paths for executing transactions (Rust and Move), it's imperative that we measure the differences in performance.
This PR adds the option to exercise the Move transfer the same way we exercise the native Rust transfer.
It also incorporates module caching as this was a significant bottleneck in Adapter flow: [fastx adapter] Cache the VM to amortise module verification #192

Usage for move:
./bench --use-move {other args}
Without move (native Rust):
./bench {other args}

Results: Move vs Native (lower% is better)
Orders Only
38k vs 45k => 15.5%
Certs Only
19k vs 23k => 17%
Orders And Certs
12.5k vs 15k => 17%

[sblackshear]

We probably shouldn't use ObjectBasics for this because it is test code that will soon be moved out of the framework (this was finally unblocked by e51bc66).

Instead, I'd recommend adding a callable version of Coin::transfer (e.g.,

public fun transfer<T>(c: Coin<T>, recipient: vector<u8>, _ctx: TxContext) {
  Transfer::transfer(c, Address:new(recipient))
}

But I recall that @lxfind had some concerns about exposing this as an entrypoint--is that right Xun?

[oxade]

We probably shouldn't use ObjectBasics for this because it is test code that will soon be moved out of the framework (this was finally unblocked by e51bc66).

Instead, I'd recommend adding a callable version of Coin::transfer (e.g.,

public fun transfer<T>(c: Coin<T>, recipient: vector<u8>, _ctx: TxContext) {
  Transfer::transfer(c, Address:new(recipient))
}

But I recall that @lxfind had some concerns about exposing this as an entrypoint--is that right Xun?

Ah I did not know ObjectBasics was temporary. Will try Coin::transfer

[lxfind]

Probably should add a new bench type for Move calls instead of adding use_move. I imagine we want to test the performance a few different primitives. Whether or not to enable move also doesn't matter much without the confirmation stage which does the execution.

As for transfer, I think adding a transfer function to Coin makes sense. I was more concerned exposing a generic transfer inside Transfer module.

[sblackshear]

As for transfer, I think adding a transfer function to Coin makes sense. I was more concerned exposing a generic transfer inside Transfer module.

Gotcha--sounds like we're on the same page!

[oxade]

Probably should add a new bench type for Move calls instead of adding use_move. I imagine we want to test the performance a few different primitives. Whether or not to enable move also doesn't matter much without the confirmation stage which does the execution.

As for transfer, I think adding a transfer function to Coin makes sense. I was more concerned exposing a generic transfer inside Transfer module.

You can specify whether to measure Order or Confirmation stages or both (default is both) with --benchmark-type
But yes overall I will define a more extensive benchmark for Move calls. This is the first step that's as similar to the existing Rust impl for quick comparisons

[oxade]

@lxfind
I actually noticed a considerable dip in performance during the Order stage with Move orders, so I reckon we keep monitoring it

[lxfind]

@lxfind I actually noticed a considerable dip in performance during the Order stage with Move orders, so I reckon we keep monitoring it

Wow good catch!

[lxfind]

I am guessing it's because we currently deserialize the modules to get the id. Easy win opportunity.

[oxade]

I am guessing it's because we currently deserialize the modules to get the id. Easy win opportunity.

Very likely. We should dig deeper

[gdanezis]

I am guessing it's because we currently deserialize the modules to get the id. Easy win opportunity.

Very likely. We should dig deeper

Yes, that is a very good catch -- we should not see such a dip at the order stage. However, my main target for optimisation is certs at this point.

[oxade]

@sblackshear
When moving over to Gas (over ObjectBasics), I was not able to get any function to work that had a signature taking generics.
For example something like this would not work because the signature violates the rules in the adapter (regardless of if I specified the type args)

public fun transfer<T>(c: Coin<T>, recipient: vector<u8>, _ctx: TxContext) {
  Transfer::transfer(c, Address:new(recipient))
}

Function signature violation occurred here https://github.com/MystenLabs/fastnft/blob/main/fastx_programmability/adapter/src/adapter.rs#L368

So the current solution uses a wrapper in GAS.move. Let me know the better way to do this from Coin.move or with generics

[oxade]

@sblackshear specifying type_args did not make a difference here so I committed them.
However I reckon they might be useful when calling a generic fn

[sblackshear]

We will need them eventually, but punting on them until we use Coin::transfer seems reasonable.

[sblackshear]

@sblackshear When moving over to Gas (over ObjectBasics), I was not able to get any function to work that had a signature taking generics. For example something like this would not work because the signature violates the rules in the adapter (regardless of if I specified the type args)

public fun transfer<T>(c: Coin<T>, recipient: vector<u8>, _ctx: TxContext) {
  Transfer::transfer(c, Address:new(recipient))
}

Function signature violation occurred here https://github.com/MystenLabs/fastnft/blob/main/fastx_programmability/adapter/src/adapter.rs#L368

So the current solution uses a wrapper in GAS.move. Let me know the better way to do this from Coin.move or with generics

My bad... there were more issues with this than I anticipated. I dropped #200 which adds an adapter test to ensure that Move-powered transfers will actually work end-to-end + fixes all the problems you found.

[oxade]

https://github.com/MystenLabs/fastnft/blob/main/fastx_programmability/adapter/src/adapter.rs#L368

Thanks
I see you removed the function signature check
What was the reason for having it before?

[sblackshear]

Can we use a seeded RNG here? Want to make sure we get the same values across different runs.

[sblackshear]

We will need them eventually, but punting on them until we use Coin::transfer seems reasonable.