Emulator Issues #4283
closedCoreTiming code cleanup patch
0%
Description
Before r7342 I had a huge FPS slowdown under linux. Patch r7342 seems to solve this issue and meanwhile I was trying to understand why, I have started to refactor the code.
I would like to submit this new version of CoreTiming.cpp that seems to me:
1°- more threadsafe, with locks as short as possible
2°- easier to understand thanks to more meaningful names of functions and a simpler design
3°- having all the events correctly ordered in a unique queue may prevent bad scheduling compared to previous design based on two queues.
The original version is based on a 2 queues design. One thread could keep on reading the events, getting a lock only once and refilling periodically the main queue with the pending events from the another queue. The drawback of this approach is that some events could be handle too late because they are waiting in the pending queue.
I this new version the locks are held only for a small time (mainly the necessary time to insert/remove events in the queue) and not all the time necessary to handle all the events as previously. So it should improved accuracy with very little speed decrease.
While doing some test under Mario Galaxy I could not see any speed difference, but it is a bit hard to evaluated.
Well I hope you will accept this patch.
Updated by Billiard26 almost 14 years ago
Your "TimeOrderedEventQueueManager" looks unnecessary.
With these changes, having separate "_Threadsafe" functions looks pointless.
I have my own patch which does something like this, using a single priority_queue.
It makes the code a lot easier to look at.
Speed seemed unaffected. I don't know why I didn't commit it.
Updated by beistin almost 14 years ago
Indeed the separate"_Threadsafe" functions are pointless because they should all be threadsafe. I just kept the different version of the function and delegate everything to the Manager because I did not want to touch too many files as I am far from understanding all the code.
I created the TimeOrderEventQueueManager just to be more object oriented, but I have another version without it if you are interested.
I am waiting for your version of the patch :D
Updated by Billiard26 almost 12 years ago
- Priority set to Low
- Relates to maintainability set to Yes
- Operating system N/A added
Updated by Billiard26 almost 12 years ago
- Status changed from New to Won't fix
The patch no longer applies cleanly to latest master.
While the changes are probably good I would maybe say they are unnecessary.
Will re-open if provided an updated patch.