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

Add proxy_to_pthread option to platform=web #79711

Merged

Conversation

adamscott
Copy link
Member

@adamscott adamscott commented Jul 20, 2023

This PR adds the option proxy_to_pthread to the web platform (with proxy_to_pthread=yes as default value).

The emscripten library compiles C/C++ apps to WASM. Unfortunately, WASM, when ran in the main thread, runs synchronously, making it block the main thread. That makes it so that if the application is in a computing intensive task, it will block the page main thread, affecting the UI and the usability of that page.

When compiled with this PR, users can use the web page (like entering text in a form) without any noticeable issues. Currently, it is impossible to write such text while the engine is loading. See that Firefox profiler as an example.

image
See the red line in the Isolate Web Content row, user input is impossible during that time

Emscripten offers a linker option to run the application in a web worker, effectively fixing the issue mentioned above. Web workers are the equivalent of threads for the web platform.

With my PR, if the user add the do_no_block_main_thread=yes option to it's platform=web scons build, the builder adds -s PROXY_TO_PTHREAD to the linker options to create a stub main() function that will act as a proxy to the newly created main web worker.

Note
My PR will add the -s TEXTDECODER=0 option to the linker too, as there was non-consistent issues about internal emscripten calls. The suggested solution from the developer seems to deactivate TEXTDECODER, as there no big impact on the usability of the application.

The Javascript C API ("platform/web/js/libs") had to be updated to specify the __proxy setting (now set to sync). This is because otherwise, the web worker will use it's default Module values that were initialized with the web worker. But all the data set before the initialization still reside in the main thread.

Setting the value sync to each function __proxy option makes it so that emscripten knows that the C API Javascript function must be run on the main thread instead of in the main web worker.


Here's a demo. Don't mind the non-fluid mouse movement, it's related to the screen capture, it was super smooth while recording. But notice how the HTML UI react even if either the editor blocks the thread when loading (as usual) or while the game blocks the thread each frame for a whole second.

Capture.video.du.2023-07-29.14-31-18.webm

TL;DR Your game will still freeze under heavy processing on the main thread. But, the web page hosting the game will not freeze, as the Godot game will run in it's own thread instead of running on the web page's main thread.


Note
This PR needs testing. There's some issues known issues about this PR. (when fixed, this PR will drop it's draft status.)

  • input is broken. (fixed)

@adamscott adamscott added this to the 4.x milestone Jul 20, 2023
@adamscott adamscott force-pushed the web-non-blocking-main-thread branch 2 times, most recently from 14fe417 to 4f94666 Compare July 20, 2023 17:20
@adamscott adamscott marked this pull request as ready for review July 20, 2023 17:20
@adamscott adamscott requested review from a team as code owners July 20, 2023 17:20
@Faless
Copy link
Collaborator

Faless commented Jul 20, 2023

Woah! That's really amazing 💙 , I've been wanting to do this for a looooooong time!
It'll need some testing, but it's looking great 🏆 !

@dsnopek
Copy link
Contributor

dsnopek commented Jul 20, 2023

Thanks so much for taking this on!

Setting the value sync to each function __proxy option makes it so that emscripten knows that the C API Javascript function must be run on the main thread instead of in the main web worker.

There's some Javascript code elsewhere in Godot's code base, for example in these modules:

  • websocket
  • webrtc
  • webxr

If you haven't checked already, it'd be good to double check that they don't also need __proxy added.

@adamscott
Copy link
Member Author

There's some Javascript code elsewhere in Godot's code base, for example in these modules:

  • websocket
  • webrtc
  • webxr

Websocket and WebRTC changes are already in my PR.

And for WebXR, a certain @dsnopek already added the __proxy prop to sync, 3 years ago. In fact, I should thank you for having me realize that the __proxy setting vaguely mentioned in the emscripten docs was to set the value as you did. 🙃

@dsnopek
Copy link
Contributor

dsnopek commented Jul 20, 2023

Websocket and WebRTC changes are already in my PR.

Ack, sorry, not sure how I glossed over that :-)

@adamscott adamscott force-pushed the web-non-blocking-main-thread branch 3 times, most recently from b165a84 to e7a0e02 Compare July 26, 2023 10:13
@adamscott adamscott force-pushed the web-non-blocking-main-thread branch 4 times, most recently from 4d63e95 to fe4b9c4 Compare July 29, 2023 18:39
@adamscott
Copy link
Member Author

