Skip to content
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

Jump to bottom

Cleanup usage of atomic/threadsafe functions #1987

Merged
merged 2 commits into from Feb 10, 2015
Merged

Cleanup usage of atomic/threadsafe functions #1987

merged 2 commits into from Feb 10, 2015

Conversation

magumagu
Copy link
Contributor

We shouldn't use atomic/threadsafe functions when they aren't necessary; it's misleading at best.

@Sonicadvance1
Copy link
Contributor

Why was PowerPCState::Exceptions using atomics anyway?

@magumagu
Copy link
Contributor Author

Looks like 8d6f984 changed PowerPCState::Exceptions... some sort of misguided attempt to fix dual-core. These days, we use ScheduleEvent_Threadsafe when the GPU thread needs to asynchronously cause an interrupt.

@magumagu
Copy link
Contributor Author

Thanks for spotting that; fixed.

@Mullin
Copy link
Contributor

Mullin commented Jan 31, 2015

You're welcome

@magumagu
Clean up usage of PowerPCState::Exceptions.
30d15b3 
Accessing any member of ppcState from a thread other than the CPU thread
is not allowed; don't pretend that there's any exception to that rule.
@magumagu
Copy link
Contributor Author

Ping.

@lioncash
Copy link
Member

Looks good to me.

magumagu added a commit that referenced this pull request Feb 10, 2015
@magumagu
Merge pull request #1987 from magumagu/thread-safety
d9988ee 
Cleanup usage of atomic/threadsafe functions
@magumagu magumagu merged commit d9988ee into dolphin-emu:master Feb 10, 2015
@Linktothepast
Copy link
Contributor

I am getting this issue every time i close a game in dolphin after this merge:
http://prntscr.com/644wsy
Should i write an issue report?

@magumagu
Copy link
Contributor Author

Yes, please; include your full configuration, and assign it to me.

@psennermann
Copy link

On my system this merge has caused a 10% speed slowdown

@magumagu
Copy link
Contributor Author

Does PR #2038 help the regression any?

@psennermann
Copy link

Since it's not merged and there's no build to try, at the moment I am not able to tell...

@psennermann
Copy link

Unfortunately it doesn't help...

@magumagu
Copy link
Contributor Author

Okay, then I'm not sure what's going on. Please file an issue report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
6 participants
@magumagu @Sonicadvance1 @Mullin @lioncash @Linktothepast @psennermann