-
Notifications
You must be signed in to change notification settings - Fork 2k
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
z80: interruptible execution #10047
z80: interruptible execution #10047
Conversation
More confident now... Let's try to make it happen! |
drives identified as broken with PR:
|
All not working drivers identified during testing fixed! |
Can you mark this as Ready for Review? |
I think it is. Yet actuated with current master. |
There seem to be 2 lines of thought with this, yet surprisingly nobody leaving comments here. The first is that the Z80 code is already ugly, relies too much on macros, and trying to fit these changes in a core never designed for it is a waste of time and we should just wait for a complete new core. The other is that this is the best way to get these changes, as it built on top of a mostly trusted Z80 core, so if things break in the process it'll be easier to identify and fix them, whereas a brand new core would almost certainly come with it's own set of problems at first that might make it difficult to identify the source of issues. I'm in the latter camp, I think this is worth doing, but maybe the lack of other comments / movement suggests that is a minority opinion. |
I agree lack of comments on this PR is discouraging. I open to any suggestions if anybody is interested. |
I don't know what the performance difference is, but I still think with
the macros that this will be easier to deal with than a python code
generator.
…On 30/08/2022 20:21, David Haywood wrote:
I'm in the latter camp, I think this is worth doing, but maybe the
lack of other comments / movement suggests that is a minority opinion.
Message ID: ***@***.***>
|
We should expect performance drop because old code was highly optimized with inlining which is not possible in inerruptible version. Based on my naive "max frame skip" performance test, the drop is ~40% from original. |
ultimately this is going to be the decision of @cuavas will you allow this route to be taken? aside from a performance decrease (which will likely happen even if a new cycle-accurate core is written from scratch) this doesn't inherently seem worse than what we have on a technical level. personally I'd like it if at the point we get python generated cores, that it be possible to generate 2 different cores, one 'performance' core, where we're confident that cycle accuracy never comes into play, and another where it does, in order to reduce any performance loss for the less demanding cases, but if such a core is written we can deal with it at the time. as it stands there are a number of cases where improving the Z80 is going to be important and could lead to improvements in the emulation of other components. |
@cuavas since no consensus can be reached on this, how about some kind of compromise, like including it as a secondary core, /z80_interruptible or similar? that way, it can be used where there seem to be benefits, with no risk to anything else, and can prove its worth in those situations? |
Are you a member of the forums? Can post threads there or chat in the chat box. https://forums.bannister.org/ |
added as a comment - sharing for discussion here:
|
I can declare a victory now! |
If there are concerns regarding this PR and we still don't see it happen, I can move tables removal and flags update to master branch. |
…anda y Goñi, Peter Helcmanovsky]
@galibert your turn |
What`s the performance drop now? |
If I did everything right... ~30%:
|
This cannot be compiled on a Mac with 16 GB of RAM and no other applications open. That's a serious problem. |
John IV reports "pacman -bench 90 from 18110 to 5749". That's 30% of the former speed, which is also a serious problem. |
Further:
Mag_xain 488 to 234, 52% hit
Tnk3 4056 to 1524, 62% hit
|
Let's revert it then. I don't have enough technical knowledge to fix these performance issues quickly. |
Yep. |
@rb6502 @john-iv
|
Credits:
z80 interruptible implementation is ready for review
Main execution don't overflow scheduler frame more than 3 icounts (atomic for IO read/write)
take_interrupt() hasn't been converted yet (possibly TODO)
Drivers which must be updated before all TIME_GUARD sections completle removed from the code:
AmstradMSXSystem1