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 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
Copy link
Member

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 pr/c++11-thread branch 4 times, most recently from 4daf03a to f7177bc Compare March 17, 2019 13:20
src/os/unix/unix.cpp Outdated Show resolved Hide resolved
@michicc michicc force-pushed the pr/c++11-thread branch 2 times, most recently from 9341231 to 21e6d4e Compare March 17, 2019 14:14
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

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()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 pr/c++11-thread branch 2 times, most recently from 0301d8b to f963e42 Compare March 17, 2019 15:04
@michicc
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
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 March 20, 2019 21:52
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

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 ;)

src/saveload/saveload.cpp Outdated Show resolved Hide resolved
@nielsmh
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
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
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 merged commit 967b27a into OpenTTD:master Apr 6, 2019
@michicc michicc deleted the pr/c++11-thread branch April 6, 2019 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants