Remove configurable_error_handler feature #18801
Conversation
SpecificProtagonist
added
A-ECS
alice-i-cecile
added
the
S-Needs-Benchmarking
|
Can you do a quick set of benchmarks on the commands benches to verify that this is just about as fast? I trust your intuition here but it will help me sleep better. If that works out we should seriously consider shipping this for 0.16: it's a better experience and removes a perf foot gun. |
|
this is just one run of each though |
|
Oh this is very unsafe! I love it. The review standard for this needs to be set very high, I think. A bad function pointer transmute is the worst kind of UB. But it looks pretty sane to me so far. |
crates/bevy_ecs/src/error/handler.rs
Outdated
| /// [`default_error_handler`]: super::default_error_handler | ||
| pub fn set_global_default_error_handler(handler: fn(BevyError, ErrorContext)) { | ||
| if HANDLER_ADDRESS | ||
| .compare_exchange(core::ptr::null_mut(), handler as *mut (), AcqRel, Acquire) |
There was a problem hiding this comment.
Might we wan this to point to panic by default instead of null, allow it to be set multiple times, and get rid of the branch in default_error_handler and the nullability in get? I'm fine either way.
There was a problem hiding this comment.
Might we wan this to point to panic by default instead of null
Good idea, done.
allow it to be set multiple times
I didn't do that because then we would either have to use Acquire loads, or have a potentially more confusing situation if the user changes it while bevy is running. Both cases are probably fine, but I'd want better benchmarks beforehand. I'm also thinking of a cfg(debug_assertions) check that the handler hasn't been read (when setting the handler).
There was a problem hiding this comment.
Just to double check, can you walk me through why we would need Acquire here? Function pointers are effectively 'static right, is that not strong enough?
My other thought is, if we only want to write to this once then we don't actually need atomics do we? There are never conflicting updates, the problem is entirely memory ordering. So we can use an unsafe cell and fences, can't we? That would alleviate the concern about the function pointer size on exotic platforms.
There was a problem hiding this comment.
if we only want to write to this once then we don't actually need atomics do we?
We can skip them if we make set_global_default_error_handler unsafe (with the precondition that all threads that one must establish a happens-before relationship with all threads that call default_error_handler). Otherwise we need atomics in order to remain sound.
| // SAFETY: We only ever store null or a valid handler function. | ||
| // `null` is guaranteed to be represent `None`: | ||
| // https://rust-lang.github.io/unsafe-code-guidelines/layout/function-pointers.html#representation | ||
| unsafe { core::mem::transmute(ptr) } |
There was a problem hiding this comment.
The rust docs have this to say on the topic of raw-pointer to function-pointer transmutation:
Note that all of this is not portable to platforms where function pointers and data pointers have different sizes.
Is this something we are concerned about? If not, it would probably be good to mention it in or around the safety comment there.
There was a problem hiding this comment.
Added a comment. I haven't added a workaround for those platforms yet because that would need a cfg and not just a branch on a constant, and I don't have a list of those platforms (if they exist yet).
There was a problem hiding this comment.
There's a crate called atomic_fn that does gross int-to-pointer casting but which seems to solve the size issue. I don't think we need to use it, merely mentioning it for posterity in case this becomes an issue later.
|
In theory, the only performance effects of this PR vs having the flag turned off would be non-local effects from not being able to inline the panic if LLVM doesn't recognize that the static variable is never written to – and of course the ever present possibility that LLVM happens to end up optimizing some things differently that any change can have. I did some benchmarking against the baseline of main with the Comparing that baseline to main with I got the following results just from comparing the baseline to a re-run of itself: So even here, where there should be no differences, there were >10% differences for some benchmarks; I suspect this difference comes simply from random changes in memory layout¹. Another thing: The benchmarks are single-threaded. And while In conclusion, uhh, I don't really know man. I don't think we can claim (in the single threaded case) that ¹: Here's a nice talk that, among other things, explains how much of an effect this can have |
|
This benchmark is not totally unsurprising. The inlined branchless uncontested acquire load in this pr should have very good performance on most systems, at least on paper, and it's pretty obviously sound. My only concern would be support for fn pointer atomics on exotic platforms, but if that does become an issue I think there's some more unsafe stuff we can do with fences. I'm in favor of removing the flag, for better UX and more consistent performance. |
crates/bevy_ecs/src/error/mod.rs
Outdated
| @@ -48,8 +47,7 @@ | |||
| //! } | |||
| //! | |||
| //! fn main() { | |||
| //! // This requires the "configurable_error_handler" feature to be enabled to be in scope. | |||
| //! GLOBAL_ERROR_HANDLER.set(my_error_handler).expect("The error handler can only be set once."); | |||
| //! set_global_error_handler(my_error_handler); | |||
There was a problem hiding this comment.
This should be set_global_default_error_handler right? Though maybe we should go with set_global_error_handler anyways for brevity.
There was a problem hiding this comment.
This should be set_global_default_error_handler right?
Yup, thanks!
Though maybe we should go with set_global_error_handler anyways for brevity.
Brevity isn't that important for a function that's only called once, but no strong opinion from me.
(relaxed load)
We can go back to a |
|
I'm a little surprised that there is that much of a performance impact on looking up the error handler. The default behavior is to panic, so I would expect it to be needed at most once! ... Ah, I see, the calls to Would it work to just.. move the Alternately, could we store it on the |
Well, it's not that clear whether/how much overhead it caused in the first place.
Yes, we totally can, great call. I'll update this PR to do that. |
This will make configuring the error handling in the render world and other sub-apps more difficult. |
|
and also let us have different error handling depending on each app... but I would prefer to do that in another PR if we want to get this one in the 0.16 |
SpecificProtagonist
force-pushed
the
atomic-default-error-handler
branch
from
e7e9e61 to
b3f8707
Compare
|
PR for moving the default error handler into Also rebased on main to resolve conflict. |
SpecificProtagonist
force-pushed
the
atomic-default-error-handler
branch
from
b3f8707 to
d5304dc
Compare
There was a problem hiding this comment.
Looks good! The API is simpler, the atomics look sound, and the transmute to a function pointer is literally the first example of valid ways to use transmute.
| // The error handler must have been already set from the perspective of this thread, | ||
| // otherwise we will panic. It will never be updated after this point. | ||
| // We therefore only need a relaxed load. | ||
| let ptr = HANDLER.load(Relaxed); |
There was a problem hiding this comment.
Using Relaxed operations on a pointer makes me nervous, but I think this is okay for anything but edge cases.
The danger would be that a user wants some run-time configuration for their global error handler, and writes to a global variable before calling set, which then doesn't get synchronized with the load. But writes to a global variable will require their own synchronization, so it's hard to come up with a situation where that leads to a data race, and doing so would require unsafe code elsewhere. (It might also be possible to get into trouble using runtime code generation? But nobody would ever generate an error handler function that way.)
I'd still be inclined to use Acquire here (and Release for the store) to be safer. Acquire loads aren't usually any slower than relaxed ones. In particular, all loads on x86 have Acquire semantics, so they'll compile to the same machine instruction.
To expand on this in case it's worth considering as an alternative: If we add a layer of indirection, then I think this could be as simple as: #[inline]
pub fn default_error_handler() -> fn(BevyError, ErrorContext) {
default_error_handler_inner
}
fn default_error_handler_inner(err: BevyError, context: ErrorContext) {
GLOBAL_ERROR_HANDLER.get_or_init(|| panic)(err, context);
}(Although I think we'd still want to introduce a |
|
I'd feel more comfortable moving this to 0.17:
|
|
Pretty much agreed. I don't its crucial to ship this change in 0.16, though I do still think it's an improvement. |
Custom error handlers is going to be pretty niche, but wanting errors to be logged as warnings instead of crashing (usually in a release build) is a pretty commonly raised complaint in the community. This is essentially the one single purpose the custom error handler feature enables (I'm not aware of any other real-world use-cases AFAIK) Could the warn-dont-crash behavior be enabled via something like a feature flag? |
Currently you have to set a feature flag (which may cause a small performance regression in commands) and then put a function pointer in a once-lock. Cart is proposing shipping this behavior in 0.16 instead of the new behavior introduced in these PRs. |
|
I wanted to avoid adding this feature flag since I can't prove that it's necessary, but given that this PR won't be in 0.16, I prefer closing it in favor of moving the global state into world. Feel free to reopen should we decide against that :) |
|
Thanks for all your hard work! |
Objective
Bevy allows globally overwriting the default error handler. However, this requires taking a common exclusive lock, leading to some performance penalties. Because of these, this ability was gated behind the
configurable_error_handlerfeature.Solution
Because error handlers are function pointers, we do not need to take a lock, we can use atomics; because it is only written to beforehand, we can use relaxed loads which generally are equivalent in performance to normal loads (e.g. compiles down to
movqon amd64).Testing
Fallible params example.