Change CallbackGuard to require explicit destruction
#250
Conversation
This commit alters the API of the `CallbackGuard` in a semver-breaking way. Previously the guard would abort the process if the current thread were panicking when it was dropped. After this change, however, the `thread::panicking` function isn't used and instead an explicit call to destruction is required. If a `CallbackGuard` is dropped "normally" (aka the compiler auto-drops it) then that likely indicates a panic (or a bug) and the process will be aborted. This tweak brings with it a few benefits: * LLVM can optimize this much better as it can infer that a chunk of code between the creation of a guard and its drop doesn't panic. Previously it would not optimize away creation/dropping the guard but after this change LLVM should be able to optimize this away. Put another way, after this commit LLVM can optimize away usage of `CallbackGuard` if it can prove the code won't panic. * Runtime-wise this should be a bit cheaper on the "fast path" as checking `thread::panicking` requires... well... at least a function call! By calling `defuse` though it's a noop at runtime which means that no extra code happens on the fast path when a panic doens't happen. * Finally, a slightly more esoteric benefit, but if a thread is panicking *already* and during a destructor some glib calls were made, this strategy will no longer abort the process. Ideally the callback guard only aborts the process if the state of the thread panicking *changes* rather than only is true on exit. This is likely to only come up pretty rarely, though!
|
@alexcrichton Thanks. |
|
Yes, but let's get it in right after the next release. This requires changes in the code generator and manual code using this currently, but improves compiler output a lot. |
|
For the next release, I'd just like to have this PR getting merged first then I'll do it. It already has a lot changes and waiting more would be problematic I think. |
|
From what I understand, all this catching of panics at the FFI boundary is not necessary anymore now that rust-lang/rust#46833 is merged. We could get rid of all the @alexcrichton can you confirm? |
|
@sdroege that is indeed correct! |
|
Great, thanks for confirming :) then let's just go with that instead of making things more complex |
|
So what should we do in here? Moving forward? |
|
With 1.24 we can get rid of the callback_guard stuff completely as the compiler takes care of that already. |
|
1.24 is out, we can remove all the callback guards \o/ |
|
@sdroege I will do it tomorrow, if you don't started already. |
|
Please go ahead :) I would otherwise do it some time next week |
|
@GuillaumeGomez This seems can be closed now |
|
Indeed. |
|
Unfortunately Rust 1.24.1 reverts this change because Windows proves to be an enemy of progress again: rust-lang/rust#48251 So for 1.24 and 1.25 these changes should all be fine, for 1.24.1 not. |
|
Sad story |
|
Or might not happen. Let's wait |
|
It did happen, so we need to revert these until 1.25 |
|
Or postpone crate release? |
|
That would mean another 6 weeks or 12 in the worst case if they don't manage to put it into 1.25. |
|
@GuillaumeGomez so we revert changes in gir and all your crates? |
|
I suppose... I'm really depressed by this issue... |
|
Me too, that's why I didn't send any PR yet. It's also IMHO the wrong decision by the Rust team. They break backwards compatibility with 1.24 features to make something work again that only worked by pure luck before due to how undefined behaviour was at that point. |
|
Yes, however we don't have much to say about this... I understand their position, I just don't agree with. :p |
This commit alters the API of the
CallbackGuardin a semver-breakingway. Previously the guard would abort the process if the current thread
were panicking when it was dropped. After this change, however, the
thread::panickingfunction isn't used and instead an explicit call todestruction is required. If a
CallbackGuardis dropped "normally" (akathe compiler auto-drops it) then that likely indicates a panic (or a
bug) and the process will be aborted.
This tweak brings with it a few benefits:
LLVM can optimize this much better as it can infer that a chunk of
code between the creation of a guard and its drop doesn't panic.
Previously it would not optimize away creation/dropping the guard but
after this change LLVM should be able to optimize this away. Put
another way, after this commit LLVM can optimize away usage of
CallbackGuardif it can prove the code won't panic.Runtime-wise this should be a bit cheaper on the "fast path" as
checking
thread::panickingrequires... well... at least a functioncall! By calling
defusethough it's a noop at runtime which meansthat no extra code happens on the fast path when a panic doens't
happen.
Finally, a slightly more esoteric benefit, but if a thread is
panicking already and during a destructor some glib calls were made,
this strategy will no longer abort the process. Ideally the callback
guard only aborts the process if the state of the thread panicking
changes rather than only is true on exit. This is likely to only
come up pretty rarely, though!