Page MenuHomeWildfire Games

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

Authored by wraitii on May 26 2019, 6:08 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22772: pthread -> std::thread (5/7) - Replace sdl semaphore with condition variable
Summary

This replaces usage of SDL semaphores with condition variables.

Test Plan

Compile, test.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.May 26 2019, 6:08 PM
Owners added a subscriber: Restricted Owners Package.May 26 2019, 6:08 PM
wraitii edited the summary of this revision. (Show Details)May 26 2019, 6:09 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/1514/display/redirect

wraitii updated this revision to Diff 9338.Aug 15 2019, 12:01 PM
wraitii retitled this revision from pthread -> std::thread (5/?) - Replace sdl semaphore with condition variable to pthread -> std::thread (5/7) - Replace sdl semaphore with condition variable.
wraitii edited the summary of this revision. (Show Details)

Working version, worked around the fact thats SDL semaphores keep a count of how many times they were asked to wake up.

Removes SDL_Thread from the engine precompiled header since it's no longer necessary.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/383/display/redirect

Stan added a subscriber: Stan.Aug 15 2019, 1:01 PM
Stan added inline comments.
source/graphics/TextureManager.cpp
357 ↗(On Diff #9338)

Debug

wraitii updated this revision to Diff 9350.Aug 15 2019, 7:28 PM

Fix debug. RC.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/393/display/redirect

wraitii updated this revision to Diff 9351.Aug 15 2019, 7:33 PM

Add <condition_variable> header.

Stan added inline comments.Aug 15 2019, 7:39 PM
source/graphics/TextureConverter.cpp
523 ↗(On Diff #9351)

Why the need for the scope and busy variable here ?

source/graphics/TextureManager.cpp
344 ↗(On Diff #9351)

SDL_Delay(0) == 1 millisec ?

wraitii updated this revision to Diff 9354.Aug 16 2019, 10:03 AM

Fix test issue, fix remarks.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/graphics/TextureConverter.h
|  71| »   »   FMT_DXT1,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCTextureManager{' is invalid C code. Use --std or --language to configure the language.

source/graphics/TextureManager.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/397/display/redirect

Stan added inline comments.Aug 16 2019, 11:18 AM
source/graphics/TextureManager.cpp
344 ↗(On Diff #9354)

That comment is still accurate isn't it ?

source/ps/UserReport.cpp
271 ↗(On Diff #9354)

Why not use a scope here like you did everywhere else ?

478 ↗(On Diff #9354)

Don't care much but maybe it should be Cv for CamelCase

Angen added a subscriber: Angen.Aug 16 2019, 12:03 PM

builds without-pch

source/graphics/TextureManager.cpp
344 ↗(On Diff #9351)

SDL_Delay(x) = wait for x + some time

It waits at least the specified time, but possibly longer due to OS scheduling
(https://wiki.libsdl.org/SDL_Delay)

wraitii added inline comments.Aug 16 2019, 12:08 PM
source/graphics/TextureConverter.cpp
523 ↗(On Diff #9351)

probably un-necessary, I ran into an issue but it wasn't related here.

source/graphics/TextureManager.cpp
344 ↗(On Diff #9354)

technically a spin-loop wouldn't sleep at all... So maybe?

source/ps/UserReport.cpp
271 ↗(On Diff #9354)

Can't because of how condition variables work.

This revision was not accepted when it landed; it landed in state Needs Review.Sat, Aug 24, 1:27 PM
This revision was automatically updated to reflect the committed changes.