I added a demo in the PR description that shows my PR load successfully the editor and runs a game without blocking the main Javascript thread.

@adamscott adamscott force-pushed the web-non-blocking-main-thread branch 4 times, most recently from 5e3821f to 7030c0c Compare August 3, 2023 17:01
@akien-mga
Copy link
Member

That looks really cool, thanks for working on it!

Some minor comments:

  • do_not_block_main_thread is a mouthful, we don't really have options which are that verbose / full sentences like this. It's not a big deal but maybe proxy_to_pthread would be more fitting, and matches the Emscripten option and the PROXY_TO_PTHREAD_ENABLED define? It's also long-ish, but 3 words vs 5 is nice ;)
  • Are there specific concerns to make this the default behavior, i.e. opt-out instead of opt-in?

@Faless
Copy link
Collaborator

Faless commented Aug 5, 2023

Once again, this is awesome 😻 .
I have been testing this, and found few issues though:

With dev_build=yes (tested on Firefox 116)

In the editor, as soon as the mouse enter the window, with the console open, I'm getting these errors (tested on Firefox):

Window::_event_callback(DisplayServer::WindowEvent)@:wasm-function[69315]:0x17d344b
void call_with_variant_args_helper<Window, DisplayServer::WindowEvent, 0ul>(Window*, void (Window::*)(DisplayServer::WindowEvent), Variant const**, Callable::CallError&, IndexSequence<0ul>)@:wasm-function[69585]:0x17f336d
CallableCustomMethodPointer<Window, DisplayServer::WindowEvent>::call(Variant const**, int, Variant&, Callable::CallError&) const@:wasm-function[69584]:0x17f31c3
Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const@:wasm-function[143572]:0x2cfaf09
DisplayServerWeb::send_window_event_callback(int)@:wasm-function[506]:0x76df9
void call_with_variant_args_static<int, 0ul>(void (*)(int), Variant const**, Callable::CallError&, IndexSequence<0ul>)@:wasm-function[997]:0x79c10
CallableCustomStaticMethodPointerRet<void, int>::call(Variant const**, int, Variant&, Callable::CallError&) const@:wasm-function[996]:0x79bd0
Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const@:wasm-function[143572]:0x2cfaf09
CallableCustomBind::call(Variant const**, int, Variant&, Callable::CallError&) const@:wasm-function[143633]:0x2cfd5da
Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const@:wasm-function[143572]:0x2cfaf09
CallQueue::_call_function(Callable const&, Variant const*, int, bool)@:wasm-function[153631]:0x2f08992
CallQueue::flush()@:wasm-function[153633]:0x2f0920b
SceneTree::physics_process(double)@:wasm-function[67938]:0x176a14c
Main::iteration()@:wasm-function[1388]:0x9b7ee
OS_Web::main_loop_iterate()@:wasm-function[1177]:0x7ee5b
main_loop_callback()@:wasm-function[1166]:0x7e98d
godot_web_main(int, char**)@:wasm-function[1167]:0x7ec4c
main@:wasm-function[1283]:0x83844
__main_void@:wasm-function[160463]:0x30f7f4f
_main_thread@:wasm-function[160465]:0x30f801c
invokeEntryPoint@http://127.0.0.1:8060/godot.editor.js:3837:37
handleMessage@http://127.0.0.1:8060/godot.editor.worker.js:124:16
godot.editor.worker.js:24:11
    threadPrintErr http://127.0.0.1:8060/godot.editor.worker.js:24
    handleMessage http://127.0.0.1:8060/godot.editor.worker.js:152

With regular builds (i.e. dev_build=no) (tested on Firefox 116, and Chromium 115)

Opening the console, and closing it, generates the following artifacts:

corrupted_size

And further resizing the window then seems to not fix the issue.

Also, and more importantly, whenever in the editor, I try to create a Mesh (to assign to a MeshInstance3D), I'm getting the following error (and the editor crashes):

