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

Codechange: Use C++11 functions for threading #7379

Merged
merged 6 commits into from Apr 6, 2019

Conversation

@michicc
Copy link
Member

michicc commented Mar 17, 2019

The C++11 STL has functions for threading and synchronization, which means we can retire our homegrown platform-dependent code.

This PR assume a conforming C++11 compiler with conforming header files. DJGPP in it's current incarnation ships thread and mutex headers, but does not define the required types (not even as empty dummies). As such, this PR kills DOS unless a lot of #ifdef's are added.

We do not assume that starting a thread will succeeded, but do assume that if threads work, mutexes do work as well.

@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Mar 17, 2019

I would suggest we remove DOS support for now, giving this PR priority. DOS has more issues (very slow), lacks networking, and more of that. Removing it has more benefits, but this shows how much we benefit from removing targets that are rarely used, and don't really support modern solutions. Just my 2 cents!

@michicc michicc force-pushed the michicc:pr/c++11-thread branch 4 times, most recently from 4daf03a to f7177bc Mar 17, 2019
src/os/unix/unix.cpp Outdated Show resolved Hide resolved
@michicc michicc force-pushed the michicc:pr/c++11-thread branch 2 times, most recently from 9341231 to 21e6d4e Mar 17, 2019
Copy link
Member

LordAro left a comment

Should resolve #6857 ?

src/genworld.h Outdated Show resolved Hide resolved
src/os/unix/unix.cpp Outdated Show resolved Hide resolved
src/os/windows/win32.cpp Outdated Show resolved Hide resolved
src/os/windows/win32.cpp Outdated Show resolved Hide resolved
src/thread.h Show resolved Hide resolved
src/thread.h Show resolved Hide resolved
* Check if we can use a thread for modal progress.
* @return Threading usable?
*/
static inline bool UseThreadedModelProgress()

This comment has been minimized.

Copy link
@LordAro

LordAro Mar 17, 2019

Member

I wonder how useful this is, given not everywhere else checks for usable threads? (I only see this used for newgrf scan & worldgen) Should probably be all or nothing, and abort if the threads fail

This comment has been minimized.

Copy link
@michicc

michicc Mar 17, 2019

Author Member

Constructing a mutex must succeed, locking a mutex may fail. The mutexes for model progress are locked right in main, if that fails it's instant exit. For the real existing C++11 implementations, mutex lock fail will only occur if threading at all is unusable.

The other threading sites try to create their threads first and fall back to a non-threaded implementation. We never got any bug reports that creating a thread succeeded whilst the subsequent mutex lock failed, and I don't see any reason that would change with this PR.

@michicc michicc force-pushed the michicc:pr/c++11-thread branch 2 times, most recently from 0301d8b to f963e42 Mar 17, 2019
@michicc

This comment has been minimized.

Copy link
Member Author

michicc commented Mar 17, 2019

I don't see any reason why this PR should touch #6857. Internally, it all resolves down to pthreads anyway as before.

@orudge

This comment has been minimized.

Copy link
Contributor

orudge commented Mar 18, 2019

I would suggest we remove DOS support for now, giving this PR priority. DOS has more issues (very slow), lacks networking, and more of that. Removing it has more benefits, but this shows how much we benefit from removing targets that are rarely used, and don't really support modern solutions. Just my 2 cents!

I did once get networking working on DOS via WATTCP, but that was a long time ago, and I never bothered trying to get that into OpenTTD itself. (In fact, I think it may have predated Rubidium's official DOS port.) No objections to dropping it, and you never know, perhaps one day somebody will 'port' std::thread to DJGPP!

@michicc michicc requested a review from TrueBrain Mar 20, 2019
Copy link
Member

TrueBrain left a comment

Not really sure why you asked me for a review. I know nothing about C++11.

Minor update: we indeed dropped DOS support, so yeah .. that is solved ;)

@michicc michicc force-pushed the michicc:pr/c++11-thread branch from f963e42 to cb384ef Mar 24, 2019
@michicc michicc requested a review from LordAro Mar 24, 2019
@michicc michicc force-pushed the michicc:pr/c++11-thread branch from cb384ef to bb3c386 Mar 26, 2019
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
@michicc michicc force-pushed the michicc:pr/c++11-thread branch from bb3c386 to 6d812b3 Apr 1, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Apr 4, 2019

This looked fine when I read through it a few days ago, are any more changes incoming or is it ready for review?

@michicc

This comment has been minimized.

Copy link
Member Author

michicc commented Apr 4, 2019

There were (and are) no changes except the comment aligning and rebasing.

src/music/extmidi.cpp Outdated Show resolved Hide resolved
michicc added 6 commits Mar 10, 2019
A conforming compiler with a valid <mutex>-header is expected.
Most parts of the code assume that locking a mutex will never fail unexpectedly,
which is generally true on all common platforms that don't just pretend to
be C++11. The use of condition variables in driver code is checked.
We assume a conforming C++11 compiler environment that has a valid <thread>-header.
Failure to run a real thread is handled gracefully.
@michicc michicc force-pushed the michicc:pr/c++11-thread branch from 6d812b3 to 43574fe Apr 4, 2019
@PeterN
PeterN approved these changes Apr 6, 2019
@michicc michicc merged commit 967b27a into OpenTTD:master Apr 6, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190404.2 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@michicc michicc deleted the michicc:pr/c++11-thread branch Apr 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.