X16Community / x16-emulator

Emulator for the Commander X16 8-bit computer
BSD 2-Clause "Simplified" License
205 stars 43 forks source link

add Memory statistics dump option. #280

Closed irmen closed 5 months ago

irmen commented 5 months ago

simple read/write access counting dump when emulator exits, enabled by the new -memorystats option

mooinglemur commented 5 months ago

Is there any benefit to having it write to stdout rather than a file?

irmen commented 5 months ago

I dunno? It was easiest for me to make (I'm not very comfortable programming in C/C++) What do you propose instead?

mooinglemur commented 5 months ago

I think the command line option with the filename parameter would be a better fit, so that the standard out of the emulator can still be used for other purposes like debug or echo.

I would recommend looking to some of the other code in the emu for examples of using SDL for file I/O, otherwise it's pretty straightforward.

https://wiki.libsdl.org/SDL2/SDL_RWFromFile to open, https://wiki.libsdl.org/SDL2/SDL_RWwrite to write, and https://wiki.libsdl.org/SDL2/SDL_RWclose to finalize.

irmen commented 5 months ago

Ok that makes sense. Is there a particular benefit to use SDL for file I/o as well? Rather than C functions?

mooinglemur commented 5 months ago

Portability and consistency. It abstracts away some of the subtle platform-specific differences, specifically for error handling and file modes. We're already using SDL for video and sound, and for most of the file ops in other parts of the emu.

irmen commented 5 months ago

Can you give an example of how to use that Sdl write function to write a simple string? It looks like it is meant to write binary objects instead? For example, I'd like to convert printf("Usage counts of all memory locations. Locations not printed have count zero.\n"); but unsure how to do this with the sdl call

mooinglemur commented 5 months ago
const char emptymsg[] = "Usage counts of all memory locations. Locations not printed have count zero.\n";
int i, siz;
char buf[200];
SDL_RWops *f = SDL_RWFromFile("debug.txt", "w");
// handle error if f is NULL and check SDL_GetError()

for (i=0; i<0xa000; i++) {
  if (mem[i] == 0) {
    if (!SDL_RWwrite(f, emptymsg, sizeof(emptymsg), 1)) {
      // handle error using SDL_GetError();
    }
  } else {
    siz = snprintf(buf, sizeof(buf), "%d: %d\n", mem[i], i);
    if (!SDL_RWwrite(f, buf, siz, 1)) {
      // handle error using SDL_GetError();
    }
  }
}

// untested of course :)
irmen commented 5 months ago

ty. What a tedious api :|

irmen commented 5 months ago

Ok I think I got it working to write to a given file instead.

mooinglemur commented 5 months ago

ty. What a tedious api :|

SDL doesn't handle format strings. So ya gotta glue the two things together somehow. :) It doesn't seem tedious to me because I guess my expectations are already set for how C behaves.

mooinglemur commented 5 months ago

With these changes, there are quite a few places that need to read w/ debugOn set that currently do not, otherwise some code will look hotter than it ought to.

For instance, set_kernal_status() inside main.c intuits a memory location based on what code looks like. We'll probably want to debugOn all of these and call real_read6502 instead.

In addition, within #ifdef TRACE in main.c, line 1496 in your branch reads the opcode and operand out without debugOn, so if you're running w/ trace output, all code will be twice as hot as it should be.

I can make these changes after accepting, or if you'd like to do it, I can defer :)

irmen commented 5 months ago

I'll have a look in the coming days to see if I can understand those mechanisms.

irmen commented 5 months ago

I tried a thing (see previous commit). But I have a few questions:

  1. I can't compile with TRACE=1 -> it's missing "rom_labels.h" file.
  2. Is the approach correct? I.e. setting debugOn to false on reads from the cpu simulator and true everywhere else
  3. with the extra boolean argument on every read, performance of the emulator still looked pretty much identical as before. But perhaps you can think of a way to not have to do this inside the cpu simulator for instance?
  4. what about writes?
mooinglemur commented 5 months ago

I tried a thing (see previous commit). Few things:

1. I can't compile with TRACE=1  -> it's missing "rom_labels.h" file.

You have to get this from the x16-rom build that you want to do traces on. copy from x16-rom/build/x16/*.h into x16-emulator/src

2. Is the approach correct? I.e. setting debugOn to false on reads from the cpu simulator and true everywhere else

Rather than changing read6502()s prototype and everything everywhere, I would have instead changed only the affected areas. in is_kernal(), set_kernal_status(), and the TRACE area, change the read6502(addr) to real_read6502(addr, true, 0).

The one you changed in ieee.c should probably real (non-debug)

3. performance looked pretty much identical as before, with the extra boolean argument on every read, but perhaps you can think of a way to not have to do this inside the cpu simulator for instance?

Unless there's a clear benefit of doing it this way, I would do what I mentioned for 2

4. what about writes?

There are no debug writes. It's not a thing. (but perhaps in the future we'd add it)

mooinglemur commented 5 months ago

As for performance, I'd love to get rid of as many conditionals as possible, but I wouldn't worry about it now. The host processor's branch prediction is likely not to hurt too much, but I will probably benchmark some trial changes later, removing some of the branches by using function pointers instead of conditionals. Especially with the added 65C816 support, there are a lot of new conditionals which have a chance of not having been optimized out.

irmen commented 5 months ago

change the read6502(addr) to real_read6502(addr, true, 0).

This will skip the test for uninitialized ram reads though, but I guess those "should never happen" for debug-reads?

Anyway, replaced the previous attempt by your suggestions.

mooinglemur commented 5 months ago

This will skip the test for uninitialized ram reads though, but I guess those "should never happen" for debug-reads?

Yup, that's fine. Debug reads should never end up in the uninitialized RAM list. The ones you changed behavior for are unlikely to ever have been encountered as uninitialized anyway as they would have been touched beforehand. The execution would touch them before the trace disassembly, and the others are generally ROM.