search

revery-ui / revery

:zap: Native, high-performance, cross-platform desktop apps - built with Reason!
https://www.outrunlabs.com/revery/
MIT License
8.07k stars 197 forks source link

fix(sdl2): Remove some unsafe acquire/release calls #1072

Closed bryphe closed 3 years ago

bryphe commented 3 years ago

Issue: From investigating https://github.com/ocaml/ocaml/issues/10422 - it looks like there are some uses of caml_acquire_runtime/caml_release_runtime that are actually unsafe - some callbacks, like the log or hit test callbacks, can actually be dispatched from these in certain platforms.

Fix: Remove these unsafe release/acquire pairs around the SDL2 functions that can dispatch events.

zbaylin commented 3 years ago

I know this is a "necessary evil", but is there any benchmark-able performance impact (just curious)? Also I wonder why this was exposed with 4.12.0

bryphe commented 3 years ago

Good questions @zbaylin -

but is there any benchmark-able performance impact (just curious)?

From my profiling, the pollevents/waittimeout/etc aren't on the hot-path - that's rendering (and GC!)

I think what's more pressing for these is to have them actually wait instead of burning CPU (something like https://github.com/libsdl-org/SDL/pull/4180)

Also I wonder why this was exposed with 4.12.0

Some extra details here - but it seems the thread-local state management introduced in https://github.com/ocaml/ocaml/pull/9674 makes it more likely for us to hit a crash during an allocation outside of acquire/release. Without 4.12 - it's still unsafe and could potentially crash, just less likely 😬

I was also wondering if this could let us bring back https://github.com/revery-ui/reason-sdl2/pull/57/files - but I think that still have the issue of not being able to differentiate if the runtime is already acquired or not.

zbaylin commented 3 years ago

From my profiling, the pollevents/waittimeout/etc aren't on the hot-path - that's rendering (and GC!)

I think what's more pressing for these is to have them actually wait instead of burning CPU (something like libsdl-org/SDL#4180)

Gotcha -- that makes sense. My memory of the rendering pipeline is a little hazy so I didn't know if we were doing any rendering in those codepaths, but makes sense!

Some extra details here - but it seems the thread-local state management introduced in ocaml/ocaml#9674 makes it more likely for us to hit a crash during an allocation outside of acquire/release. Without 4.12 - it's still unsafe and could potentially crash, just less likely 😬

Oof, that's rough. I wonder if this means we're able to close out some of the random crash bugs in Oni!

I was also wondering if this could let us bring back https://github.com/revery-ui/reason-sdl2/pull/57/files - but I think that still have the issue of not being able to differentiate if the runtime is already acquired or not.

Oh that's a great point. I never did get a response to my forum question here: https://discuss.ocaml.org/t/determining-if-the-runtime-system-is-currently-acquired-from-c/6934. I tried to write a PR to the compiler to add in functionality to do that, but I got lost writing tests for it 😳

bryphe commented 3 years ago

Thanks for looking this over, @zbaylin !

zbaylin commented 3 years ago

I've been thinking about it and this PR opens up some things that weren't possible in the past, namely continuous resize (#1077) and right-click menus on macOS! I think there are a lot of cases now where the ambiguity of "do we have the runtime lock?" is erased. 🎉