rP22772 introduced the bug. I guess it came from a misunderstanding of how the predicate of wait works. (see http://www.cplusplus.com/reference/condition_variable/condition_variable/wait/)
I also think using a predicate is not really useful here. So this patch just removes the predicate.
With the predicate the wait doesn't wait if m_Enabled is true making it busy waiting the whole time.
Details
Details
- Reviewers
wraitii Silier - Commits
- rP23259: Fix User Reporter worker thread always using a full CPU-Thread
- Trac Tickets
- #5620
Start the application and enable the User Reporter. Even in the main menu the CPU load of 0ad should be as high as one virtual core. With this patch it should be very low.
Diff Detail
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
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/454/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/969/display/redirect
Comment Actions
If enabled, condition
return m_Enabled || m_Shutdown;
is always true.
From specification of std::condition_variable::wait
void wait (unique_lock<mutex>& lck, Predicate pred);
If pred is specified (2), the function only blocks if pred returns false, and notifications can only unblock the thread when it becomes true
in current case mean that code will never enter wait when in state enabled.
Since code do check against m_Enabled and m_Shutdown after wake up, conditional wait is not crucial in this case. To make condition wait work there would need to be
(!m_ReportQueue.empty() && m_Enabled) || m_Shutdown
what means wake if you have something to do and you are enabled or you are going to shut down else wait
but maybe it would need to add check for
timer_Time() < m_PauseUntilTime
as well.
but I do not see it necessary as we do all that check along the way and now it should wait correctly, sure there may be more suspicious wake ups than there would be with conditional, but I think they will not be in such numbers that would be troublesome.