ensure! is better than assert!?

[AurevoirXavier]

          ensure is better than assert, because the benchmarking pipeline can handle errors

Originally posted by @shawntabrizi in #6310 (comment)

I noticed that the benchmark code throughout this repository heavily utilizes assert!. Should we consider replacing all instances of it?

[shawntabrizi]

Wherever we can return an error, we should probably use ensure, as it allows the benchmarking pipeline to exit gracefully.

For example, imagine you run benchmarks on the whole runtime, all of that data will be saved in memory before it is parsed and put into JSON / handlebars templates.

If the benchmark process panics, all of that data is just thrown away.

AFAIK, it should be possible to get lots of reasonable and usable data even when a benchmark returns an error.

Some functions may not handle returning an error (like some kind of setup function), in which case assert is fine. But ensure is def better on average afaik.

cc @ggwpez

[ggwpez]

The benchmarking CLI handles panics and errors in the same way, since it spawns a new wasm instance, these panics never propagate upwards anyway.

If the benchmark process panics, all of that data is just thrown away.

This is pretty much mitigated since we run them per-pallet (and also continuously in CI to check that they are not broken).
So it would throw away the results of the pallet, but not of the other pallets. I think there is no way around this, as we cannot write partial pallet results to the weight files. Then there would be some calls missing.

AFAIK, it should be possible to get lots of reasonable and usable data even when a benchmark returns an error.

Yea it could be improved to still write partial weight files or output the intermediary JSON for recovery.

Should we consider replacing all instances of it?

No this would be too much time and reviews wasted on something minuscle. For new code we can keep it in mind, but i would not dig up old code just for that change.