indigodarkwolf / box16

A fork of the official X16 emulator, converted to C++20 and with a bunch of features tweaked and added.
MIT License
42 stars 19 forks source link

C++20? #70

Closed FlightControl-User closed 1 year ago

FlightControl-User commented 1 year ago

Would it be an issue if i would "upgrade" the vc repository to C++20? Now it is at C++17. Reason, C++20 provides within std new capabilities like format for string formatting and jthread for threading. This can be used to implement faster trace buffers and in a thread to output the trace while execution is progressing. Compiling the code in C++20 mode, gives some minor issues with some char conversions.

indigodarkwolf commented 1 year ago

The only problem I have with C++20, and the reason I've held off from upgrading, is support on compilers. C++20 would need to be supported by the major Linux distros' build-essential packages (i.e. g++), as well as msvc and Raspian. At present, g++ on my WSL2 distribution of Debian is limited to 8.3.0-6, which doesn't have full support for the C++20 standard. That also looks to be the latest version backported to Debian Buster. And my Raspian distro is in a similar state.

I see my Windows 11 install of WSL has g++ 10.2.1-6, which should have full C++20 support, so I'll have to see about upgrading to Debian Bullseye... it doesn't look that involved, but I don't want to attempt it at the end of the evening on a work night. And for Raspian, I might just re-flash the SD card with the latest version, after I've backed up any files that I haven't already and want to preserve. Also not something until the weekend, at least.

So before I'd approve moving to C++20, I need to upgrade my own build environments so I can support C++20 across all the platforms I officially support.

What I will otherwise say, though, is that I'm going to pay a lot more attention and be a lot more cautious about any commits or PRs involving multithreading. I've lost count of the number of times I've been burned in various ways by multithreading, sometimes months or years after implementing something, and I'm extremely skeptical that it's the best answer, or even a necessary answer, to performance problems in trace output.

Also, since this is performance critical code, I'll admit I'm also skeptical that wins will actually come from using std::format since its interfaces all appear to require building a formatted string before forwarding it to any sort of stream that would, by necessity, have to subsequently copy the string data. That seems like another place where you could easily lose more than you think you're winning, and that's considering that the trace logging already has a lot of string copying going on as it builds subsections of the output to be eventually formatted together onto a line.

But I suspect you could get a lot of performance wins just by formatting out to a larger buffer, say 8KB, and then writing out in bulk increments once it's filled to 4KB or something.

FlightControl-User commented 1 year ago

OK. thank you for the feedback. I wanted to first ask. Then i'll keep things very simple for the moment and when the time comes we will include more advanced functions. format is a great improvement, better than sprintf and the likes :-). There won't be any copy if you use format_to().

Anyway, thanks for the feedback, let us hold off on these changes for now, but at least we've exchanged some info on the idea and the dependencies and complexities. Let us keep this thread open (I suggest) until we see the moment is ready.

Is ok?

FlightControl-User commented 1 year ago

But I suspect you could get a lot of performance wins just by formatting out to a larger buffer, say 8KB, and then writing out in bulk increments once it's filled to 4KB or something.

Have implemented this and want to send a new pull request. Just sorting out the memory buffering for the moment. Also, have extended the -dump functionality with an L option, to express the buffer length in lines ... Once it's in a good state i'll commit and share the code in my branch so you can have a look first before i pull request. I'll open a different issue regardiing this topic, not to mangle subjects .

indigodarkwolf commented 1 year ago

Works for me. And yeah, looks like std::format_to does address a lot of my concerns with string copies. Thanks for pointing that out.

indigodarkwolf commented 1 year ago

I have my Pi updated, and I have the solution building on it and in Windows. Once I get my WSL Debian install updated, I'll be pushing the changes to enable C++20. I'm not 100% sure when that'll be, because I'm uncertain whether it'll require a Windows reboot. In case it does, I'll be waiting until a lengthy file copy is done -- there's some story behind it, but you'd need to be on the Commander X16 Discord server and it's just too depressing for me to want to re-hash.

Those const warnings... I swear, every time something shows up in a warning, it seems to lead me to coding practices that bug me as dangerous. one of these days I'm simply going to enable all the extra compiler warnings, pedantic and otherwise, and go on a tear fixing all the warnings.

indigodarkwolf commented 1 year ago

Now running C++20.