Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support async (emterpreter/asyncify) #70

Closed
Beuc opened this issue Jan 22, 2019 · 25 comments
Closed

Support async (emterpreter/asyncify) #70

Beuc opened this issue Jan 22, 2019 · 25 comments

Comments

@Beuc
Copy link
Contributor

@Beuc Beuc commented Jan 22, 2019

Currently SDL2 needs to be locally patched in order to leverage Emterpreter's emscripten_sleep().
Ideally SDL2 would support Emterpreter out of the box.

See https://groups.google.com/forum/#!topic/emscripten-discuss/v1w6PGYhNOA for a rationale.

For reference, here is my local patch:

diff --git a/src/timer/unix/SDL_systimer.c b/src/timer/unix/SDL_systimer.c
index 159bda5c5..222a7455b 100644
--- a/src/timer/unix/SDL_systimer.c
+++ b/src/timer/unix/SDL_systimer.c
@@ -31,6 +31,8 @@
 #include "SDL_assert.h"
 #include "../SDL_timer_c.h"
 
+#include <emscripten.h>
+
 /* The clock_gettime provides monotonous time, so we should use it if
    it's available. The clock_gettime function is behind ifdef
    for __USE_POSIX199309
@@ -187,6 +189,10 @@ SDL_GetPerformanceFrequency(void)
 void
 SDL_Delay(Uint32 ms)
 {
+#ifdef __EMSCRIPTEN__
+    emscripten_sleep_with_yield(ms);
+    return;
+#else
     int was_error;
 
 #if HAVE_NANOSLEEP
@@ -225,6 +231,7 @@ SDL_Delay(Uint32 ms)
         was_error = select(0, NULL, NULL, NULL, &tv);
 #endif /* HAVE_NANOSLEEP */
     } while (was_error && (errno == EINTR));
+#endif /* __EMSCRIPTEN__ */
 }
 
 #endif /* SDL_TIMER_UNIX */
diff --git a/src/video/emscripten/SDL_emscriptenopengles.c b/src/video/emscripten/SDL_emscriptenopengles.c
index fe5e07342..6cd6a0e5f 100644
--- a/src/video/emscripten/SDL_emscriptenopengles.c
+++ b/src/video/emscripten/SDL_emscriptenopengles.c
@@ -95,9 +95,16 @@ Emscripten_GLES_DeleteContext(_THIS, SDL_GLContext context)
 }
 
 SDL_EGL_CreateContext_impl(Emscripten)
-SDL_EGL_SwapWindow_impl(Emscripten)
 SDL_EGL_MakeCurrent_impl(Emscripten)
 
+int
+Emscripten_GLES_SwapWindow(_THIS, SDL_Window * window)
+{
+    EGLBoolean ret = SDL_EGL_SwapBuffers(_this, ((SDL_WindowData *) window->driverdata)->egl_surface);
+    emscripten_sleep_with_yield(0);
+    return ret;
+}
+
 void
 Emscripten_GLES_GetDrawableSize(_THIS, SDL_Window * window, int * w, int * h)
 {

Emterpreter whitelist:

-s EMTERPRETIFY_WHITELIST='[..., "_SDL_WaitEvent", "_SDL_WaitEventTimeout", "_SDL_Delay", "_SDL_RenderPresent", "_GLES2_RenderPresent", "_SDL_GL_SwapWindow", "_Emscripten_GLES_SwapWindow", ...]'
@Daft-Freak
Copy link
Member

@Daft-Freak Daft-Freak commented Jan 22, 2019

Hmm... Maybe we could make nanosleep call emscripten_sleep_with_yield when using Emterpreter? (Possibly with a new option)

@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Jan 22, 2019

Maybe.
That wouldn't distinguish between emscripten_sleep and emscripten_sleep_with_yield.
Also that wouldn't take care of SwapWindow.

@Daft-Freak
Copy link
Member

@Daft-Freak Daft-Freak commented Jan 22, 2019

Wouldn't we have the yield vs non-yield issue regardless of where we implemented this though? That was part of why I was suggesting an option (something like -s YIELD_IN_NANOSLEEP=1). For SwapWindow, we could do something in eglSwapBuffers. (Though, we'll need something for the software renderer as well).

@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Feb 1, 2019

An issue with adding code in nanosleep is that libc.bc is cached. This needs additional logic in Emscripten to ensure a new libc variant is used.

Another issue with eglSwapBuffers is that it's implemented in JavaScript, and resuming to JavaScript is not supported by Emterpreter. I used to add a emscripten_sleep_with_yield in library_elg.js and that appeared to work, but the behavior is not specified. That's why I moved to call back to SDL2, in C.
I keep getting random Emterpreter crashes in my application so I'd rather not take chances.

