gungraun: Fix benchmark name should not depend on the (truncated) description

[gamma0987]

As mentioned in #638 the benchmark name in bencher uses the whole first line as benchmark name which consists of the module path the optional id and a description. However, a benchmark can be uniquely identified by the module path and if present, the id. Since the description is possibly truncated and isn't required to distinguish benchmarks, it shouldn't be part of the benchmark name.

[epompeii]

@gamma0987 when was this description added?

My worry is that this is going to be a breaking change for all users of the rust_gungraun adapter. Any change to the benchmark name will effectively create a new Benchmark with no historical data.

I see two potential paths forward:

1. Limit this change to Support gungraun json output #638
2. Add aliases to Benchmarks, including in BMF

Do you see another option?

[gamma0987]

I've been looking at the version history of lib/bencher_adapter/src/adapters/rust/iai_callgrind.rs and noticed the description has always been part of the benchmark name.

Add aliases for benchmarks, also in BMF

I'm not sure how this is supposed to work, but a database migration would be an alternative. The new id can easily be derived from the existing benchmark name. This corresponds exactly to the logic I've already implemented in the adapter for this pull request.

Limit this change to #638

I don't expect any significant differences between the two parsers and would therefore try to make the various parser options consistent. This way, users can easily switch between them.

Alternatively, everything could be left as is. The main problem, however, is that a new benchmark name and a new database entry are created as soon as someone makes a change to the benchmark settings that are irrelevant to the benchmark case but are shown in the description. The description was never meant as an identifier in contrast to file::group::function id. Using the id without a description would be more stable and convenient.

[epompeii]

I've been looking at the version history of lib/bencher_adapter/src/adapters/rust/iai_callgrind.rs and noticed the description has always been part of the benchmark name.

Thank you for doing the digging @gamma0987!

Add aliases for benchmarks, also in BMF

I'm not sure how this is supposed to work

It would complicate BMF quite a bit, but we could allow for an alias object or a metric object:

{
  "new_benchmark": {
    "value": 42.0
  },
  "old_benchmark": {
    "alias": "new_benchmark"
  }
}

Or something like that. I haven't totally thought there this though. Just spitballing.

but a database migration would be an alternative. The new id can easily be derived from the existing benchmark name. This corresponds exactly to the logic I've already implemented in the adapter for this pull request.

Yeah, we could try to make this work as a migration.
I had originally dismissed this thinking there would be a coordination problem (API and CLI). However, I moved the benchmark harness adapters from the CLI to the API server long, long ago 😅

Would we change just the name or the slug as well?

Since Benchmarks can now be specified by their uuid, slug, or name (checked in that order) there are some edge cases around names that are valid slugs. However, I don't think this is an issue with Gungraun since all benchmark names will have :: in them right?

We would also have to make sure that none of the new names end with an Alert suppressing suffix.

Limit this change to #638

I don't expect any significant differences between the two parsers and would therefore try to make the various parser options consistent. This way, users can easily switch between them.

Agreed, it would be best for them to be consistent. I've sort of been torn on whether it's better to just parse the default human readable output or prefer JSON output parsing. The former is less error prone for users, but the latter is less error prone for parsing.

Alternatively, everything could be left as is. The main problem, however, is that a new benchmark name and a new database entry are created as soon as someone makes a change to the benchmark settings that are irrelevant to the benchmark case but are shown in the description. The description was never meant as an identifier in contrast to file::group::function id. Using the id without a description would be more stable and convenient.

Can you help me understand the id here more?
Where is the description being set? In the benchmark settings?

[gamma0987]

Yeah, we could try to make this work as a migration.

I think that's the best way, too.

Would we change just the name or the slug as well?

As far as I can see the name but maybe also the slug. The slug is derived from the name?

However, I don't think this is an issue with Gungraun since all benchmark names will have :: in them right?

yes

We would also have to make sure that none of the new names end with an Alert suppressing suffix.

If someone tried to change the benchmark name so that it is ignored (which isn't possible with the description in the name), then the names should end with an alert suppressing suffix. Using just the module path and id for the benchmark name would actually make this feature available work with Gungraun benchmarks.

Can you help me understand the id here more?

The id is the part after the double colons #[bench::some_id(arg_1)], The "description" (in the lack of a better name) is generated from the arguments to the benchmark function, here arg_1. For example

// filename: gungraun_benchmark.rs
use gungraun::prelude::*;

#[library_benchmark]
#[bench::some_id(100)]
fn benchmark_function(a: usize) -> usize { a }

library_benchmark_group!(name = group_name, benchmarks = benchmark_function);
main!(library_benchmark_groups = group_name);

the benchmark name is derived from the first output line gungraun_benchmark::group_name::benchmark_function some_id:(100). So, some_id is the id and (100) is the description.

[epompeii]

The slug is derived from the name?

Yes, the vast majority of the time the slug is derived from the name. If the Benchmark POST API is used either through the Console, CLI, or directly then an explicit slug can be requested.

One could argue that changing the slug in a migration would be a breaking change. For example someone could have a script or something that uses the slug in it. However, I would contend that they should be using the UUID and not the slug.

That is, I think we should try to update both the names and slugs in the migration.

We would also have to make sure that none of the new names end with an Alert suppressing suffix.

If someone tried to change the benchmark name so that it is ignored (which isn't possible with the description in the name), then the names should end with an alert suppressing suffix. Using just the module path and id for the benchmark name would actually make this feature available work with Gungraun benchmarks.

The way the Alert suppressing suffixes work is they strip the suffix from the Benchmark name before it is stored.

If we parse: my_benchmark_bencher_ignore then all Bencher stores is my_benchmark
This allows us to keep the same Benchmark history one later removes _bencher_ignore and we just parse my_benchmark

Does that make sense?

Can you help me understand the id here more?

The id is the part after the double colons #[bench::some_id(arg_1)], The "description" (in the lack of a better name) is generated from the arguments to the benchmark function, here arg_1. For example

// filename: gungraun_benchmark.rs
use gungraun::prelude::*;

#[library_benchmark]
#[bench::some_id(100)]
fn benchmark_function(a: usize) -> usize { a }

library_benchmark_group!(name = group_name, benchmarks = benchmark_function);
main!(library_benchmark_groups = group_name);
the benchmark name is derived from the first output line gungraun_benchmark::group_name::benchmark_function some_id:(100). So, some_id is the id and (100) is the description.

Thank you for the explanation. Just so I understand, is the description/arg_1 here optional?

[gamma0987]

Does that make sense?

yes thanks for explaining.

Just so I understand, is the description/arg_1 here optional?

yes. if there is no function argument there is no description. For example:

#[library_benchmark]
#[bench::some_id()]
fn benchmark_function() -> usize { 42 }

btw. The id is optional too, if it is not required to uniquely identify the benchmark because there are no #[bench] annotations:

#[library_benchmark]
fn benchmark_function() -> usize { 42 }