Remove redundant bounds check in Entities::get

[mjhostet]

Objective

Entities::get does a manual bounds check of a Vec before taking a reference to that slot using brackets, which does the same bounds check again to see if it needs to panic (which of course it can't). This is wasteful.

Solution

Use Vec::get to both bounds check and create a reference in one step. Although the redundant bounds check should be optimized away in release mode, using Vec::get is shorter and more idiomatic, and potentially faster in debug builds.

[alice-i-cecile]

Could you run the benchmarks for this before / after? I think this is a good change even if it's performance neutral, but it's always nice to have measurements to report.

[james7132]

Looking at the generated assembly with all the optimizations on, this indeed doesn't make a change. Rust optimizes out the second bounds check. Though in lower optimization levels this does get rid of an extra check and the panic, and the code is both more idiomatic and readable. LGTM.

[mjhostet]

Could you run the benchmarks for this before / after? I think this is a good change even if it's performance neutral, but it's always nice to have measurements to report.

I'm happy to benchmark (even though this has merged). But I don't see instructions how to do this in CONTRIBUTING.md or elsewhere, did I miss it?

I took a guess and did:

$ cd benches
$ cargo bench --bench ecs -- --save-baseline before
$ git checkout redundant_bounds_check
$ cargo bench --bench ecs -- --save-baseline after
$ critcmp before after > ~/perf_diff.txt

But please let me know if there's a better way.

perf_diff.txt