Project

General

Profile

Actions

Emulator Issues #4283

closed

CoreTiming code cleanup patch

Added by beistin about 13 years ago.

Status:
Won't fix
Priority:
Low
Assignee:
-
% Done:

0%

Operating system:
N/A
Issue type:
Other
Milestone:
Regression:
No
Relates to usability:
No
Relates to performance:
No
Easy:
No
Relates to maintainability:
Yes
Regression start:
Fixed in:

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.

Actions #1

Updated by Billiard26 about 13 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.

Actions #2

Updated by beistin about 13 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

Actions #3

Updated by skidau about 13 years ago

  • Issue type set to Other
Actions #4

Updated by Billiard26 over 11 years ago

  • Priority set to Low
  • Relates to maintainability set to Yes
  • Operating system N/A added
Actions #5

Updated by Billiard26 about 11 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.

Actions

Also available in: Atom PDF