Fixes high cpu usage spikes on win10 #656
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
p12tic
approved these changes
May 6, 2020
|
Thanks! cc. @AdrianKoshka |
|
cc. @walker0643 Do you remember if the issue #4 that was fixed there appears on windows <=7 or >=8? In this PR I issumed that change dbfb04a was made to fix behaviour on windows7, like qt bug linked above, but I'm not sure. |
|
@galkinvv Could you rebase on newest master to fix build failure on Mac? |
shymega
added
bug
…nding Current code base don't use IEventQueue::isEmpty() method. Remove it to simplify IEventQueue API and to remove confusion with IEventQueueBuffer with same name. The IEventQueueBuffer::isEmpty() and all its implementations kept unchanged
galkinvv
force-pushed
the
fix-win10-cpu-usage-spikes
branch
from
fa588a0
to
91db66a
Compare
p12tic
reviewed
May 29, 2020
Fixes high cpu usage spikes on win10. When queue was containing messages of only non-QS_POSTMESSAGE type the "while (m_buffer->isEmpty())" busy-looped in EventQueue::getEvent since isEmpty was true (checked only QS_POSTMESSAGE message type), but waitForEvent returned immediately (checked more message types). Investigation shows that the difference was introduced in debauchee@dbfb04a6e to fix a problem with bad behaviour of GetQueueStatus Researching showed that a similar problem was fixed in Qt, and the solution was "pass different flags to GetQueueStatus depending on version of windows" https://bugreports.qt.io/browse/QTBUG-29097 So this patch makes changes to a barrier non-GUI core similar to Qt fix.
galkinvv
force-pushed
the
fix-win10-cpu-usage-spikes
branch
from
91db66a
to
95f2a84
Compare
|
I've tested fixed client on windows 10 for 2-3 hours - works fine. However I didn't test windows 7 and server on windows 10 at all. |
|
Given https://codereview.qt-project.org/c/qt/qtbase/+/63327/2/src/corelib/kernel/qeventdispatcher_win.cpp I think the risk of this change is fairly small. |
ghost
mentioned this pull request
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using barrierc on windows10 I noticed that sometimes but quite often, say 1 times of 5 opens of a task manager (even when server is offline) it begins utilizing 100% of a cpu core.
Fixes high cpu usage spikes on win10.
When queue was containing messages of only non-QS_POSTMESSAGE type the
"while (m_buffer->isEmpty())" busy-looped in EventQueue::getEvent
since isEmpty was true (checked only QS_POSTMESSAGE message type),
but waitForEvent returned immediately (checked more message types).
Investigation shows that the difference was introduced in
dbfb04a6e
to fix a problem with bad behaviour of GetQueueStatus
Researching showed that a similar problem was fixed in Qt,
and the solution was
"pass different flags to GetQueueStatus depending on version of windows"
https://bugreports.qt.io/browse/QTBUG-29097
So this patch makes changes to a barrier non-GUI core similar to Qt fix.
Unfortunately I'm not using barrier often now, so only basic tests were performed and only windows as a client, not as a server.
However, this PR is created since the problem may be common, so more testers may appear)