Skip to content

Remove code duplications in benchmarks#5209

Closed
ThierryBerger wants to merge 11 commits intobevyengine:mainfrom
ThierryBerger:5161-benchmark-ecs-less-code
Closed

Remove code duplications in benchmarks#5209
ThierryBerger wants to merge 11 commits intobevyengine:mainfrom
ThierryBerger:5161-benchmark-ecs-less-code

Conversation

@ThierryBerger
Copy link
Member

Objective

Fixes #5161
Built ont top of #4972

Solution

  • Leverage Rust generics to remove code duplication in benches.
  • Remain as close as possible to existing code

Copy link
Member Author

@ThierryBerger ThierryBerger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed a code duplication as an example of what's possible, The future commits should be very similar so feedbacks are already appreciated 👍.

I recommend to check this latest commit 31733cc rather than the whole diff because this Pull Request is built on top of another branch.
Older commit d68486f is also new, but builds on what is or will be agreed from parent branch #4972.

@@ -1,3 +1,5 @@
use crate::generic_bench;
Copy link
Member Author

@ThierryBerger ThierryBerger Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file use the same logic of #4972

@@ -0,0 +1,212 @@
use crate::generic_bench;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No changes in this file because part of #4972

Comment on lines +15 to +25
type BenchGroup<'a> = BenchmarkGroup<'a, criterion::measurement::WallTime>;

pub fn generic_bench<P: Copy>(
bench_group: &mut BenchmarkGroup<criterion::measurement::WallTime>,
mut benches: Vec<Box<dyn FnMut(&mut BenchGroup, P)>>,
bench_parameters: P,
) {
for b in &mut benches {
b(bench_group, bench_parameters);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #4972.

Few questions:

  • Should this approach be translated into Benchmark structs ?
  • Or Benchmark structs converted to this approach ?
  • Or leave as-is for now 🤷

@alice-i-cecile
Copy link
Member

Take a look at #3505 too :) I stumbled across it the other day. If you want to incorporate those changes please do so and credit the author.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Diagnostics Logging, crash handling, error reporting and performance analysis labels Jul 5, 2022
@bas-ie
Copy link
Contributor

bas-ie commented Oct 4, 2024

Backlog cleanup: closing due to inactivity, with a nod to existing tracking issue #5161.

@bas-ie bas-ie closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Code-Quality A section of code that is hard to understand or change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean benchmarks storage type code duplication

3 participants