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

Use mingw-std-threads in MinGW builds #85039

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Nov 18, 2023

Quoting @akien-mga:

[...] I believe RandomShaper intends to look into a better fix later (though it's starting to look like we'd have to fix MinGW or GCC itself :)).

This PR replaces the implementation of std::thread and friends provided by MinGW with a third-party one (so, in a sense, it indeed fixes MinGW). This seems to fix crashes and hangs occuring in complex multi-threading scenarios.

Fixes #78734.
Fixes #85105.

Limitations:

  • Some additional files are modified to avoid compile errors due to some additional Windows defines leaking around. It's not as simple as undefining them in typedefs.h. If this PR is accepted to at least workaround the serious issue, further work could be done on that area to keep things a bit less ugly, but this is as far as I've been able to reach so far.
  • 3rd-party code still uses the std::* symbols provided by MinGW. That doesn't seem to be a big issue. However, a nice piece of future work would be to make everything use the same, given the replacements provided in this PR seem to work better. And to avoid duplicates.
  • With/without this PR I'm getting certain non-fatal errors at exit (of the kind of ERROR: BUG: Unreferenced static string to 0: completed). That means this PR doesn't completely fix all the threading issues caused by MinGW. UPDATE: After replacing the recursive mutex (lack thereof was a left-behing), I'm no longer getting those errors! I'm starting to think this may be fixing other Windows-only stability issues in prod builds.

EXTRA (2023-11-21):
This may allow us to revert the following PRs without regressions:

@RandomShaper
Copy link
Member Author

RandomShaper commented Nov 18, 2023

In draft because I've realized a few mistakes. The overall approach is already reviewable, though. UPDATE: Fully ready now, after fixing for the non-MinGW builds.

@RandomShaper RandomShaper force-pushed the mingwthreads branch 4 times, most recently from a6a4d65 to 1a43eff Compare November 18, 2023 10:52
@RandomShaper RandomShaper marked this pull request as ready for review November 18, 2023 11:05
@Chubercik
Copy link
Contributor

Awesome stuff :)

@akien-mga
Copy link
Member

akien-mga commented Nov 21, 2023

Test build of 4.2 RC 1 + this PR:
https://downloads.tuxfamily.org/godotengine/testing/Godot_v4.2-rc1_mingw-meganz-threads_win64.exe.zip


We discussed this on RC with @RandomShaper, @hpvb and @YuriSizov, and agreed to go with this approach for 4.2.

I was wary of replacing our threads implementation for Windows so late in the release cycle, but the alternatives are either building with MSVC (which is also a risky change to do so late in the cycle, and has a number of other issues with known bugs and performance regressions), or finding another workaround for what seems to be a bug in mingw's current threads implementation.

RandomShaper tried a potential workaround in #85165 but that didn't seem to work.

This PR however properly solves the linked issues.

The implementation of std::thread for mingw by Mega NZ seems to be used by a few other projects, according to some checks by RandomShaper:

@hpvb also reviewed the library and thinks it should be a good option for us with the given constraints.

So let's test in 4.2 RC 2 and hope it doesn't regress in other use cases.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

There might be ways to improve the namespace issue, as well as preventing windows.h defines from leaking everywhere, but as a short term fix this should be good enough.

We can clean this up after the 4.2 release (and backport the cleanup to 4.2.x, as this might still cause downstream issues for users with custom modules that would also end up importing some of those new headers).

@akien-mga akien-mga merged commit c2f8fb3 into godotengine:master Nov 21, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Jimmio92
Copy link

godotengine/godot-proposals#6219 godotengine/godot-proposals#6062 just wanted to mention these proposals from nearly a year ago as it directly relates to threading and had action been taken on said proposals, they would have eliminated whatever this bandaid fix is by using OS specific threading systems directly.

@RandomShaper
Copy link
Member Author

You have written comments both here and in one of the proposals, but I'll just follow up here. Your proposals are about threading, indeed, but about a different aspect of it. Thia PR replaces an implementation of std threading stuff with another. Besides that, it's as if anything else had changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants