Faster From<Vec<T>> for doubling SplitVec #90
Conversation
|
Hi @TechnoPorg . Thanks a lot for the PR! Regardless of the impact on performance, it is awesome that your PR relaxes the I have a couple of small suggestions. It would be great if you could take a look at them before merging. 1. Extending the test It would be great to extend your test with some edge case numbers. This is the current test you've added. #[test]
fn from_same_as_push() {
for len in 0..135 {
let vec: Vec<_> = (0..len).collect();
let split_vec_from: SplitVec<_, Doubling> = vec.clone().into();
let mut split_vec_manual = SplitVec::new();
for item in vec {
split_vec_manual.push(item);
}
assert_eq!(split_vec_from, split_vec_manual);
}
}It would be great if you could simply extend this test with different lengths. As you know, the lengths of the fragments are 4, 8, 16, etc. So testing lengths 11, 12, 13 would be nice. And also 0, 1 are special. Feel free to add other magic numbers :) #[test_matrix([0, 1, 4, 5, 11, 12, 13, 135])]
fn from_same_as_push(len: usize) {
for len in 0..len {
let vec: Vec<_> = (0..len).map(|x| x.to_string()).collect();
let split_vec_from: SplitVec<_, Doubling> = vec.clone().into();
let mut split_vec_manual = SplitVec::new();
for item in vec {
split_vec_manual.push(item);
}
assert_eq!(split_vec_from, split_vec_manual);
}
}2. Benchmark I wouldn't worry much if the performance did not change at all since removing Consider for instance the benchmarked function: fn doubling_from_std_vec<T: Clone, F: Fn(usize) -> T>(n: usize, value: F) -> SplitVec<T, Doubling> {
// part A
let mut std_vec = Vec::new();
for i in 0..n {
std_vec.push(value(i));
}
// part B
SplitVec::from(std_vec)
}Here, we only want to measure part B: I don't think we can completely avoid part A. Maybe, we might at least mitigate the impact. For instance, how about the following: fn doubling_from_std_vec<T: Clone>(vec: &Vec<T>) -> SplitVec<T, Doubling> {
let std_vec = vec.clone();
SplitVec::from(std_vec)
}We could then build the |
|
Thanks for the review! I've adjusted the benchmark to construct the Vec outside of the function being benchmarked, which has made the numbers more consistent. I'm not entirely sure what is meant by # 1 - since the test is iterating over all vector lengths from 0 to 135, it already passes through 11, 12, 13, and more. Updated benchmarksThis PR
Current main
|
TechnoPorg
force-pushed
the
faster-doubling-from-vec
branch
from
370fb44 to
94808d5
Compare
|
Thanks @TechnoPorg . No worries for my first point, all looks great. Will merge once the checks are complete. |
2 participants
This reworks the
From<Vec<T>>conversions for SplitVecs with the Doubling growth strategy to not clone the input values, which should increase both performance and usability.In practice, I'm not sure how effective this is. According the benchmark I've added here, performance stays roughly the same across the board; for two cases it speeds up, while for the other two, it slows down. I have limited experience with micro-optimization of this sort, so any input you have would be appreciated.
faster-doubling-from-vec
main