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

GSdx: Improve GSJobQueue behaviour #1741

Merged
merged 4 commits into from Jan 7, 2017

Conversation

Projects
None yet
3 participants
@turtleli
Member

turtleli commented Jan 4, 2017

Changes:

  • Don't use a separate counter to count queue items. Seems to fix #998.
  • Use a separate mutex for waiting - should reduce context switches.
  • Change the exit flag within a lock and make it non-atomic.

turtleli added some commits Jan 1, 2017

gsdx: Use separate mutex for waiting
In the previous code, the worker thread would notify the MTGS thread
while the mutex is still locked, which could cause the MTGS thread to
wake up and immediately go back to sleep again since it can't lock the
mutex.

Use a separate mutex for waiting, which avoids the issue.
gsdx: Don't use separate count variable
It's only ever updated after the queue is updated, so its state will
always lag slightly behind it. It's sufficient to just use empty().

This seems to fix some caching issues that were noticeable on Skylake
CPUs (#998).
gsdx: Don't use atomic for exit variable
All accesses are protected by locks, so there's no need for it to be
atomic.
@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jan 5, 2017

To be honest, I'm not 100% sure that it is 100% atomic/thread safe (disclaimer I'm far from an atomic expert). Some reading here http://preshing.com/20141024/my-multicore-talk-at-cppcon-2014/

empty() is potentially inaccurate because of the relaxed atomic. I think you have only the guarantee to have the value written by the current thread. And I don't know if lock/unlock mutex pattern protect enough.

It would require some testing to ensure there isn't any rare bug.

@turtleli

This comment has been minimized.

Member

turtleli commented Jan 6, 2017

empty() is potentially inaccurate because of the relaxed atomic.

I've taken that into account:

  • the worker thread will not report not empty when it's not - the read index is updated on this thread.
  • the worker thread may report empty when it's not - the m_lock mutex locking deals with that.
  • the main thread will not report empty when it's not - the write index is updated on this thread.
  • the main thread may report not empty when it's not - the m_wait_lock mutex locking deals with that.

So it should be thread-safe (or so I think - can never be too sure).

EDIT: Even if it's not relaxed, the queue might be updated after empty() returns a value, so the same logic will still apply.

@wareya

This comment has been minimized.

wareya commented Jan 6, 2017

Works for me, and general performance the same as your previous test builds.

@gregory38 gregory38 merged commit c2e21fa into PCSX2:master Jan 7, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jan 7, 2017

As a side note, potentially relaxing the fetch_add/sub atomic could have been enough. It might worth to do a review of fetch in MTGS/MTVU code path.

@turtleli turtleli deleted the turtleli:gsdx-threading branch Jan 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment