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

Modernize Thread #45315

Merged
merged 1 commit into from
Jan 31, 2021
Merged

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Jan 19, 2021

Continuing with the effort started with Mutex and Thread, and following also what is possible now with Semaphore, RWLock is getting the same kind of changes.

  • Based on C++11's thread and thread_local
  • No more need to allocate-deallocate or check for null (see note 2)
  • No pointer anymore, just a member variable (see note 2)
  • Platform-specific implementations no longer needed (except for the few cases of non-portable functions)
  • Simpler for NO_THREADS
  • Thread ids are now the same across platforms (main is 1; others follow)

NOTE: This is done on top of #45314, intended to be reviewed/merged after it. Anyway, the commit for Thread can be checked separately. UPDATE: Rebased after the other PR having been merged, so this is standalone now.

NOTE 2: There are still cases where you need to use memnew(Thread) and use it through a pointer. Namely, having a container of Threads, creating a Thread conditionally (unless the condition is about a non-threaded build) and using Thread in a class that needs to be copied. Without adding move semantics, we can't do better.


This code is generously donated by IMVU.

@RandomShaper RandomShaper added this to the 4.0 milestone Jan 19, 2021
@RandomShaper RandomShaper requested review from Faless, reduz and a team January 19, 2021 16:38
@RandomShaper RandomShaper changed the title Modernize Thread Modernize Thread Jan 19, 2021
@RandomShaper RandomShaper force-pushed the modernize_thread branch 11 times, most recently from 8c7f49a to 9b10382 Compare January 19, 2021 17:38
@RandomShaper RandomShaper marked this pull request as ready for review January 19, 2021 17:40
@RandomShaper RandomShaper force-pushed the modernize_thread branch 2 times, most recently from 3ce1a07 to d269cce Compare January 19, 2021 20:11
Thread::~Thread() {
if (id != 0) {
#ifdef DEBUG_ENABLED
WARN_PRINT("A Thread object has been destroyed without wait_to_finish() having been called on it. Please do so to ensure correct cleanup of the thread.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still really needed? Documentation states resources are freed once the thread exits: https://en.cppreference.com/w/cpp/thread/thread/detach

Copy link
Member Author

@RandomShaper RandomShaper Jan 19, 2021

Choose a reason for hiding this comment

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

My concern is that the code running on the thread, if left to run wildly, may access something that is already released on or in a wrong state.
Maybe in practice that's unlikely, but I thought it was a good idea to just warn against fire-and-forget, so joining the thread is the de facto standard practice, which can help catching bugs.

@Faless
Copy link
Collaborator

Faless commented Jan 25, 2021

I'm getting this runtime error on the Javascript build (NO_THREADS):

exception thrown: RuntimeError: abort(undefined). Build with -s ASSERTIONS=1 for more info.,abort@http://127.0.0.1:8060/tmp_js_export.js:9:9257
_abort@http://127.0.0.1:8060/tmp_js_export.js:9:97189
std::__2::__throw_overflow_error(char const*)@http://127.0.0.1:8060/tmp_js_export.js line 175 > WebAssembly.instantiate:wasm-function[1529]:0x5d294
std::__2::thread::thread<void (&)(ThreadWorkPool::ThreadData*), ThreadWorkPool::ThreadData*, void>(void (&)(ThreadWorkPool::ThreadData*), ThreadWorkPool::ThreadData*&&)@http://127.0.0.1:8060/tmp_js_export.js line 175 > WebAssembly.instantiate:wasm-function[35008]:0x73c3a5
ThreadWorkPool::init(int)@http://127.0.0.1:8060/tmp_js_export.js line 175 > WebAssembly.instantiate:wasm-function[48280]:0xa1b819
RenderingServer::RenderingServer()@http://127.0.0.1:8060/tmp_js_export.js line 175 > WebAssembly.instantiate:wasm-function[19601]:0x337d30
RenderingServerDefault::RenderingServerDefault()@http://127.0.0.1:8060/tmp_js_export.js line 175 > WebAssembly.instantiate:wasm-function[52386]:0xafa13b
Main::setup2(unsigned long long)@http://127.0.0.1:8060/tmp_js_export.js line 175 > WebAssembly.instantiate:wasm-function[74107]:0xd4b2e8
Main::setup(char const*, int, char**, bool)@http://127.0.0.1:8060/tmp_js_export.js line 175 > WebAssembly.instantiate:wasm-function[74106]:0xd4aba7
godot_js_main(int, char**)@http://127.0.0.1:8060/tmp_js_export.js line 175 > WebAssembly.instantiate:wasm-function[15972]:0x27994c
main@http://127.0.0.1:8060/tmp_js_export.js line 175 > WebAssembly.instantiate:wasm-function[36726]:0x7b9931
Godot/Module._main@http://127.0.0.1:8060/tmp_js_export.js:9:231735
callMain@http://127.0.0.1:8060/tmp_js_export.js:9:236878
Engine.prototype.start/</<@http://127.0.0.1:8060/tmp_js_export.js:362:14
Engine.prototype.start/<@http://127.0.0.1:8060/tmp_js_export.js:357:11
promise callback*Engine.prototype.start@http://127.0.0.1:8060/tmp_js_export.js:302:20
Engine.prototype.startGame/<@http://127.0.0.1:8060/tmp_js_export.js:381:20
promise callback*Engine.prototype.startGame@http://127.0.0.1:8060/tmp_js_export.js:376:6
@http://127.0.0.1:8060/tmp_js_export.html:268:12
@http://127.0.0.1:8060/tmp_js_export.html:273:5

@RandomShaper
Copy link
Member Author

RandomShaper commented Jan 26, 2021

The problem was is that the RenderingServer has started to use ThreadWorkPool recently. Remarks:

  • ThreadWorkPool was using std::thread directly, regardless NO_THREADS. I've modified it to use the new Godotish Thread (added in this PR), which is little more than a wrapper around std::thread, but graceful with NO_THREADS.
  • ThreadWorkPool is calling OS::get_singleton()->get_processor_count() to get the number of threads to spawn. It's returning -1 on the web version. That value is being casted to unsigned; you can imagine... Besides making that method return a sensible number (maybe by using std::thread::hardware_concurrency instead; haven't tried myself), ThreadWorkPool and/or RenderingServer will need changes to be nicer with NO_THREADS. In any case, I believe those fixes are out of the scope of this PR (the problem was already there).

@Faless, does this sound good to move forward with this PR? CC @reduz

@Faless
Copy link
Collaborator

Faless commented Jan 26, 2021

problem was is that the RenderingServer has started to use ThreadWorkPool recently.

Oh, that's weird, it was only used in the RenderingDevice bit at first. Is this by design or a side effect of a refactoring @reduz ?

* `ThreadWorkPool` is calling `OS::get_singleton()->get_processor_count()` to get the number of threads to spawn. It's returning `-1` on the web version

Oh, that explains why I was getting a deadlock on the threaded version. I'll make it return the value when known, 0 when threads are not available or the value is unknown.

@RandomShaper
Copy link
Member Author

He did the change in 77bc3e9, to add some threaded culling power to the renderer.

@Faless, if you think this PR is good by itself, considering the worker threads issue has to be addressed separately, kindly approve this PR so it can be merged (sorry for insisting, but more PRs are being worked on top of this and I want to reduce the risk of rebase hell to a minimum).

@RandomShaper RandomShaper force-pushed the modernize_thread branch 2 times, most recently from 671f7e8 to 7f76c18 Compare January 28, 2021 09:04
- Based on C++11's `thread` and `thread_local`
- No more need to allocate-deallocate or check for null
- No pointer anymore, just a member variable
- Platform-specific implementations no longer needed (except for the few cases of non-portable functions)
- Simpler for `NO_THREADS`
- Thread ids are now the same across platforms (main is 1; others follow)
@akien-mga akien-mga merged commit 5525cd8 into godotengine:master Jan 31, 2021
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the modernize_thread branch January 31, 2021 19:02
akien-mga added a commit to akien-mga/godot that referenced this pull request Mar 14, 2021
akien-mga added a commit that referenced this pull request Mar 16, 2021
Was a regression from #45315.

Fixes #46998.

(cherry picked from commit 7a64819)
lekoder pushed a commit to KoderaSoftwareUnlimited/godot that referenced this pull request Apr 24, 2021
akien-mga added a commit to akien-mga/godot that referenced this pull request May 11, 2021
akien-mga added a commit that referenced this pull request May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants