raw memcards: revert last change so flushes are still time-driven. #979
Conversation
|
I didn't see any Common:: replacement for this std::atomic usage. And, it seems straightforward... |
|
This looks like something that should be added to Flag (with tests!). Could you try doing that? As long as you commit to doing it, I'm fine with merging this before it's done since it's a regression. |
|
Oh, FWIW, I haven't reviewed this yet. Give me some time and reping me every few hours until I actually do it :p It's multithreaded crap, it really needs reviews. |
|
Nevermind, I replaced it with existing Common::Flag code. |
| @@ -78,10 +67,21 @@ void MemoryCard::FlushThread() | |||
| Common::SetCurrentThreadName( | |||
| StringFromFormat("Memcard%x-Flush", card_index).c_str()); | |||
|
|
|||
| const auto flush_interval = std::chrono::seconds(15); | |||
|
|
|||
| for (;;) | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
I really don't like the fact that we lock on IO in emulation code. This is basically as bad as having IO the emulation thread. Any way to get around that? |
|
@delroth now lock is around copy to temp buffer. |
| { | ||
| std::unique_lock<std::mutex> l(m_flush_mutex); | ||
| memcpy(&m_memcard_data[destaddress], srcaddress, length); | ||
| MakeDirty(); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
popped dirty setting out of the lock. |
|
LGTM, please merge as soon as @JMC47 has confirmed it works fine. |
|
I can also confirm that this change (applied to master as a git patch) successfully fixed the issue. |
|
I checked this out and didn't notice any issues with GCI-folders or raw memcards. Tested Wind Waker, Melee and Mario Kart. |
|
Actually, I need to take that back. My original comment was referencing 4b886b8 System: Linux 3.16.1, Linux Mint 17 |
|
can you check the backtrace to make sure it's related to this commit. |
|
Would the backtrace have been printed to console, or are those dumped to files? |
|
Okay I was able to reproduce it, it's a segmentation fault (all that's returned to console) that occurs when (I imagine) the save is being flushed to disk. Right before you expect the message to come up on the OSD. |
|
sigh...found it...really stupid mistake |
|
Glad I could help you find it. |
It turns out the actual slowdown was from memcpy'ing the entire memcard buffer...not synchronization overhead or the flush itself.
|
Appears 96d7b64 works correctly. Although I will note in my first test the OSD appeared very quickly. On the second test, I noticed the OSD took a few seconds to pop up. I don't entirely know the relevance. First test: Ran dolphin from commandline and selected game |
|
Ultimately though, so long as the OSD about writing to disk isn't 'lying' if you would, I'd call this fixed. That's my personal opinion though so obviously y'all do what you need to. |
raw memcards: revert last change so flushes are still time-driven.
It turns out the actual slowdown was from memcpy'ing the entire memcard buffer...not synchronization overhead or the flush itself.