Pthread 0x013e8db8 sent an error! http://127.0.0.1:8060/godot.editor.js:8677: Uncaught TypeError: GLctx.getBufferSubData is not a function
onPrintError @ godot.editor.js:13776
worker.onerror @ godot.editor.js:3713
godot.editor.js:3714 Uncaught ErrorEvent {isTrusted: true, message: 'Uncaught TypeError: GLctx.getBufferSubData is not a function', filename: 'http://127.0.0.1:8060/godot.editor.js', lineno: 8677, colno: 16, …}
worker.onerror @ godot.editor.js:3714
godot.editor.js:8677 Uncaught TypeError: GLctx.getBufferSubData is not a function
    at _glGetBufferSubData (godot.editor.js:8677:16)
    at GLES3::Utilities::buffer_get_data(unsigned int, unsigned int, unsigned int) (15a0ca8a:0xb28ef6)
    at GLES3::MeshStorage::mesh_get_surface(RID, int) const (15a0ca8a:0xaf3186)
    at RenderingServerDefault::mesh_get_surface(RID, int) const (15a0ca8a:0x2a8e463)
    at RenderingServer::mesh_surface_get_arrays(RID, int) const (15a0ca8a:0x26c3d26)
    at PrimitiveMesh::surface_get_arrays(int) const (15a0ca8a:0x22ae5e5)
    at Mesh::generate_triangle_mesh() const (15a0ca8a:0x2211391)
    at MeshInstance3DGizmoPlugin::redraw(EditorNode3DGizmo*) (15a0ca8a:0x173954b)
    at EditorNode3DGizmo::redraw() (15a0ca8a:0x145e10e)
    at Node3D::_update_gizmos() (15a0ca8a:0x1d6943d)

This might be an emscripten bug (i.e. glGetBufferSubData not being proxied to main).

@adamscott adamscott force-pushed the web-non-blocking-main-thread branch from c27b2a9 to d1863ed Compare August 22, 2023 15:25
@adamscott
Copy link
Member Author

Opening the console, and closing it, generates the following artifacts:

corrupted_size

Found a fix for that issue! I fixed it by using emscripten_set_canvas_element_size() to tell the framework to update it's internal rendering viewport. It seems that this issue don't appear when Godot runs on the main thread.

@adamscott adamscott force-pushed the web-non-blocking-main-thread branch 2 times, most recently from 025cb21 to aae0592 Compare August 23, 2023 15:02
@adamscott adamscott force-pushed the web-non-blocking-main-thread branch 3 times, most recently from 7a35924 to e17bd9c Compare August 27, 2023 14:16
platform/web/os_web.h Outdated Show resolved Hide resolved
platform/web/display_server_web.h Outdated Show resolved Hide resolved
platform/web/display_server_web.h Outdated Show resolved Hide resolved
@adamscott adamscott force-pushed the web-non-blocking-main-thread branch 2 times, most recently from 01563fc to 79b7f4f Compare September 16, 2023 18:55
@adamscott adamscott requested a review from Faless September 18, 2023 20:09
@adamscott adamscott modified the milestones: 4.x, 4.2 Oct 3, 2023
@adamscott adamscott force-pushed the web-non-blocking-main-thread branch 6 times, most recently from da255d3 to ab851c7 Compare October 5, 2023 14:20
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work 🚀 🥇 !

It seems that dlink + proxy_to_pthread is broken (needs further investigation), I suggest disabling it in that case but leave it on by default:

diff --git a/platform/web/detect.py b/platform/web/detect.py
index 4ad66d0b25..d306869889 100644
--- a/platform/web/detect.py
+++ b/platform/web/detect.py
@@ -216,6 +216,10 @@ def configure(env: "Environment"):
             env.Append(LINKFLAGS=["-Wl,-u,scalbnf"])
 
     if env["dlink_enabled"]:
+        if env["proxy_to_pthread"]:
+            print("GDExtension support requires proxy_to_pthread=no, disabling")
+            env["proxy_to_pthread"] = False
+
         if cc_semver < (3, 1, 14):
             print("GDExtension support requires emscripten >= 3.1.14, detected: %s.%s.%s" % cc_semver)
             sys.exit(255)

I'm approving because all my other tests (including testing the 2d and 3d platformer demos) worked well.

@adamscott adamscott force-pushed the web-non-blocking-main-thread branch from ab851c7 to 78c2a08 Compare October 9, 2023 15:50
@akien-mga akien-mga merged commit a28dab7 into godotengine:master Oct 9, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants