erkyrath / lectrote

The IF interpreter in an Electron shell
Other
248 stars 28 forks source link

Glulxe profiler #104

Open curiousdannii opened 6 years ago

curiousdannii commented 6 years ago

I thought people might appreciate being able to use the Glulxe profiler in Lectrote so they don't need to compile it themselves.

Porting Glulxe to emglken was easy enough. I set up the profile stream with glk_fileref_create_by_name/glk_stream_open_file, which works, but it's not ideal: the file isn't named .xml and it could be in a folder the user doesn't expect.

If this is something you'd be interested in including in Lectrote, do you have any ideas for a better way to make the profile stream? And Lectrote would need to provide some way to set the filename too (perhaps a separate menu item "Run with profiler" which opens a second file dialog for the profiler data after the user has picked the gamefile?)

curiousdannii commented 6 years ago

I want to keep the core VM decoupled from the actual creation of the profile stream, so maybe a function could be passed in the VM's options, and then the VM wouldn't need to be compiled with built in support for Lectrote.

Edit: Hm, I think I know what I'd like best. If a glkapi.js stream can be created manually for the output file, then the disprock can be passed into the VM's starting function, and it will just make a C struct to refer to it. That should be nicely decoupled. If the interpreter can make a file stream for an .xml file then great, but if it's only able to make a .glkdata stream that's fine too.

Hmm. If I'm reading the code correcting, Dialog.file_fopen looks like it will accept any filename directly, not just sanitised ones. That could be useful. It might be possible to overwrite the stream's .fstream property.

Unrelated question: For Glulxe in Emglken, the gamefile is a memory stream rather than a filestream. Does that mean the SERIALIZE_CACHE_RAM option is useless, and actually would result in duplicated memory?

erkyrath commented 6 years ago

Using the SERIALIZE_CACHE_RAM option with a memory stream will result in two copies in memory, yes. It's not entirely useless; the code in serial.c will access the SERIALIZE_CACHE_RAM array directly, rather than having to go through glk_get_char_stream calls. So it will be slightly faster.

Maybe it would be easier to use a file stream for the game file and then rely on SERIALIZE_CACHE_RAM for speed.

erkyrath commented 6 years ago

As for the general question: I'd like to generalize Lectrote in the direction of "there are several Glulx interpreters, which one would you like to use?" (Could be multiple Z-machine interpreter, too, of course. Et cetera.) So you could select from Quixe, Git, Glulxe, Glulxe+profiling.

(Glulxe+debugger is also a desirable option. That comes with the problem of selecting the gameinfo.dbg file to load.)

I haven't thought about UI or how to get the info into the core VM. The code in profile_quit() was pretty much written with cheapglk in mind, so I'm not surprised it's awkward. But does it make sense to open the profiling output file when the VM starts up? Having it open while the game is running could in theory disturb the game's functioning.

(Similarly, the debugger code is careful to open, read, and close the debug file before the game launches.)

curiousdannii commented 6 years ago

I've settled on a simple option, just create a stream and pass it in to the VM:

https://github.com/curiousdannii/emglken/blob/e99218b/tests/glulxe.js#L26-L37

One minor catch for glkote-term was that this needs to call Glk functions before Glk.init has been called, and so the external references weren't set. That probably won't be an issue for Lectrote as the files are loaded all upfront and then referenced using global variables.

My test script just makes a .glkdata file, but the ideal would be for it to be a .xml file. I guess you'll have to do that outside of the Glk api. But as long as you can pass a Glk compatible stream into the VM it should work.

Next step is to actually build Glulxe twice, with and without the profiler. Or is it not so much of a performance hit to have compiled but inactive profiling code to be worth having two builds?

erkyrath commented 6 years ago

It's worth having two builds.

curiousdannii commented 6 years ago

Alright, that's done. It builds to glulxe.js and glulxe-profiler.js.

Pull request: #106

If you are planning to update Electron soon then I'll switch to making WASM builds instead.

Edit: actually there are a lot of float errors when I try building with WASM, so let's leave that for later.

curiousdannii commented 6 years ago

Sorry that I didn't actually test it until now... but the Lectrote 1.3.0 release doesn't actually include the Glulxe files. Oops!

erkyrath commented 6 years ago

Oops. Gotta include them in makedist.py.