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

Use CoreTiming for MemoryWatcher. #3791

Merged
merged 1 commit into from May 3, 2016
Merged

Conversation

spxtr
Copy link
Member

@spxtr spxtr commented Apr 22, 2016

It might be worth moving some code from Core.cpp into MemoryWatcher.cpp. Let me know.


This change is Reviewable

@JosJuice
Copy link
Member

Is there any particular reason for this change? Determinism? Simplicity?


Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.


Source/Core/Core/Core.cpp, line 118 [r1] (raw file):
Please use longer names. fps is fine to use as an acronym (though it's inaccurate in this case – MemoryWatcher callbacks are not frames), but mw should be memory_watcher, and I don't even know what et is supposed to be.


Source/Core/Core/Core.cpp, line 123 [r1] (raw file):
You might want to subtract cyclesLate from the next callback's delay.


Comments from Reviewable

static void MWCallback(u64 userdata, s64 cyclesLate)
{
s_memory_watcher->Step();
CoreTiming::ScheduleEvent(SystemTimers::GetTicksPerSecond() / s_mw_fps, et_MW);

This comment was marked as off-topic.

@degasus
Copy link
Member

degasus commented Apr 22, 2016

In my opinion, there is no need to ask why something should be deterministic. As long as there is no good reason not to be so, everything should be determinnistic ;)

So I like this change. A dummy thread is dropped, a race condition is dropped (you had accessed the GC memory in a unsafe way). But yeah, moving more code into MemoryWatcher.cpp would be nice.

@JosJuice
Copy link
Member

Indeed, it's better to have this deterministic. I was just asking because it wasn't clear at first that determinism was the reason for this change.

@spxtr
Copy link
Member Author

spxtr commented Apr 23, 2016

Determinism is nice, but the real reason is that I want to run it at roughly 5x speed, and polling at a fixed time interval wasn't good enough for that.

Thanks for the comments, PTAL.


Review status: 0 of 3 files reviewed at latest revision, 6 unresolved discussions.


Source/Core/Core/Core.cpp, line 118 [r1] (raw file):
Ok, picked some better names. I took et from SystemTimers.cpp.


Source/Core/Core/Core.cpp, line 123 [r1] (raw file):
Done.


Source/Core/Core/Core.cpp, line 123 [r1] (raw file):
Done.


Source/Core/Core/Core.cpp, line 299 [r1] (raw file):
Done.


Source/Core/Core/Core.cpp, line 372 [r1] (raw file):
Done.


Source/Core/Core/MemoryWatcher.cpp, line 85 [r1] (raw file):
I still want it because if there is no Locations.txt or if the socket doesn't open then I want to skip this. It's certainly possible to get that behavior by only scheduling the event if loading is successful, but this was easier.


Comments from Reviewable

@JosJuice
Copy link
Member

Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions.


Source/Core/Core/MemoryWatcher.cpp, line 25 [r2] (raw file):
Shouldn't this be SystemTimers::GetTicksPerSecond() / MW_RATE - cyclesLate?


Comments from Reviewable

@spxtr
Copy link
Member Author

spxtr commented Apr 25, 2016

Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions.


Source/Core/Core/MemoryWatcher.cpp, line 25 [r2] (raw file):
Done.


Comments from Reviewable

@degasus
Copy link
Member

degasus commented Apr 25, 2016

Reviewed 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@JosJuice
Copy link
Member

Please squash the commits.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@spxtr
Copy link
Member Author

spxtr commented Apr 29, 2016

Done.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@degasus
Copy link
Member

degasus commented Apr 29, 2016

LGTM. IMO fine for 5.0.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@Parlane Parlane merged commit e0a5f1b into dolphin-emu:master May 3, 2016
@vladfi1
Copy link
Contributor

vladfi1 commented Feb 18, 2017

So it happens that I came up with a different solution to this almost a year ago at https://github.com/vladfi1/dolphin - instead of having a separate thread periodically checking, I had a hook from VideoInterface::EndField that would poll once each frame. Doing this on SSBM gave me no missed frames under a lot of different conditions. Would that be preferable to the current approach?

I also turned made the memory watcher use zmq (for synchronization) - if we did the same with the named pipes I think that would be enough for windows compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants