Merge pull request godotengine#90913 from RandomShaper/wtp_servers_pro

Apply additional fixes to servers' threading

Author: akien-mga

core/object/object.cpp

@@ -2307,9 +2307,9 @@ void ObjectDB::setup() {
 }
 
 void ObjectDB::cleanup() {
-	if (slot_count > 0) {
-		spin_lock.lock();
+	spin_lock.lock();
 
+	if (slot_count > 0) {
 		WARN_PRINT("ObjectDB instances leaked at exit (run with --verbose for details).");
 		if (OS::get_singleton()->is_stdout_verbose()) {
 			// Ensure calling the native classes because if a leaked instance has a script
@@ -2340,10 +2340,11 @@ void ObjectDB::cleanup() {
 			}
 			print_line("Hint: Leaked instances typically happen when nodes are removed from the scene tree (with `remove_child()`) but not freed (with `free()` or `queue_free()`).");
 		}
-		spin_lock.unlock();
 	}
 
 	if (object_slots) {
 		memfree(object_slots);
 	}
+
+	spin_lock.unlock();
 }

core/templates/command_queue_mt.h

@@ -302,7 +302,7 @@ class CommandQueueMT {
 	struct CommandBase {
 		bool sync = false;
 		virtual void call() = 0;
-		virtual ~CommandBase() = default; // Won't be called.
+		virtual ~CommandBase() = default;
 	};
 
 	struct SyncCommand : public CommandBase {
@@ -368,6 +368,10 @@ class CommandQueueMT {
 				sync_cond_var.notify_all();
 			}
 
+			// If the command involved reallocating the buffer, the address may have changed.
+			cmd = reinterpret_cast<CommandBase *>(&command_mem[flush_read_ptr]);
+			cmd->~CommandBase();
+
 			flush_read_ptr += size;
 		}
 		WorkerThreadPool::thread_exit_command_queue_mt_flush();
@@ -384,6 +388,8 @@ class CommandQueueMT {
 		} while (sync_head != sync_head_goal); // Can't use lower-than because of wraparound.
 	}
 
+	void _no_op() {}
+
 public:
 	void lock();
 	void unlock();
@@ -405,18 +411,25 @@ class CommandQueueMT {
 			_flush();
 		}
 	}
+
 	void flush_all() {
 		_flush();
 	}
 
+	void sync() {
+		push_and_sync(this, &CommandQueueMT::_no_op);
+	}
+
 	void wait_and_flush() {
 		ERR_FAIL_COND(pump_task_id == WorkerThreadPool::INVALID_TASK_ID);
 		WorkerThreadPool::get_singleton()->wait_for_task_completion(pump_task_id);
 		_flush();
 	}
 
 	void set_pump_task_id(WorkerThreadPool::TaskID p_task_id) {
+		lock();
 		pump_task_id = p_task_id;
+		unlock();
 	}
 
 	CommandQueueMT();

drivers/gles3/rasterizer_gles3.cpp

@@ -395,10 +395,13 @@ void RasterizerGLES3::_blit_render_target_to_screen(RID p_render_target, Display
 	glBindFramebuffer(GL_DRAW_FRAMEBUFFER, GLES3::TextureStorage::system_fbo);
 
 	if (p_first) {
-		Size2i win_size = DisplayServer::get_singleton()->window_get_size();
 		if (p_screen_rect.position != Vector2() || p_screen_rect.size != rt->size) {
 			// Viewport doesn't cover entire window so clear window to black before blitting.
-			glViewport(0, 0, win_size.width, win_size.height);
+			// Querying the actual window size from the DisplayServer would deadlock in separate render thread mode,
+			// so let's set the biggest viewport the implementation supports, to be sure the window is fully covered.
+			GLsizei max_vp[2] = {};
+			glGetIntegerv(GL_MAX_VIEWPORT_DIMS, max_vp);
+			glViewport(0, 0, max_vp[0], max_vp[1]);
 			glClearColor(0.0, 0.0, 0.0, 1.0);
 			glClear(GL_COLOR_BUFFER_BIT);
 		}

drivers/gles3/storage/texture_storage.cpp

@@ -2312,9 +2312,11 @@ void TextureStorage::_clear_render_target(RenderTarget *rt) {
 
 	if (rt->backbuffer_fbo != 0) {
 		glDeleteFramebuffers(1, &rt->backbuffer_fbo);
+		rt->backbuffer_fbo = 0;
+	}
+	if (rt->backbuffer != 0) {
 		GLES3::Utilities::get_singleton()->texture_free_data(rt->backbuffer);
 		rt->backbuffer = 0;
-		rt->backbuffer_fbo = 0;
 	}
 	if (rt->backbuffer_depth != 0) {
 		GLES3::Utilities::get_singleton()->texture_free_data(rt->backbuffer_depth);

platform/linuxbsd/x11/display_server_x11.cpp

@@ -4268,6 +4268,8 @@ bool DisplayServerX11::_window_focus_check() {
 }
 
 void DisplayServerX11::process_events() {
+	ERR_FAIL_COND(!Thread::is_main_thread());
+
 	_THREAD_SAFE_LOCK_
 
 #ifdef DISPLAY_SERVER_X11_DEBUG_LOGS_ENABLED

platform/macos/display_server_macos.mm

@@ -2985,7 +2985,7 @@
 }
 
 void DisplayServerMacOS::process_events() {
-	_THREAD_SAFE_LOCK_
+	ERR_FAIL_COND(!Thread::is_main_thread());
 
 	while (true) {
 		NSEvent *event = [NSApp
@@ -3018,11 +3018,11 @@
 
 	if (!drop_events) {
 		_process_key_events();
-		_THREAD_SAFE_UNLOCK_
 		Input::get_singleton()->flush_buffered_events();
-		_THREAD_SAFE_LOCK_
 	}
 
+	_THREAD_SAFE_LOCK_
+
 	for (KeyValue<WindowID, WindowData> &E : windows) {
 		WindowData &wd = E.value;
 		if (wd.mpass) {
@@ -3051,7 +3051,7 @@
 }
 
 void DisplayServerMacOS::force_process_and_drop_events() {
-	_THREAD_SAFE_METHOD_
+	ERR_FAIL_COND(!Thread::is_main_thread());
 
 	drop_events = true;
 	process_events();

platform/windows/display_server_windows.cpp

@@ -2962,30 +2962,28 @@ String DisplayServerWindows::keyboard_get_layout_name(int p_index) const {
 }
 
 void DisplayServerWindows::process_events() {
-	_THREAD_SAFE_LOCK_
-
-	MSG msg;
+	ERR_FAIL_COND(!Thread::is_main_thread());
 
 	if (!drop_events) {
 		joypad->process_joypads();
 	}
 
+	_THREAD_SAFE_LOCK_
+	MSG msg = {};
 	while (PeekMessageW(&msg, nullptr, 0, 0, PM_REMOVE)) {
 		TranslateMessage(&msg);
 		DispatchMessageW(&msg);
 	}
+	_THREAD_SAFE_UNLOCK_
 
 	if (!drop_events) {
 		_process_key_events();
-		_THREAD_SAFE_UNLOCK_
 		Input::get_singleton()->flush_buffered_events();
-	} else {
-		_THREAD_SAFE_UNLOCK_
 	}
 }
 
 void DisplayServerWindows::force_process_and_drop_events() {
-	_THREAD_SAFE_METHOD_
+	ERR_FAIL_COND(!Thread::is_main_thread());
 
 	drop_events = true;
 	process_events();
@@ -4664,10 +4662,12 @@ LRESULT DisplayServerWindows::WndProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARA
 		} break;
 		case WM_TIMER: {
 			if (wParam == windows[window_id].move_timer_id) {
+				_THREAD_SAFE_UNLOCK_
 				_process_key_events();
 				if (!Main::is_iterating()) {
 					Main::iteration();
 				}
+				_THREAD_SAFE_LOCK_
 			} else if (wParam == windows[window_id].activate_timer_id) {
 				_process_activate_event(window_id);
 				KillTimer(windows[window_id].hWnd, windows[window_id].activate_timer_id);

servers/physics_server_2d_wrap_mt.cpp

@@ -32,45 +32,37 @@
 
 #include "core/os/os.h"
 
-void PhysicsServer2DWrapMT::thread_exit() {
-	exit = true;
+void PhysicsServer2DWrapMT::_assign_mt_ids(WorkerThreadPool::TaskID p_pump_task_id) {
+	server_thread = Thread::get_caller_id();
+	server_task_id = p_pump_task_id;
 }
 
-void PhysicsServer2DWrapMT::thread_step(real_t p_delta) {
-	physics_server_2d->step(p_delta);
-	step_sem.post();
+void PhysicsServer2DWrapMT::_thread_exit() {
+	exit = true;
 }
 
-void PhysicsServer2DWrapMT::thread_loop() {
-	server_thread = Thread::get_caller_id();
-
-	physics_server_2d->init();
-
-	command_queue.set_pump_task_id(server_task_id);
+void PhysicsServer2DWrapMT::_thread_loop() {
 	while (!exit) {
 		WorkerThreadPool::get_singleton()->yield();
 		command_queue.flush_all();
 	}
-
-	command_queue.flush_all();
-
-	physics_server_2d->finish();
 }
 
 /* EVENT QUEUING */
 
 void PhysicsServer2DWrapMT::step(real_t p_step) {
 	if (create_thread) {
-		command_queue.push(this, &PhysicsServer2DWrapMT::thread_step, p_step);
+		command_queue.push(physics_server_2d, &PhysicsServer2D::step, p_step);
 	} else {
-		command_queue.flush_all(); // Flush all pending from other threads.
 		physics_server_2d->step(p_step);
 	}
 }
 
 void PhysicsServer2DWrapMT::sync() {
 	if (create_thread) {
-		step_sem.wait();
+		command_queue.sync();
+	} else {
+		command_queue.flush_all(); // Flush all pending from other threads.
 	}
 	physics_server_2d->sync();
 }
@@ -85,21 +77,26 @@ void PhysicsServer2DWrapMT::end_sync() {
 
 void PhysicsServer2DWrapMT::init() {
 	if (create_thread) {
-		exit = false;
-		server_task_id = WorkerThreadPool::get_singleton()->add_task(callable_mp(this, &PhysicsServer2DWrapMT::thread_loop), true);
-		step_sem.post();
+		WorkerThreadPool::TaskID tid = WorkerThreadPool::get_singleton()->add_task(callable_mp(this, &PhysicsServer2DWrapMT::_thread_loop), true);
+		command_queue.set_pump_task_id(tid);
+		command_queue.push(this, &PhysicsServer2DWrapMT::_assign_mt_ids, tid);
+		command_queue.push_and_sync(physics_server_2d, &PhysicsServer2D::init);
+		DEV_ASSERT(server_task_id == tid);
 	} else {
+		server_thread = Thread::MAIN_ID;
 		physics_server_2d->init();
 	}
 }
 
 void PhysicsServer2DWrapMT::finish() {
 	if (create_thread) {
-		command_queue.push(this, &PhysicsServer2DWrapMT::thread_exit);
+		command_queue.push(physics_server_2d, &PhysicsServer2D::finish);
+		command_queue.push(this, &PhysicsServer2DWrapMT::_thread_exit);
 		if (server_task_id != WorkerThreadPool::INVALID_TASK_ID) {
 			WorkerThreadPool::get_singleton()->wait_for_task_completion(server_task_id);
 			server_task_id = WorkerThreadPool::INVALID_TASK_ID;
 		}
+		server_thread = Thread::MAIN_ID;
 	} else {
 		physics_server_2d->finish();
 	}
@@ -108,9 +105,6 @@ void PhysicsServer2DWrapMT::finish() {
 PhysicsServer2DWrapMT::PhysicsServer2DWrapMT(PhysicsServer2D *p_contained, bool p_create_thread) {
 	physics_server_2d = p_contained;
 	create_thread = p_create_thread;
-	if (!create_thread) {
-		server_thread = Thread::MAIN_ID;
-	}
 }
 
 PhysicsServer2DWrapMT::~PhysicsServer2DWrapMT() {

servers/physics_server_2d_wrap_mt.h

@@ -53,17 +53,14 @@ class PhysicsServer2DWrapMT : public PhysicsServer2D {
 
 	mutable CommandQueueMT command_queue;
 
-	void thread_loop();
-
 	Thread::ID server_thread = Thread::UNASSIGNED_ID;
 	WorkerThreadPool::TaskID server_task_id = WorkerThreadPool::INVALID_TASK_ID;
 	bool exit = false;
-	Semaphore step_sem;
 	bool create_thread = false;
 
-	void thread_step(real_t p_delta);
-
-	void thread_exit();
+	void _assign_mt_ids(WorkerThreadPool::TaskID p_pump_task_id);
+	void _thread_exit();
+	void _thread_loop();
 
 public:
 #define ServerName PhysicsServer2D