chore(bench): enforce bench spec for json output format

[SouchonTheo]

This is a pr regarding bench spec

The rules are related to bench common but can evolve to match a specific specification

---

This change is 

[soonum]

Nice job.

@soonum reviewed 31 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on SouchonTheo).

---

tfhe-benchmark/src/name_spec.rs line 8 at r2 (raw file):

    HlapiCuda,
    HlapiHpu,
}

I'm thinking about plugin the other layers in the future.
Shouldn't we have something like that to avoid having repetitive combination of BenchPrefix::<LayerBackend>?

Suggestion:

pub enum Layer {
    Hlapi,
    Integer,
    Shortint,
    Boolean,
    CoreCrypto,
}

pub enum Backend {
    CPU, // write nothing on Display implem
    Cuda,
    Hpu
}

---

tfhe-benchmark/src/name_spec.rs line 23 at r2 (raw file):

///
/// `func_name`, `type_name` and `param_name` are kept as `&str` because their values
/// are generated dynamically: `func_name` and `type_name` come from `stringify!()` in

That's not really the case for some benchmarks. One could craft the type_name from within the benchmark implementation.
You can look at integer ZK for example.

[soonum]

We're finally gonna have it 🎉 ! We're almost ready to merge, some details to address though.

@soonum reviewed 24 files and all commit messages, and made 7 comments.
Reviewable status: 36 of 40 files reviewed, 8 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and SouchonTheo).

---

utils/benchmark_spec/src/bench_crate.rs line 14 at r3 (raw file):

impl BenchCrate {
    fn layer(&self) -> &TfheLayer {

I'm not sure about this output type. What if another crate doesn't have any notion of tfhe-rs layer ? 🤔

---

utils/benchmark_spec/src/layer.rs line 9 at r3 (raw file):

#[derive(Clone, Copy, Display)]
#[strum(serialize_all = "snake_case")]
pub enum HlapiOp {

I don't this only applies to HLAPI, it's more like an AtomicOp or ArthimeticOp. If you look into integer layer, you will most likely reuse almost of these operation names.

---

utils/benchmark_spec/src/layer.rs line 55 at r3 (raw file):

#[derive(Display)]
#[strum(serialize_all = "snake_case")]
pub enum OpsLayer {

Regarding what I've written above, I'm not sure we need this kind of indirection.
I'm trying to think to be the most generic possible. For example, where would fit HLAPI ERC20 operations in this case ?

---

utils/benchmark_spec/src/lib.rs line 50 at r3 (raw file):

/// Enforces the naming convention for benchmark IDs.
///
/// Format: `{crate}::{layer}[::{backend}]::{ops}::{op}[::throughput]::{param}[::scalar][::{type}]`

Maybe something more like this no ? Just to be more precise.

Suggestion:

/// Format: `{crate}::{layer}[::{backend}]::{operation_kind}::{function_name}[::throughput]::{parameter_set}[::scalar][::{type}]`

---

utils/benchmark_spec/src/lib.rs line 58 at r3 (raw file):

    pub bench_crate: BenchCrate,
    pub backend: Backend,
    pub param_name: &'a str,

Couldn't we use an impl trait bound like this ?
And then we call the param.name() inside the body of the function instead of from the body of the caller since now the bench_id will be created using the implementation of the Display trait.

Suggestion:

pub param: impl NamedParam,

---

utils/benchmark_spec/src/lib.rs line 83 at r3 (raw file):

    }

    pub fn hlapi(

@IceTDrinker is this a more idiomatic way of naming a new function in Rust ?

Suggestion:

 pub fn new_hlapi(

[IceTDrinker]

a few comments, good first step !

let's be mindful when adding some bench structures, maybe it's not possible to have a bench spec for all crates, if not then we may have somethign like TfheBenchSpec, ZkBenchSpec down the line

All that to say : let's try to make it generic, but if the genericity becomes a big burden in terms of code, then we can have some genericity up to a point

@IceTDrinker reviewed 40 files and all commit messages, and made 9 comments.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, soonum, and SouchonTheo).

---

utils/benchmark_spec/src/lib.rs line 83 at r3 (raw file):

Previously, soonum (David Testé) wrote…

@IceTDrinker is this a more idiomatic way of naming a new function in Rust ?

I never know, whatever is readable, new_hlapi is good I would say

---

utils/benchmark_spec/Cargo.toml line 6 at r3 (raw file):

edition = "2024"
rust-version.workspace = true

you can add

publish = false

to the [package] section since we don't plan on publishing it

---

utils/benchmark_spec/Cargo.toml line 7 at r3 (raw file):

rust-version.workspace = true

[features]

do we need features here ?

I think we could just have everything available at all times ?

wdyt ?

---

utils/benchmark_spec/src/bench_crate.rs line 10 at r3 (raw file):

#[strum(serialize_all = "snake_case")]
pub enum BenchCrate {
    Tfhe(TfheLayer),

should Tfhe contain a backend, and the backend contain TfheLayer ?

---

utils/benchmark_spec/src/layer.rs line 66 at r3 (raw file):

    }

    pub(crate) fn fmt_with_backend(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

with backend ?

if needed you call it "pretty_fmt" or something, here it says with_backend, but there is no backend right ?

---

utils/benchmark_spec/src/layer.rs line 75 at r3 (raw file):

#[strum(serialize_all = "snake_case")]
pub enum TfheLayer {
    Hlapi(OpsLayer),

OpsLayer ?

Isn't it "just" Ops ?

or even HlapiOps ? (since shortint and core_crypto can have different ops)

---

utils/benchmark_spec/src/lib.rs line 13 at r3 (raw file):

use std::{env, fmt};

pub static BENCH_TYPE: OnceLock<BenchmarkType> = OnceLock::new();

can be put directly in the function get_bench_type as a local static

---

utils/benchmark_spec/src/lib.rs line 50 at r3 (raw file):

/// Enforces the naming convention for benchmark IDs.
///
/// Format: `{crate}::{layer}[::{backend}]::{ops}::{op}[::throughput]::{param}[::scalar][::{type}]`

maybe use a more regex-like representation

something like

{crate}::{layer}(::{backend})?::{ops}::{op}(::throughput)?::{param}(::scalar)?[::{type}]

for ops, here it could also be erc20 e.g. right ?

not sure to name it, benchmark family ? really not sure, so no need to change but worth pondering