bevyengine / bevyengine/bevy

Consider switching to aborting on panic in hot ECS functions

Open
#12,107 2 comments 1 reaction 0 assignees View on GitHub
A-ECS C-Feature C-Performance I-Unsound X-Needs-SME
Dominant language
Rust
Stars
48.2k
Forks
4.8k
Avg merge
3d 22h
Merged PRs (30d)
161

Description

## What problem does this solve or what need does it fill?
Some potentially very hot and heavily monomorphized ECS functions still have panics in the case something goes wrong. Panicking results in a lot of codegen, which can make binary sizes much larger, and can affect instruction fetch locality and thus performance in very hot ECS functions.

As seen via the [asembly generation tests](https://github.com/search?q=repo%3Ajames7132%2Fbevy_asm_tests%20panic&type=code), there are still many locations where panics may crop up.

## What solution would you like?
Create a utility called [`AbortOnPanic`](https://github.com/tokio-rs/tokio/blob/a2096049ee2b8e30b420ba7ecff6b81e609428b3/tokio/src/runtime/scheduler/multi_thread/worker.rs#L453) which turns all panics in the local scope into aborts, only in release mode.

This technique is used by a number of fairly low level crates like `tokio` or `async-task`, where a panic would otherwise be an artifact of incorrect safety or correctness-related invariants not being satisfied. Any location where it's calling into user provided code (i.e. trait functions turned into function pointers) must be wrapped in `std::panic::catch_unwind` to avoid aborting on user-provided code.

This may also be a correctness and soundness issue, since panicking on unsatisfied invariant can be caught via `catch_unwind`, and leave the ECS in an invalid and unsound state.

Note that this likely won't affect builds using `panic = "abort"` in their build profiles, but will likely improve the default `--release ` configuration that cargo provides.

This should only be used in locations where we know that we're not calling into user provided code, and we should only be panicking

## What alternative(s) have you considered?
Leaving it as is, panicking instead of aborting.

The other alternative is to use `std::hint::unreachable_unchecked`, which is both unsafe and very easy to result in undefined behavior is used incorrectly.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.