For reference, Alon suggests (cf. mailing-list link) implementing a hook in SDL2 that would be initialized with a no-op and redefined at run-time.

I don't have a definite opinion on this.

@Daft-Freak
Copy link
Member

@Daft-Freak Daft-Freak commented Feb 1, 2019

I don't know much about Emterpreter, so @kripken probably has a better idea of what would work here than me. As long as whatever we come up with doesn't require multiple SDL builds I don't really mind where it's done.

@kripken
Copy link
Member

@kripken kripken commented Feb 1, 2019

I think we can apply that patch to SDL, so that emscripten_sleep_with_yield is called in all those cases. And we can make emscripten do nothing in that function when the emterpreter is disabled (that may already be the case now - if not, we just need to add it as an empty function), and the normal emterpreter behavior otherwise. That should just work with a single SDL2 build, and the user specifies -s EMTERPRETIFY=1 at link time optionally.

@Daft-Freak
Copy link
Member

@Daft-Freak Daft-Freak commented Feb 4, 2019

We would still need the sleep in SDL_Delay to delay in a non-emterpreter build though. (And we'll need an emscripten_sleep_with_yield in Emscripten_UpdateWindowFramebuffer for the software renderer.)

@Beuc Beuc changed the title Support Emterpreter Support async (emterpreter/asyncify) Jul 25, 2019
@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Aug 27, 2019

I worked again on async support with kripken's callback idea
(though given that Emterpreter is being deprecated, maybe we could just call emscripten_sleep() for simplicity).

Here is a crude patch. I am not sure what kind of API we want to tell SDL we're enabling the callback
(or more generally trigger async support).
Here it's a global emscripten_sdl_async_callback symbol that is NULL by default.

With this a single SDL2 build supports normal and async modes. Thoughts?

diff --git a/src/timer/unix/SDL_systimer.c b/src/timer/unix/SDL_systimer.c
index 5045996f3..f53abad9f 100644
--- a/src/timer/unix/SDL_systimer.c
+++ b/src/timer/unix/SDL_systimer.c
@@ -31,6 +31,8 @@
 #include "SDL_assert.h"
 #include "../SDL_timer_c.h"
 
+#include <emscripten.h>
+
 /* The clock_gettime provides monotonous time, so we should use it if
    it's available. The clock_gettime function is behind ifdef
    for __USE_POSIX199309
@@ -184,9 +186,18 @@ SDL_GetPerformanceFrequency(void)
     return 1000000;
 }
 
+#ifdef __EMSCRIPTEN__
+void (*emscripten_sdl_async_callback)(Uint32) = 0;
+#endif
 void
 SDL_Delay(Uint32 ms)
 {
+#ifdef __EMSCRIPTEN__
+    if (emscripten_sdl_async_callback) {
+        emscripten_sdl_async_callback(ms);
+        return;
+    }
+#else
     int was_error;
 
 #if HAVE_NANOSLEEP
@@ -225,6 +236,7 @@ SDL_Delay(Uint32 ms)
         was_error = select(0, NULL, NULL, NULL, &tv);
 #endif /* HAVE_NANOSLEEP */
     } while (was_error && (errno == EINTR));
+#endif /* __EMSCRIPTEN__ */
 }
 
 #endif /* SDL_TIMER_UNIX */
diff --git a/src/video/emscripten/SDL_emscriptenframebuffer.c b/src/video/emscripten/SDL_emscriptenframebuffer.c
index bfdec3b56..c9014f683 100644
--- a/src/video/emscripten/SDL_emscriptenframebuffer.c
+++ b/src/video/emscripten/SDL_emscriptenframebuffer.c
@@ -56,6 +56,8 @@ int Emscripten_CreateWindowFramebuffer(_THIS, SDL_Window * window, Uint32 * form
     return 0;
 }
 
+extern void (*emscripten_sdl_async_callback)(Uint32);
+
 int Emscripten_UpdateWindowFramebuffer(_THIS, SDL_Window * window, const SDL_Rect * rects, int numrects)
 {
     SDL_Surface *surface;
@@ -162,6 +164,11 @@ int Emscripten_UpdateWindowFramebuffer(_THIS, SDL_Window * window, const SDL_Rec
                      SDL_GetWindowID(window), ++frame_number);
         SDL_SaveBMP(surface, file);
     }*/
+
+    if (emscripten_sdl_async_callback) {
+        emscripten_sdl_async_callback(0);
+    }
+
     return 0;
 }
 
