chore(bench): use the bench spec for erc7984 and dex in hlapi

[SouchonTheo]

pr to use the bench specification on for dex and erc7984

there is also a refacto to have two different type for benchmark type
one dedicated to the pattern matchin to know which bench we will execute and one for the bench spec this allow us to add pbs count which is specific erc7984 and dex bench

also we added the num element field in bench id so it could be useful in another pr to add it for all the throughput

Unit tests was made by AI

---

This change is 

[soonum]

Marvelous, you're the Tiger you think you are.

@soonum reviewed 7 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on nsarlin-zama and SouchonTheo).

---

tfhe-benchmark/benches/high_level_api/dex.rs line 229 at r1 (raw file):

    use std::path::Path;

    fn write_result(file: &mut File, name: String, value: usize) {

Maybe we can move this function into the utils.
In addition, I think we want to use a name: impl Into<String> and do a name.into() inside the body of the function so that consumers can pass any object as long as it implement the trait Into<String>, thus no need to call test_name.to_string() in the functions below.

---

tfhe-benchmark/benches/high_level_api/erc7984.rs line 811 at r1 (raw file):

    let mut c = Criterion::default().sample_size(10).configure_from_args();

    let bench_type = get_bench_type();

I didn't manage to find the usage of this variable in the diff below, am I missing something here?

---

utils/benchmark_spec/src/lib.rs line 122 at r1 (raw file):

        type_name: Option<&'a str>,
        bench_type: impl Into<BenchmarkMetric>,
        backend: Backend,

I'm wondering if we could fetch the backend from within the body of these functions, since we do always the same thing. It would remove the need to pass this argument.

---

utils/benchmark_spec/src/tfhe/hlapi/mod.rs line 32 at r1 (raw file):

    fn op(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            HlapiBench::Ops(op) => write!(f, "::{op}"),

Why can't we use op.fmt_spec(f) here ?

[SouchonTheo]

ahah
I did what you suggested
for the backend we can let it like that for now or wait the answer of IceT

@SouchonTheo made 5 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).

---

tfhe-benchmark/benches/high_level_api/dex.rs line 229 at r1 (raw file):

Previously, soonum (David Testé) wrote…

Maybe we can move this function into the utils.
In addition, I think we want to use a name: impl Into<String> and do a name.into() inside the body of the function so that consumers can pass any object as long as it implement the trait Into<String>, thus no need to call test_name.to_string() in the functions below.

yes you are right the function is created 7 times accross the bench
purrfect refacto

---

tfhe-benchmark/benches/high_level_api/erc7984.rs line 811 at r1 (raw file):

Previously, soonum (David Testé) wrote…

I didn't manage to find the usage of this variable in the diff below, am I missing something here?

it's used one time for the pattern match
I removed it and use it directly

---

utils/benchmark_spec/src/lib.rs line 122 at r1 (raw file):

Previously, soonum (David Testé) wrote…

I'm wondering if we could fetch the backend from within the body of these functions, since we do always the same thing. It would remove the need to pass this argument.

It was the case at the beginning
I remember that @IceTDrinker recommend me to let this part in tfhe-benchmark but can't remember why
Maybe we can re consider it you are right

---

utils/benchmark_spec/src/tfhe/hlapi/mod.rs line 32 at r1 (raw file):

Previously, soonum (David Testé) wrote…

Why can't we use op.fmt_spec(f) here ?

Because some operator like HlIntegerOp is only leaf enum
so fmt_spec is not implemented

[soonum]

utils/benchmark_spec/src/tfhe/hlapi/mod.rs line 32 at r1 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

Because some operator like HlIntegerOp is only leaf enum
so fmt_spec is not implemented

Got it.

[soonum]

tfhe-benchmark/benches/high_level_api/dex.rs line 229 at r1 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

yes you are right the function is created 7 times accross the bench
purrfect refacto

Nice job for the refacto !

[soonum]

Terrific ! Thanks a lot !
We should launch both benchmarks (ERC and DEX) before merging.

@soonum reviewed 7 files, made 1 comment, and resolved 4 discussions.
Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on IceTDrinker and nsarlin-zama).

[IceTDrinker]

utils/benchmark_spec/src/lib.rs line 122 at r1 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

It was the case at the beginning
I remember that @IceTDrinker recommend me to let this part in tfhe-benchmark but can't remember why
Maybe we can re consider it you are right

not sure what the question is here ? do we have to give the backend to the function ?

