
# Objective #13432 added proper reflection-based cloning. This is a better method than cloning via `clone_value` for reasons detailed in the description of that PR. However, it may not be immediately apparent to users why one should be used over the other, and what the gotchas of `clone_value` are. ## Solution This PR marks `PartialReflect::clone_value` as deprecated, with the deprecation notice pointing users to `PartialReflect::reflect_clone`. However, it also suggests using a new method introduced in this PR: `PartialReflect::to_dynamic`. `PartialReflect::to_dynamic` is essentially a renaming of `PartialReflect::clone_value`. By naming it `to_dynamic`, we make it very obvious that what's returned is a dynamic type. The one caveat to this is that opaque types still use `reflect_clone` as they have no corresponding dynamic type. Along with changing the name, the method is now optional, and comes with a default implementation that calls out to the respective reflection subtrait method. This was done because there was really no reason to require manual implementors provide a method that almost always calls out to a known set of methods. Lastly, to make this default implementation work, this PR also did a similar thing with the `clone_dynamic ` methods on the reflection subtraits. For example, `Struct::clone_dynamic` has been marked deprecated and is superseded by `Struct::to_dynamic_struct`. This was necessary to avoid the "multiple names in scope" issue. ### Open Questions This PR maintains the original signature of `clone_value` on `to_dynamic`. That is, it takes `&self` and returns `Box<dyn PartialReflect>`. However, in order for this to work, it introduces a panic if the value is opaque and doesn't override the default `reflect_clone` implementation. One thing we could do to avoid the panic would be to make the conversion fallible, either returning `Option<Box<dyn PartialReflect>>` or `Result<Box<dyn PartialReflect>, ReflectCloneError>`. This makes using the method a little more involved (i.e. users have to either unwrap or handle the rare possibility of an error), but it would set us up for a world where opaque types don't strictly need to be `Clone`. Right now this bound is sort of implied by the fact that `clone_value` is a required trait method, and the default behavior of the macro is to use `Clone` for opaque types. Alternatively, we could keep the signature but make the method required. This maintains that implied bound where manual implementors must provide some way of cloning the value (or YOLO it and just panic), but also makes the API simpler to use. Finally, we could just leave it with the panic. It's unlikely this would occur in practice since our macro still requires `Clone` for opaque types, and thus this would only ever be an issue if someone were to manually implement `PartialReflect` without a valid `to_dynamic` or `reflect_clone` method. ## Testing You can test locally using the following command: ``` cargo test --package bevy_reflect --all-features ``` --- ## Migration Guide `PartialReflect::clone_value` is being deprecated. Instead, use `PartialReflect::to_dynamic` if wanting to create a new dynamic instance of the reflected value. Alternatively, use `PartialReflect::reflect_clone` to attempt to create a true clone of the underlying value. Similarly, the following methods have been deprecated and should be replaced with these alternatives: - `Array::clone_dynamic` → `Array::to_dynamic_array` - `Enum::clone_dynamic` → `Enum::to_dynamic_enum` - `List::clone_dynamic` → `List::to_dynamic_list` - `Map::clone_dynamic` → `Map::to_dynamic_map` - `Set::clone_dynamic` → `Set::to_dynamic_set` - `Struct::clone_dynamic` → `Struct::to_dynamic_struct` - `Tuple::clone_dynamic` → `Tuple::to_dynamic_tuple` - `TupleStruct::clone_dynamic` → `TupleStruct::to_dynamic_tuple_struct`
166 lines
5.0 KiB
Rust
166 lines
5.0 KiB
Rust
use core::{hint::black_box, iter, time::Duration};
|
|
|
|
use benches::bench;
|
|
use bevy_reflect::{DynamicList, List};
|
|
use criterion::{
|
|
criterion_group, measurement::Measurement, AxisScale, BatchSize, BenchmarkGroup, BenchmarkId,
|
|
Criterion, PlotConfiguration, Throughput,
|
|
};
|
|
|
|
criterion_group!(
|
|
benches,
|
|
concrete_list_apply,
|
|
concrete_list_to_dynamic_list,
|
|
dynamic_list_apply,
|
|
dynamic_list_push
|
|
);
|
|
|
|
// Use a shorter warm-up time (from 3 to 0.5 seconds) and measurement time (from 5 to 4) because we
|
|
// have so many combinations (>50) to benchmark.
|
|
const WARM_UP_TIME: Duration = Duration::from_millis(500);
|
|
const MEASUREMENT_TIME: Duration = Duration::from_secs(4);
|
|
|
|
/// An array of list sizes used in benchmarks.
|
|
///
|
|
/// This scales logarithmically.
|
|
const SIZES: [usize; 5] = [100, 316, 1000, 3162, 10000];
|
|
|
|
/// Creates a [`BenchmarkGroup`] with common configuration shared by all benchmarks within this
|
|
/// module.
|
|
fn create_group<'a, M: Measurement>(c: &'a mut Criterion<M>, name: &str) -> BenchmarkGroup<'a, M> {
|
|
let mut group = c.benchmark_group(name);
|
|
|
|
group
|
|
.warm_up_time(WARM_UP_TIME)
|
|
.measurement_time(MEASUREMENT_TIME)
|
|
// Make the plots logarithmic, matching `SIZES`' scale.
|
|
.plot_config(PlotConfiguration::default().summary_scale(AxisScale::Logarithmic));
|
|
|
|
group
|
|
}
|
|
|
|
fn list_apply<M, LBase, LPatch, F1, F2, F3>(
|
|
group: &mut BenchmarkGroup<M>,
|
|
bench_name: &str,
|
|
f_base: F1,
|
|
f_patch: F3,
|
|
) where
|
|
M: Measurement,
|
|
LBase: List,
|
|
LPatch: List,
|
|
F1: Fn(usize) -> F2,
|
|
F2: Fn() -> LBase,
|
|
F3: Fn(usize) -> LPatch,
|
|
{
|
|
for size in SIZES {
|
|
group.throughput(Throughput::Elements(size as u64));
|
|
|
|
group.bench_with_input(
|
|
BenchmarkId::new(bench_name, size),
|
|
&size,
|
|
|bencher, &size| {
|
|
let f_base = f_base(size);
|
|
let patch = f_patch(size);
|
|
bencher.iter_batched(
|
|
f_base,
|
|
|mut base| base.apply(black_box(&patch)),
|
|
BatchSize::SmallInput,
|
|
);
|
|
},
|
|
);
|
|
}
|
|
}
|
|
|
|
fn concrete_list_apply(criterion: &mut Criterion) {
|
|
let mut group = create_group(criterion, bench!("concrete_list_apply"));
|
|
|
|
let empty_base = |_: usize| Vec::<u64>::new;
|
|
let full_base = |size: usize| move || iter::repeat_n(0, size).collect::<Vec<u64>>();
|
|
let patch = |size: usize| iter::repeat_n(1, size).collect::<Vec<u64>>();
|
|
|
|
list_apply(&mut group, "empty_base_concrete_patch", empty_base, patch);
|
|
|
|
list_apply(&mut group, "empty_base_dynamic_patch", empty_base, |size| {
|
|
patch(size).to_dynamic_list()
|
|
});
|
|
|
|
list_apply(&mut group, "same_len_concrete_patch", full_base, patch);
|
|
|
|
list_apply(&mut group, "same_len_dynamic_patch", full_base, |size| {
|
|
patch(size).to_dynamic_list()
|
|
});
|
|
|
|
group.finish();
|
|
}
|
|
|
|
fn concrete_list_to_dynamic_list(criterion: &mut Criterion) {
|
|
let mut group = create_group(criterion, bench!("concrete_list_to_dynamic_list"));
|
|
|
|
for size in SIZES {
|
|
group.throughput(Throughput::Elements(size as u64));
|
|
|
|
group.bench_with_input(
|
|
BenchmarkId::from_parameter(size),
|
|
&size,
|
|
|bencher, &size| {
|
|
let v = iter::repeat_n(0, size).collect::<Vec<_>>();
|
|
|
|
bencher.iter(|| black_box(&v).to_dynamic_list());
|
|
},
|
|
);
|
|
}
|
|
|
|
group.finish();
|
|
}
|
|
|
|
fn dynamic_list_push(criterion: &mut Criterion) {
|
|
let mut group = create_group(criterion, bench!("dynamic_list_push"));
|
|
|
|
for size in SIZES {
|
|
group.throughput(Throughput::Elements(size as u64));
|
|
|
|
group.bench_with_input(
|
|
BenchmarkId::from_parameter(size),
|
|
&size,
|
|
|bencher, &size| {
|
|
let src = iter::repeat_n((), size).collect::<Vec<_>>();
|
|
let dst = DynamicList::default();
|
|
|
|
bencher.iter_batched(
|
|
|| (src.clone(), dst.to_dynamic_list()),
|
|
|(src, mut dst)| {
|
|
for item in src {
|
|
dst.push(item);
|
|
}
|
|
},
|
|
BatchSize::SmallInput,
|
|
);
|
|
},
|
|
);
|
|
}
|
|
|
|
group.finish();
|
|
}
|
|
|
|
fn dynamic_list_apply(criterion: &mut Criterion) {
|
|
let mut group = create_group(criterion, bench!("dynamic_list_apply"));
|
|
|
|
let empty_base = |_: usize| || Vec::<u64>::new().to_dynamic_list();
|
|
let full_base = |size: usize| move || iter::repeat_n(0, size).collect::<Vec<u64>>();
|
|
let patch = |size: usize| iter::repeat_n(1, size).collect::<Vec<u64>>();
|
|
|
|
list_apply(&mut group, "empty_base_concrete_patch", empty_base, patch);
|
|
|
|
list_apply(&mut group, "empty_base_dynamic_patch", empty_base, |size| {
|
|
patch(size).to_dynamic_list()
|
|
});
|
|
|
|
list_apply(&mut group, "same_len_concrete_patch", full_base, patch);
|
|
|
|
list_apply(&mut group, "same_len_dynamic_patch", full_base, |size| {
|
|
patch(size).to_dynamic_list()
|
|
});
|
|
|
|
group.finish();
|
|
}
|