diff --git a/src/video/emscripten/SDL_emscriptenopengles.c b/src/video/emscripten/SDL_emscriptenopengles.c
index f83954cfa..fd49a55f8 100644
--- a/src/video/emscripten/SDL_emscriptenopengles.c
+++ b/src/video/emscripten/SDL_emscriptenopengles.c
@@ -82,9 +82,20 @@ Emscripten_GLES_LoadLibrary(_THIS, const char *path) {
 }
 
 SDL_EGL_CreateContext_impl(Emscripten)
-SDL_EGL_SwapWindow_impl(Emscripten)
 SDL_EGL_MakeCurrent_impl(Emscripten)
 
+extern void (*emscripten_sdl_async_callback)(Uint32);
+
+int
+Emscripten_GLES_SwapWindow(_THIS, SDL_Window * window)
+{
+    EGLBoolean ret = SDL_EGL_SwapBuffers(_this, ((SDL_WindowData *) window->driverdata)->egl_surface);
+    if (emscripten_sdl_async_callback) {
+        emscripten_sdl_async_callback(0);
+    }
+    return ret;
+}
+
 void
 Emscripten_GLES_GetDrawableSize(_THIS, SDL_Window * window, int * w, int * h)
 {
@kripken
Copy link
Member

@kripken kripken commented Aug 29, 2019

Would it be useful if we had an emscripten API that checked if asyncify or emterpreter-async are enabled, so that emscripten_sleep will work as expected?

@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Aug 29, 2019

Do you mean we'd unconditionally call emscripten_sleep() on SDL_Delay or buffer swaps?
So we block in sync builds (no-op for 0ms) and return to browser in async builds?
That sounds good to me.
For Emterpreter I'd need emscripten_sleep_with_yield instead though so that complexifies things; but I think I could live with a local patch.
@Daft-Freak ?

@Daft-Freak
Copy link
Member

@Daft-Freak Daft-Freak commented Aug 30, 2019

emscripten_sleep falling back for non-async would work. Though I think @kripken was suggesting something like:

if(emscripten_is_async())
    emscripten_sleep(...);

?

@kripken
Copy link
Member

@kripken kripken commented Aug 30, 2019

I was suggesting that last thing, yeah. I did think earlier that maybe a single call for both async and non-async is ok, and we'd either do nothing in the non-async case, or synchronously block, but I think both of those are too dangerous - things just won't work in general.

So how does this sound: emscripten_sleep throws if called from a non-async build, and emscripten_is_async says whether this is an async build (in which it is ok to call emscripten_sleep)?

@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Sep 4, 2019

(I thought I had answered, must have closed the window :/)
Generally speaking I would welcome ways to detect async at runtime, and at compile-time (#define's) as well.
So that sounds like a good thing to do - do you intend to do it or do you need a PR?

About: hard-coding emscripten_sleep (as opposed to Emterpreter / emscripten_sleep_with_yield), unlike what I wrote earlier, I actually have yet to achieve a working build of my project with new-asyncify, so I'm not sure we're in position to do that yet.

@kripken
Copy link
Member

@kripken kripken commented Sep 5, 2019

@Beuc - a PR would be great! (Otherwise I can get to it eventually, but have a lot on my plate.)

I think it has to be runtime, since ASYNCIFY etc. are link-time options - a source file can be compiled before we know if the project will use ASYNCIFY.

Name-wise, perhaps emscripten_has_asyncify()? (or _supports_, _uses_?) We can make it answer yes for EMTERPRETIFY too, but as that's in fastcomp which is deprecated, seems nice to make a future-focused name.

@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Oct 29, 2019

By making SDL2 synchronous or pseudo-synchronous depending directly on '-s ASYNCIFY', we enforce emscripten_sleep() even if the user enabled it for e.g. a emscripten_wget() on start-up but then switches to an asynchronous main game loop (emscripten_set_main_loop).

I'm not sure a lot of users will use Emscripten Asyncify like that, but if the behavior is unwanted, they'll have to patch SDL2.
WDYT?

In any case, I'll send a PR for emscripten_has_asyncify() which is useful on its own.

@kripken
Copy link
Member

@kripken kripken commented Oct 29, 2019

@Beuc Sorry, I didn't understand the point you're making about sleep, the main loop, and needing to patch SDL2?

Beuc added a commit to Beuc/emscripten that referenced this issue Oct 30, 2019
@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Oct 30, 2019

Sorry, let me reformulate.

Case 1 (that we're currently addressing):

int main(void) {
  init();
  while (complex condition) {
    deep_nested_func();
  }
}
void init() {}
void deep_nested_func() {
  SDL_RenderPresent();  // should call emscripten_sleep();
}

We compile with -s ASYNCIFY=1 -s ASYNCIFY_WHITELIST=[trace,to,deep_nested_loop,...], SDL2 detects that and calls emscripten_sleep(). All good.

Case 2 (that I attempted to describe):

int main(void) {
  init();
  emscripten_set_main_loop(one_loop);
}
void init() {
  emscripten_wget("/remote_bundle.dat", "./local_bundle.dat");
  load_bundle();
}
void one_loop() {
  deep_nested_func();
}
void deep_nested_func() {
  SDL_RenderPresent();  // should /not/ call emscripten_sleep();
}

The user compiles with -s ASYNCIFY=1 -s ASYNCIFY_WHITELIST=[init]. They only used ASYNCIFY for the pseudo-synchronous wget. In this case SDL2 should not sleep, as this would require asyncify-ing all the functions leading to deep_nested_func, with a performance hit.

We can ignore case 2, otherwise we may need a new API to tell SDL2 whether it should call emscripten_sleep().

@kripken
Copy link
Member

@kripken kripken commented Oct 31, 2019

I see. Yeah, I guess we should ignore case 2 for now. If it's an issue then we can look for a solution for that specifically. But I'm not too worried, anyone turning on Asyncify should expect some perf decrease anyhow.

kripken added a commit to emscripten-core/emscripten that referenced this issue Oct 31, 2019
Checks if ASYNCIFY was used in the build.

This can help SDL decide how to sleep, Cf. emscripten-ports/SDL2#70
@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Nov 1, 2019

I pushed what we discussed!

@Daft-Freak
Copy link
Member

@Daft-Freak Daft-Freak commented Nov 5, 2019

If we need to make this optional in future, we could use a hint like we have for keyboard events.

@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Nov 6, 2019

Good idea :)

@Beuc Beuc closed this Nov 6, 2019
@Ghabry
Copy link

@Ghabry Ghabry commented Jan 14, 2020

I want to discuss "case 2" because I already have this issue now.

Our code has only one path that after a while reaches a sleep statement which is responsible for yielding back to the browser:

main() -> while (!exit) MainFunction() -> Update() -> Sleep()

My first testing with asynchify was in September with ASYNCIFY_IGNORE_INDIRECT and there it worked perfectly.
Now in January I always got "unreachable" and I had to disable the indirect option, this appears to be caused by the PR closing this issue.

Our codepath to SDL2 has indirection because we are multiplatform and allow the use of different backends (implemented as child classes), so there is an indirection here

main() -> while (!exit) MainFunction() ->
GetDisplayUi() [returns SDL2Ui] -> Sdl2Ui->UpdateDisplay() 

Is there a good solution to get this working or can you tell me a way to instrument ASYNCHIFY-callstacks so I know what I have to add to the whitelist? (the indirect callpath to Sdl2-update is short, so I would be fine with this solution).

@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Jan 14, 2020

For reference, my SDL-related whitelist:

-s ASYNCIFY_WHITELIST='["main", "SDL_WaitEvent", "SDL_WaitEventTimeout", "SDL_Delay", "SDL_RenderPresent", "GLES2_RenderPresent", "SDL_GL_SwapWindow", "Emscripten_GLES_SwapWindow", "byn$$fpcast-emu$$Emscripten_GLES_SwapWindow", "SDL_UpdateWindowSurface", "SDL_UpdateWindowSurfaceRects", "Emscripten_UpdateWindowFramebuffer"]'
@kripken
Copy link
Member

@kripken kripken commented Jan 15, 2020

Maybe it would be useful to automatically add that in emscripten to the whitelist if SDL2 + ASYNCIFY are enabled?

@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Jan 18, 2020

TBH I didn't get what @Ghabry 's use case was, precisely (I'm not sure this is really case #2, more likely case #1 and an Update() function that is now redundant with the new SDL2 behavior).

Adding these funcs to ASYNCIFY_WHITELIST automatically could help but isn't enough, since the call path between main and SDL_WaitEvent/SDL_Delay/SDL_RenderPresent needs to be specified as well. Maybe there's somewhere to document it.

belraquib added a commit to belraquib/emscripten that referenced this issue Dec 23, 2020
Checks if ASYNCIFY was used in the build.

This can help SDL decide how to sleep, Cf. emscripten-ports/SDL2#70
haberbyte added a commit to haberbyte/emscripten that referenced this issue Jan 31, 2021
Checks if ASYNCIFY was used in the build.

This can help SDL decide how to sleep, Cf. emscripten-ports/SDL2#70
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants