Remove unnecessary allocations from subdivide_triangles

[TimTheBig]

No description provided.

[timschmidt]

I don't like the manual loop here. Something like:

let next_level: Vec<_> = queue.into_iter()
.flat_map(subdivide_triangle)
.collect();

Would be better.

It'd also be nice to have a function-level test submitted with the change, verifying that the changed function works.

[TimTheBig]

I don't like the manual loop here. Something like:

let next_level: Vec<_> = queue.into_iter() .flat_map(subdivide_triangle) .collect();

Would be better.

That would get rid of the pre-allocation, I have benchmarked collect and push with with_capacity before and collect is slower

It'd also be nice to have a function-level test submitted with the change, verifying that the changed function works.

I will make a test and test it on the original and this patch

[timschmidt]

That would get rid of the pre-allocation, I have benchmarked collect and push with with_capacity before and collect is slower

Supposedly, collect is smart enough to catch the size hint from further up the chain and allocate only once. As would the following:

let mut next_level = Vec::with_capacity(queue.len() * 4);
for t in queue {
next_level.extend(subdivide_triangle(t)); // or extend_from_slice
}

Iterators are far easier to multithread with rayon's par_iter so that's my preference. extend, like the iterator, is harder to screw up than the for loop, so it's next in my preference.

IOW, I'm more worried about the code being correct and robust to future changes than squeezing every last ounce of single-threaded performance out of it.

[TimTheBig]

Supposedly, collect is smart enough to catch the size hint from further up the chain and allocate only once. As would the following:

let mut next_level = Vec::with_capacity(queue.len() * 4); for t in queue { next_level.extend(subdivide_triangle(t)); // or extend_from_slice }

extend for Vec uses Copy

Iterators are far easier to multithread with rayon's par_iter so that's my preference. extend, like the iterator, is harder to screw up than the for loop, so it's next in my preference.

In this case par_iter would be uneeded

IOW, I'm more worried about the code being correct and robust to future changes than squeezing every last ounce of single-threaded performance out of it.

Fair

[TimTheBig]

append may be the way to go

[recurseml]

Review by RecurseML

🔍 Review performed on 1ba9354..857ed77

✨ No bugs found, your code is sparkling clean

✅ Files analyzed, no issues (1)

• src/mesh/polygon.rs