[IceTDrinker]

Haven't read the bench updates for now, a few questions/nits to understand what you changed

@IceTDrinker reviewed 5 files and all commit messages, and made 7 comments.
Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).

---

utils/benchmark_spec/src/lib.rs line 23 at r2 (raw file):

}

impl BenchmarkTestResult {

test result ?

---

utils/benchmark_spec/src/lib.rs line 31 at r2 (raw file):

    pub fn from_path(path: &Path) -> Self {
        if !path.exists() {
            File::create(path).expect("cannot create benchmark result file");

create and parent dirs too ? not sure it is done automatically

---

utils/benchmark_spec/src/tfhe/hlapi/dex.rs line 10 at r2 (raw file):

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

that would be a DexOp to me 😄

not sure if it's worth renaming to make it easier to read at call site

---

utils/benchmark_spec/src/tfhe/hlapi/dex.rs line 32 at r2 (raw file):

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

that's more a "flavor" than an "op" ?

---

utils/benchmark_spec/src/tfhe/hlapi/erc7984.rs line 30 at r2 (raw file):

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

flavor ?

---

utils/benchmark_spec/src/tfhe/hlapi/mod.rs line 32 at r2 (raw file):

    fn op(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            HlapiBench::Ops(op) => write!(f, "::{op}"),

should this be updated to call op.fmt_spec() ?

[IceTDrinker]

@IceTDrinker reviewed 5 files and made 1 comment.
Reviewable status: 11 of 12 files reviewed, 7 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).

---

tfhe-benchmark/src/bin/wasm_benchmarks_parser.rs line 54 at r2 (raw file):

            benchmark_test_result.write_result(&prefixed_full_name, *val as usize);
        } else {
            let value_in_ns = (val * 1_000_000_f32) as usize;

@soonum a bit surprised by this value a million to get something in nanoseconds ? does it mean val is always saved as a millisecond value ?

[soonum]

@soonum made 5 comments.
Reviewable status: 11 of 12 files reviewed, 7 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and SouchonTheo).

---

tfhe-benchmark/src/bin/wasm_benchmarks_parser.rs line 54 at r2 (raw file):

Previously, IceTDrinker wrote…

@soonum a bit surprised by this value a million to get something in nanoseconds ? does it mean val is always saved as a millisecond value ?

WASM benchmarks are stored as milliseconds. In the database (and then in Grafana) we deal with nanoseconds, since Criterion returns nanoseconds, hence the 1_000_000 conversion.

---

utils/benchmark_spec/src/lib.rs line 122 at r1 (raw file):

Previously, IceTDrinker wrote…

not sure what the question is here ? do we have to give the backend to the function ?

Yes, I was imagine removing the need to pass this argument to the function by calling the get_backend* function from within its body.

---

utils/benchmark_spec/src/lib.rs line 31 at r2 (raw file):

Previously, IceTDrinker wrote…

create and parent dirs too ? not sure it is done automatically

Yes we should create the parents, you're right.

---

utils/benchmark_spec/src/tfhe/hlapi/dex.rs line 10 at r2 (raw file):

Previously, IceTDrinker wrote…

that would be a DexOp to me 😄

not sure if it's worth renaming to make it easier to read at call site

👍

---

utils/benchmark_spec/src/tfhe/hlapi/dex.rs line 32 at r2 (raw file):

Previously, IceTDrinker wrote…

that's more a "flavor" than an "op" ?

Agree.

[IceTDrinker]

@IceTDrinker made 1 comment and resolved 1 discussion.
Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).

---

utils/benchmark_spec/src/lib.rs line 122 at r1 (raw file):

Previously, soonum (David Testé) wrote…

Yes, I was imagine removing the need to pass this argument to the function by calling the get_backend* function from within its body.

having it done automatically could avoid some mistakes I guess, thing is if we want it to be auto detected, it requires features in the bench spec which is not exactly interesting to manage, but yeah I see we always call a function here so it could be done automatically

[SouchonTheo]

new request have been patched

@SouchonTheo made 8 comments and resolved 2 discussions.
Reviewable status: 10 of 12 files reviewed, 4 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).

---

utils/benchmark_spec/src/lib.rs line 122 at r1 (raw file):

Previously, IceTDrinker wrote…

having it done automatically could avoid some mistakes I guess, thing is if we want it to be auto detected, it requires features in the bench spec which is not exactly interesting to manage, but yeah I see we always call a function here so it could be done automatically

