QueryParIter::map_collect and similar operations

[stepancheg]

Objective

par_iter is convenient, unless you need to collect the results into a Vec or HashMap.

Bevy commonly uses thread-local collectors, and aggregates them, for example, here:

bevy/crates/bevy_render/src/view/visibility/mod.rs

Lines 452 to 454 in 101037d

	for cell in &mut thread_queues {
	visible_entities.entities.append(cell.get_mut());
	}

it is not convenient because:

• boilerplate thread-local setup
• non-deterministic output

Solution

Add QueryParIter functions:

• flat_map_collect (similar to .flat_map(...).collect()
• filter_map_collect
• map_collect

These functions might be not as universally applicable as for_each with thread-local aggregators, because more expensive, but:

• easy to use
• deterministic

Changelog

QueryParIter has new functions: flat_map_collect, filter_map_collect, map_collect.

[james7132]

This can be quite performance sensitive code. Some form of profiling on our existing stress tests would be nice. I'm also hesitant to promote a option that allocates on every call as well, particular if it's a Vec being recreated and regrown every tick of the app.

[stepancheg]

@james7132 suggestions how to benchmarks?

Overall, I don't believe, it will be bad. Malloc is cheap. Moreover, it is quite possible, it will actually increase performance, because it gives users an instrument to parallelize code, which was too hard to parallelize before.

[NthTensor]

I was helping some who needed this yesterday. This allows us to, for example, buffer events parallel and then dispatch them in sequence while preserving order. Seems super useful to me.

Many useful features can degrade performance if misapplied. If the docs are clear about the performance characteristics, I think allocation shouldn't be a blocking issue.

[stepancheg]

I did lame benchmark.

I ran

cargo run --example many_cubes

and patched check_visibility function, because it seems to be a bottleneck.

bevy/crates/bevy_render/src/view/visibility/mod.rs

Line 404 in c1b785c

visible_aabb_query.par_iter_mut().for_each(|query_item| {

By default on my laptop it outputs about 18.5 FPS.

If I replace par_iter with non-parallel iter, FPS drops to about 15.0 FPS. So this function is indeed a bottleneck.

If I rewrite the function with .filter_map_collect(), performance stays at about the same 18.5 FPS. (diff).

Also tried running the same test with --release, performance of all three is roughly the same 200 FPS, so bottleneck is elsewhere.

To get precise perf difference, much better benchmark is needed.

So my conclusion is that if filter_map_collect is slower, it is not significantly slower. So seems to be good enough.

[hymm]

There is one bench that tests parallel iteration. You can run it with cargo bench heavy when in the `benches folder. I get around 250-260us a run on my machine so it is a little noisy.

[stepancheg]

Ok, I tried adding another fake benchmark there.

• Reduce the iterations from base benchmark to 10 (to make it a bit more realistic)
• But increase batch size 10 times
• copy-paste base benchmark to map_collect (non-parallel) and par_map_collect

diff

The output is

heavy_compute/base
                        time:   [530.31 µs 531.80 µs 533.31 µs]
heavy_compute/par_map_collect
                        time:   [570.93 µs 572.52 µs 574.15 µs]
heavy_compute/map_collect
                        time:   [2.4066 ms 2.4105 ms 2.4144 ms]

In this artificial setup, parallel map_collect is somewhat more expensive than parallel for_each and as expected significantly faster than non-parallel version.

Which is exactly what is expected: code is doing what it is supposed to do.

More correct benchmark would be comparison with Local<ThreadLocal<Cell<Vec<bool>>>> shenanigans. That would be a bit more effort to write, and this is probably not what we'd recommend to users anyway.

[hymm]

can you compare heavy_compute/base to main branch?

For checking with many_cubes, we usually use tracy https://github.com/bevyengine/bevy/blob/main/docs/profiling.md. And use show the histograms diff for the relevant systems.

[stepancheg]

I can compare against main branch, but:

• criterion is not reliable benchmarking tool. It can easily show reliable 5% speedup with no changes at all
• this PR does not change compilation of for_each implementation, only syntax. fold_impl now returns Vec<()>, but
  • it has no allocations
  • it was there before, only hidden, here:

bevy/crates/bevy_tasks/src/task_pool.rs

Line 287 in 06bf928

pub fn scope<'env, F, T>(&self, f: F) -> Vec<T>

[stepancheg]

OK, benchmark against main.

On main.

cargo bench -p benches --bench ecs -- heavy_compute --save-baseline main

on this branch:

cargo bench -p benches --bench ecs -- heavy_compute --baseline main

heavy_compute/base      time:   [564.08 µs 565.16 µs 566.09 µs]
                        change: [-0.4176% -0.0620% +0.2685%] (p = 0.73 > 0.05)
                        No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
  2 (2.00%) low severe
  4 (4.00%) low mild
  3 (3.00%) high mild
  2 (2.00%) high severe