search

johanberntsson / ozmoo

A Z-machine interpreter for the Commodore 64 and similar computers
GNU General Public License v2.0
117 stars 19 forks source link

Add vmem stress test and fix some vmem bugs #13

Closed ZornsLemma closed 4 years ago

ZornsLemma commented 4 years ago

Hi,

I've been playing around with the virtual memory implementation as part of my Acorn port of ozmoo (https://stardot.org.uk/forums/viewtopic.php?f=2&t=19975&p=279511#p279067).

This pull request adds a new debug option VMEM_STRESS which forces only two blocks of virtual memory to be available to stress-test the virtual memory system. Since we only need one block for the PC and one block for data access, I think this should work, although of course it's incredibly slow. I found two bugs in the virtual memory implementation which stopped this working; these are fixed by one of the commits on this branch (and the commit message has further details). With this fix, the benchmark runs without crashing as far as I've had time to execute it.

I haven't seen these bugs trigger in a more realistic situation, but I don't think it's impossible they could come into play so I think it's worth fixing them.

Let me know if you have any questions!

Cheers.

Steve

johanberntsson commented 4 years ago

Thanks, looking good but I want to check with Fredrik before merging. I'm impressed with your Acorn work and I hope we can add it to Ozmoo eventually. I have recently added a -t:target option to prepare for this. Can you send me an email to gsejapan AT yahoo DOT com? Easier to have direct contact.

fredrikr commented 4 years ago

Hi Steve,

Looks good.

One question though: Can the new error (NO_VMEM_INDEX) actually occur when using the interpreter to play a game? If so, why is it only checked when in debug mode?

/Fredrik

ZornsLemma commented 4 years ago

Hi Fredrik,

The NO_VMEM_INDEX error is really a kind of assertion - if there are no bugs in the Ozmoo vmem code, it should never occur.

Before this change, vmem_oldest_index always had a valid index in (even if it might be one which contained the PC and so wasn't really valid). With this change, vmem_oldest_index is (in a non-DEBUG build) only populated inside the loop if we find a non-PC vmem block older than the initial vmem_oldest_age of $ff. That should always happen, but what if it doesn't? We'd be left with an invalid index and would start to trample on random stuff. I can't see how this could happen, otherwise I'd have fixed the problem already, but it seemed like a good idea to have a check, at least at first while this change is new and relatively untested.

Since the check costs a few cycles and bytes to implement, I thought I'd make it conditional on DEBUG. That way the check gets included in a BENCHMARK run which seems to me to give the code a fairly decent hammering (with or without VMEM_STRESS turned on). It might be better to just do the check all the time; I'll bow to your judgement on that, as you will have a far better idea of Ozmoo's philosophy here.

Cheers.

Steve

ZornsLemma commented 4 years ago

That's great, thanks!