Add/fix track_caller attribute on panicking entity accessor methods

[MrGunflame]

Objective

World::entity, World::entity_mut and Commands::entity should be marked with track_caller to display where (in user code) the call with the invalid Entity was made. Commands::entity already has the attibute, but it does nothing due to the call to unwrap_or_else.

Solution

• Apply the track_caller attribute to the World::entity_mut and World::entity.
• Remove the call to unwrap_or_else which makes the track_caller attribute useless (because unwrap_or_else is not track_caller itself). The avoid eager evaluation of the panicking branch it is never inlined.

[alice-i-cecile]

Being able to debug this better would be incredible: I was confused about why track_caller wouldn't work.

[nicopap]

I had to remember why you'd want the #[inline(never)] and #[cold], but I recall its' because it will avoid too much overhead from that panic call. I would have preferred to see an explanation for them, but alright.

[mockersf]

would running benches be relevant when changing those functions?

[alice-i-cecile]

Yeah, it's worth checking. I'll do a bench before merging this.

[MrGunflame]

I had to remember why you'd want the #[inline(never)] and #[cold], but I recall its' because it will avoid too much overhead from that panic call. I would have preferred to see an explanation for them, but alright.

Yes, this will avoid having the panic in the hot code path. Maybe adding some comment for that is a good idea.

would running benches be relevant when changing those functions?

I suspect there to be little difference between this and the previous version, they should effectively result in the same codegen, but the additional cold attribute could result in some (tiny) speedup.

[james7132]

I've seen similar uses in the stdlib pretty often, so the practice makes plenty of sense. Though I'd love to see concrete benches before merging. Wish there was a better way to do this without the boilerplate though.

[SkiFire13]

LGTM

[alice-i-cecile]

Results are main vs this PR, for relevant benchmarks:

world_get/50000_entities_table
time: [125.89 µs 126.21 µs 126.54 µs]
change: [+1.7698% +2.1650% +2.5762%] (p = 0.00 < 0.05)
Performance has regressed.
Found 7 outliers among 100 measurements (7.00%)
5 (5.00%) high mild
2 (2.00%) high severe
world_get/50000_entities_sparse
time: [161.59 µs 162.49 µs 163.66 µs]
change: [-0.9856% +0.5145% +1.6212%] (p = 0.51 > 0.05)
No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
5 (5.00%) high mild
6 (6.00%) high severe

insert_commands/insert time: [301.21 µs 301.67 µs 302.17 µs]
change: [-13.413% -12.216% -10.967%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
4 (4.00%) high mild
5 (5.00%) high severe
insert_commands/insert_batch
time: [263.40 µs 263.60 µs 263.84 µs]
change: [-1.4678% +0.4835% +2.3222%] (p = 0.65 > 0.05)
No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
5 (5.00%) high mild
7 (7.00%) high severe

[alice-i-cecile]

These benchmarks are noisy for me, even with every other program closed. I'd call this "probably performance-neutral", and this is very impactful for UX. @james7132, if you agree please merge <3

[james7132]

These do look like they're within margin of error, sans the insert_commands results, which is typically pretty stable and a 10-13% improvement is indeed welcome. Merging.