HomeWildfire Games

pthread -> std::thread (5/7) - Replace sdl semaphore with condition variable

Description

pthread -> std::thread (5/7) - Replace sdl semaphore with condition variable

This removes a dependency on the SDL_Thread header.

Differential Revision: https://code.wildfiregames.com/D1921

Event Timeline

Imarok raised a concern with this commit.Oct 14 2019, 10:55 PM
Imarok added a subscriber: Imarok.

With this revision the CPU usage of 0ad on my machine is always 12 percentage points higher than before this revision.
That equals exactly one virtual core, so one thread. Guess you are badly polling somewhere.

This commit now has outstanding concerns.Oct 14 2019, 10:55 PM

Reverting the UserReporter back to rP22771 proves it is the UserReporter

Now has a ticket at #5620

Imarok added inline comments.Oct 14 2019, 11:29 PM
/ps/trunk/source/ps/UserReport.cpp
272

Not sure why you introduced the predicate, but if I remove it the CPU load is gone.
Like that:

m_WorkerCV.wait(lock);

Seems also a bit unclean to access these variables directly here and through getters a few lines later.
I know, there they need to lock the mutex, so maybe you should have just removed the getters.

Imarok added inline comments.Oct 14 2019, 11:38 PM
/ps/trunk/source/ps/UserReport.cpp
272

Yep the predicate has to go.
According to [http://www.cplusplus.com/reference/condition_variable/condition_variable/wait/ http://www.cplusplus.com/reference/condition_variable/condition_variable/wait/]
"If pred is specified (2), the function only blocks if pred returns false"
So if they are both false the wait doesn't wait.

Imarok added inline comments.Oct 14 2019, 11:51 PM
/ps/trunk/source/ps/UserReport.cpp
272

I meant: If the User Reporter is enabled the wait doesn't wait.

Stan added a subscriber: Stan.Oct 15 2019, 8:25 AM

Can confirm it also affects windows + 10% CPU

Imarok resigned from this commit.Dec 18 2019, 6:24 PM

Fixed by rP23259.

This commit no longer requires audit.Dec 18 2019, 6:24 PM
phosit added a subscriber: phosit.Sep 5 2022, 1:25 PM
phosit added inline comments.
/ps/trunk/source/graphics/TextureConverter.cpp
306

The mutex is locked immediatly after it is unlocked. For the working thread there is almost no time to lock the mutex and react to the signal. If i run pyrogenesis in valgrind condition_variable::notify_all is called 100000000 times.
This could be solved by adding std::this_thread::yield() just bevore this line.

546

Line 560 unconditionaly takes the first element of the queue. If the condition_variable wakes up spuriously the queue could be empty