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.

Sign up for GitHub

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

Rewrite raw memcard threading code. Fixes issue 7484. #778

Merged
merged 3 commits into from Aug 18, 2014
Merged

Rewrite raw memcard threading code. Fixes issue 7484. #778

merged 3 commits into from Aug 18, 2014

Conversation

shuffle2
Copy link
Contributor

No description provided.

@shuffle2
Copy link
Contributor Author

This probably fixes other weird issues which weren't caught...it looks like when raw memcards were split from the exi device class, a lot of stuff was duplicated and not really integrated correctly (duplicated m_bDirty variables, etc).

However, I would really like people to review the code around the sync primitives in MemoryCard::FlushToFile and MemoryCard::FlushThread ... :)

delroth
delroth reviewed Aug 11, 2014
View reviewed changes
Source/Core/Core/HW/GCMemcard.h
{
return address <= (memory_card_size - 1);
}
virtual void MakeDirty() {};

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@shuffle2
Copy link
Contributor Author

I changed the flush strategy and changed the threading stuff up a bit...still not awesome but lmk.

@shuffle2
Copy link
Contributor Author

After I pushed, I realized a slightly better way (avoids situation where last changed buffer wouldn't be written to disk until closing the device): Instead of managing delay of calls from outside the FlushThread, just have FlushThread sleep itself, wake up (possibly on event), and decide for itself whether to flush or not.

delroth
delroth reviewed Aug 13, 2014
View reviewed changes
Source/Core/Common/Event.h
@@ -47,6 +48,20 @@ class Event final
m_flag.Clear();
}

template<class Rep, class Period>

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@shuffle2
Copy link
Contributor Author

rebased and fixed a warning.

@shuffle2 shuffle2 changed the title Rewrite raw memcard threading code. Intended to fix issue 7484. Rewrite raw memcard threading code. Fixes issue 7484. Aug 18, 2014
@delroth
Copy link
Member

delroth commented Aug 18, 2014

@shuffle2: This comment grants you the permission to merge this pull request whenever you think it is ready. After addressing the remaining comments, click this link to merge.

The code seems ok in terms of thread safety (though it confused me more than once, which is probably not a good thing, but making it better would require a large architectural change...).

Please address that last comment above. @dolphin-emu-bot allowmerge

@shuffle2
Add Common/Event.h to the VS project files.
375be67
@shuffle2
Add Common::Event::WaitFor(), which has the same semantics as std::co…
87c324c 
…ndition_variable::wait_for() (with millisecond units).
@shuffle2
Rewrite raw memcard threading code. Intended to fix issue 7484.
bd7f856 
EXI memcard code now doesn't know specifics of how data is flushed to whatever backing storage is used.
GC raw memcard now flushes every 15 seconds if dirty, and on memcard destruction.
GCI folder now flushes only on memcard destruction.
shuffle2 added a commit that referenced this pull request Aug 18, 2014
@shuffle2
Merge pull request #778 from shuffle2/fix-memcard-flush
a6ebdff 
Rewrite raw memcard threading code. Fixes issue 7484.
@shuffle2 shuffle2 merged commit a6ebdff into dolphin-emu:master Aug 18, 2014
@shuffle2 shuffle2 deleted the fix-memcard-flush branch August 18, 2014 05:01
@iori3000
Copy link

This fixes crash issue but not perfect.
In my Soulcalibur2 test, frame and sound laggy when memory save, then there is no crash of Dolphin 1,2 times but continuing over 3~4times make crash the Dolphin finally. I think it's litte unstable although more stable than before.

@shuffle2
Copy link
Contributor Author

Again, the lag is not my fault :/
However, please provide a crashdump of when it crashes.

On Tue, Aug 26, 2014 at 3:15 AM, iori3000 notifications@github.com wrote:

This fixes crash issue but not perfect.
In my Soulcalibur2 test, frame and sound laggy when memory save, then
there is no crash of Dolphin 1,2 times but continuing over 3~4times make
crash the Dolphin finally. I think it's litte unstable although more stable
than before.

—
Reply to this email directly or view it on GitHub
#778 (comment).

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
7 participants
@shuffle2 @delroth @iori3000 @lioncash @ds84182 @archshift @badkarma12