yep this one will not be applied

---

utils/benchmark_spec/src/lib.rs line 23 at r2 (raw file):

Previously, IceTDrinker wrote…

test result ?

updated

---

utils/benchmark_spec/src/lib.rs line 31 at r2 (raw file):

Previously, soonum (David Testé) wrote…

Yes we should create the parents, you're right.

I totally agree did copy paste without really looking

---

utils/benchmark_spec/src/tfhe/hlapi/dex.rs line 10 at r2 (raw file):

Previously, soonum (David Testé) wrote…

👍

this cannot be renamed because strum rely on the name of the enum
if you really want to we can change behavior but we lose simplicity of strum logic

---

utils/benchmark_spec/src/tfhe/hlapi/dex.rs line 32 at r2 (raw file):

Previously, soonum (David Testé) wrote…

Agree.

done

---

utils/benchmark_spec/src/tfhe/hlapi/erc7984.rs line 30 at r2 (raw file):

Previously, IceTDrinker wrote…

flavor ?

done

---

utils/benchmark_spec/src/tfhe/hlapi/mod.rs line 32 at r2 (raw file):

Previously, IceTDrinker wrote…

should this be updated to call op.fmt_spec() ?

nope

Because some operator like HlIntegerOp is only leaf enum
so fmt_spec is not implemented from previous conversation

[IceTDrinker]

@IceTDrinker made 5 comments and resolved 2 discussions.
Reviewable status: 2 of 12 files reviewed, 2 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).

---

utils/benchmark_spec/src/lib.rs line 122 at r1 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

yep this one will not be applied

there is no perfect choice here I guess, so we'll see with usage what seems to be the best, it seemed easier to not have features on the spec, if it turns out to be useful for this then I guess it can be updated

TLDR: fine to keep as is for now, to keep in mind for the future

---

utils/benchmark_spec/src/lib.rs line 23 at r2 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

updated

actually my question was: why is it called test restul ? 😅

is it for benchmarks or for tests ?

---

utils/benchmark_spec/src/tfhe/hlapi/dex.rs line 10 at r2 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

this cannot be renamed because strum rely on the name of the enum
if you really want to we can change behavior but we lose simplicity of strum logic

ah ok yes, fine to keep dex then to have the auto display from strum

---

utils/benchmark_spec/src/tfhe/hlapi/dex.rs line 32 at r2 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

done

does that cause issues for the bench name formatting ? (since strum ?)

---

utils/benchmark_spec/src/tfhe/hlapi/mod.rs line 32 at r2 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

nope

Because some operator like HlIntegerOp is only leaf enum
so fmt_spec is not implemented from previous conversation

can you remind me why we don't have fmt_spec on leaf stuff ?

[SouchonTheo]

@SouchonTheo made 5 comments.
Reviewable status: 2 of 12 files reviewed, 2 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).

---

utils/benchmark_spec/src/lib.rs line 122 at r1 (raw file):

Previously, IceTDrinker wrote…

there is no perfect choice here I guess, so we'll see with usage what seems to be the best, it seemed easier to not have features on the spec, if it turns out to be useful for this then I guess it can be updated

TLDR: fine to keep as is for now, to keep in mind for the future

yep exactly I prefer to let this crate as pure as possible too

---

utils/benchmark_spec/src/lib.rs line 23 at r2 (raw file):

Previously, IceTDrinker wrote…

actually my question was: why is it called test restul ? 😅

is it for benchmarks or for tests ?

ahah totally misunderstood on my side

didn't know how to call it honestly

---

utils/benchmark_spec/src/tfhe/hlapi/dex.rs line 10 at r2 (raw file):

Previously, IceTDrinker wrote…

ah ok yes, fine to keep dex then to have the auto display from strum

purfect

---

utils/benchmark_spec/src/tfhe/hlapi/dex.rs line 32 at r2 (raw file):

Previously, IceTDrinker wrote…

does that cause issues for the bench name formatting ? (since strum ?)

nope we don't try stringify the enum type but the inner variant (not sure of the naming but the leaf)

---

utils/benchmark_spec/src/tfhe/hlapi/mod.rs line 32 at r2 (raw file):

Previously, IceTDrinker wrote…

can you remind me why we don't have fmt_spec on leaf stuff ?

because we don't want to add ::ops so we directly write the op instead of using fmt_spec
we could in another way implement it if you want
but we will do the line written here in the function fmt_spec
if you feel it's easier to understand we could do it