Converting OXCE to rapidyaml

[Delian]

Either share data, or have 1 global script that handles all RuleStatBonus, or have multiple global scripts, one for each type of stat. Or something. I don't know.
Or maybe, have an unordered_map cache of <script text, compiled script>. So if a script was already compiled, you don't need to compile it again, just copy the compiled script from the cache? And clear the cache after the loading is finished.

Edit: I just tested it, and it seems to work pretty well. RuleStatBonus::load() goes from 2050ms down to 320ms if I use a cache of compiled scripts.

[Yankes]

did you copy vectors content? this is not right as some operations refer to original memory in that vector.

[Delian]

If the script contains pointers, then obviously copying the compiled script isn't right. But as far as I can tell, the scripts generated in RuleStatBonus::load() don't contain any pointers. But let me check...

...nope, I don't see any pointers. Or at least none are dynamic. To check, I stored all the compiled scripts and compared the scripts with the same script text. And the conclusion is that all the scripts with the same script text always generated the same vector of bytes (in the _container._current._proc). For instance, all the scripts with the text:
mul bonus 1000; unit.firingBonusStats bonus 1000000 0 0 0; if ge bonus 0; add bonus 500; else; sub bonus 500; end; div bonus 1000; return bonus;
generated the same 87 bytes:
[13 00 00 e8 03 00 00 72 92 17 42 00 04 00 00 00 40 42 0f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 06 00 00 00 00 00 00 3b 00 00 00 47 00 00 00 0f 00 00 f4 01 00 00 01 4e 00 00 00 11 00 00 f4 01 00 00 25 00 00 e8 03 00 00 00 00]

[Yankes]

add debug text and it will not be true anymore. Overall optimization like this is ticking bomb as idea was scripts "proc buffer" is not copyable.
Changing this will hinder my future plans like adding const arrays that will be stored at end of this "proc buffer".

For me better solution would be change "owning" model of scripts that instead of vector they store only pointer to final "proc buffer" that is stored in common
object, similar how whole "global scripts" share data. Probably with some simple recounting.

Some pseudo code:

Code: [Select]

std::map<std::string, std::shared_ptr<const std::vector<Uint8>>> _cache;

auto c = _cache.at("script ...");
std::shared_ptr<const Uint8[]> _proc(c, c->data()); // only one indirection to access "proc buffer"

_cache.clear(); // drop all not used vectors after load


[Delian]

>add debug text and it will not be true anymore
I dumped the whole ModScriptGlobal and its internal state before and after the calls to RuleStatBonus._container.load() was the same. The only change in memory were the _proc vectors. Am I misunderstanding something? What is not true?

>scripts "proc buffer" is not copyable
Haven't you heard of self-modifying code? :p

>change "owning" model of scripts
That might reduce script memory usage. But would it reduce the CPU time needed for parsing?

My point is that I would only add caching for the scripts generated in RuleStatBonus::load(), because these scripts are simple (probably because they only read and write from CPU registers). Hmm. Maybe there exists a more general way to determine when scripts are simple? You could include a "simplicity" bool in the cache. So you can ask the cache: Is this script text a simple script? Yes -> then copy bytecode from cache. No -> don't copy the bytecode, just parse normally.

[Meridian]

Please don't lose focus.
Let's return to yaml lib migration.
This optimisation can be done separately.

[Yankes]

Please don't lose focus.
Let's return to yaml lib migration.
This optimisation can be done separately.

True, before scripts was around 3% of total load time and any improvement would be barely visible and now its only "problem" because everything else is running a lot faster. We should deliver this "faster" version before we try deliver "fastest" one.

>add debug text and it will not be true anymore
I dumped the whole ModScriptGlobal and its internal state before and after the calls to RuleStatBonus._container.load() was the same. The only change in memory were the _proc vectors. Am I misunderstanding something? What is not true?

>scripts "proc buffer" is not copyable
Haven't you heard of self-modifying code? :p

>change "owning" model of scripts
That might reduce script memory usage. But would it reduce the CPU time needed for parsing?

My point is that I would only add caching for the scripts generated in RuleStatBonus::load(), because these scripts are simple (probably because they only read and write from CPU registers). Hmm. Maybe there exists a more general way to determine when scripts are simple? You could include a "simplicity" bool in the cache. So you can ask the cache: Is this script text a simple script? Yes -> then copy bytecode from cache. No -> don't copy the bytecode, just parse normally.

when I mean debug script I mean `debug_log "Test" x;` code like this can easy happens when modder try override script or I add some debug lines to test default scripts. This is "footgun", it will fork fine for years but when someone forget it every thing will explode. I prefer that is physically impossible to make error like this.
Yes, adding some additional flags could be solution but is more a bandaid than fixing underling problem, this mean there is multiple scripts that are duplication of each other, my solution is simply represent this reality in script data itself by using `std::shared_ptr`.
In theory it should be cheaper as we pay only for atomic counter increment and we skip most of coping and memory allocations.

but anyway, as Meridian point out, this is not point of this "task", we should focus on yaml itself and deliver this first. when this is in hand of players then we can make it even faster.

[Delian]

Please don't lose focus.
Let's return to yaml lib migration.
This optimisation can be done separately.

Ok, I've also tested the embedded loader now. It seems to work fine.
To enable embedding in a windows build:
1. In the Project properties, go to C/C++ > Preprocessor, and edit Preprocessor Definitions and add EMBED_ASSETS.
2. In the Project properties, go to Resources, and edit Preprocessor Definitions and add EMBED_ASSETS. Yes, it has to be defined in two places.
3. The zip files need to be at OpenXcom\common.zip and OpenXcom\standard.zip. So above the project dir.

This will generate an exe file that is 4MB larger than usual.

What's next? Note that I can only do testing and building on Windows.

Current list of incompatibilities:
If a yaml line ends with ] or }, the trailing spaces need to be removed.
"  - a: [1, 2, 3]    " is bad
"  - a: { b: 2, c: 3 }    " is bad
"  - a: [1, 2, 3]" is good
"  - a: { b: 2, c: 3 }" is good
"  - a: [1, 2, 3]  #comment" is good

If a yaml line contains any (trailing) tabs, they need to be removed
"  - a: 1         " is bad
"  - a: 1         #comment" is bad
"  - a: 1" is good
"  - a: 1 #comment" is good

If a yaml line ends with a comment, the comment needs a preceeding space
"  - a: 1#comment" is bad
"  - a: 1 #comment" is good

damageAlter: FireThreshold no longer accepts float values
"      FireThreshold: 10.0" is bad
"      FireThreshold: 10" is good

strings starting with a single or double quote require a closing quote
 - 'Akamu   bad
 - Akamu    good
 - '''Akamu'   good
 - "'Akamu"   good

[Meridian]

What's next? Note that I can only do testing and building on Windows.

Thanks for the test, that's good news.

Now, I think it's up to me to test various builds and up to Yankes to continue review.
Give us some time, we're both rather busy with real life(tm).

[Meridian]

There was a problem with parsing UTF-8 files with BOM. I had to make a workaround that skips the BOM if it encounters it. I guess it's a bug in rapidyaml. Issue reported! Anyway, X-Com Files seems to work fine now.

What was the issue exactly?
I tried removing your fix and load a file with BOM, but I didn't encounter any error.

PS: looks like ryml author started working on your reported issues

[Delian]

The BOM gets included in the key of the first element.

So if my options.cfg has a BOM, after parsing, the key of first element is not going to be:
mods
but
mods
So loading options will quietly fail because it will look for a "mods" key, but won't find it.

It's great that the author started working on those issues. But until he releases version 0.7.3 of rapidyaml, there's not much to do. And even after it's release, that will only fix one of the incompatibilities. A major one for sure, but there's no need to hold your breath hehe.

[Meridian]

What's next? Note that I can only do testing and building on Windows.

I can test on various platforms.
But I have trouble understanding the instructions here: https://github.com/biojppm/rapidyaml?tab=readme-ov-file#using-ryml-in-your-project
I have practically zero experience with cmake and have no idea what to do.

Have you come across some sample project, which I could use as inspiration?
(our Linux, Macos and Android builds use cmake.)

In the meantime, I'll try the iOS build, which isn't based on cmake as far as I can tell.

[Delian]

Uhh... from what I can tell, all of those instructions are irrelevant. We don't use a single header file, and we aren't trying to amalgamate one. We aren't using the pre-built library. We aren't trying to make a library. We aren't using package managers to install anything, and aren't going to link the project in github (unless you want to). We aren't building a quickstart example, so the CMakeFiles.txt links don't matter. And we aren't using/changing any CMake variables because we're using default build options.

[Yankes]

but we need use cmake to compile gcc version, btw I already took this task. and find some bugs like missing includes
(yaml-cpp brings some headers that now are missing).
One thing to you to fix could be reduce warning spam in GCC:

Code: [Select]

In file included from /home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/RuleItem.h:23,
                 from /home/Yankes/prog/openxcom/src/Ufopaedia/ArticleState.h:21,
                 from /home/Yankes/prog/openxcom/src/Ufopaedia/Ufopaedia.h:24,
                 from /home/Yankes/prog/openxcom/src/Ufopaedia/UfopaediaSelectState.h:21,
                 from /home/Yankes/prog/openxcom/src/Ufopaedia/UfopaediaStartState.cpp:21:
/home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/Yaml.h:26: warning: ignoring ‘#pragma warning ’ [-Wunknown-pragmas]
   26 | #pragma warning(push)
      |
/home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/Yaml.h:27: warning: ignoring ‘#pragma warning ’ [-Wunknown-pragmas]
   27 | #pragma warning(disable : 4668 6011 6255 6293 6386 26439 26495 26498 26819)
      |
/home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/Yaml.h:30: warning: ignoring ‘#pragma warning ’ [-Wunknown-pragmas]
   30 | #pragma warning(pop)
      |
In file included from /home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/../../libs/rapidyaml/c4/yml/yml.hpp:11,
                 from /home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/../../libs/rapidyaml/ryml.hpp:4,
                 from /home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/Yaml.h:28:
/home/Yankes/prog/openxcom/libs/rapidyaml/c4/yml/parse.hpp:15:21: warning: redundant redeclaration of ‘c4::yml::id_type c4::yml::estimate_tree_capacity(c4::csubstr)’ in same scope [-Wredundant-decls]
   15 | RYML_EXPORT id_type estimate_tree_capacity(csubstr src);
      |                     ^~~~~~~~~~~~~~~~~~~~~~
In file included from /home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/../../libs/rapidyaml/c4/yml/yml.hpp:9:
/home/Yankes/prog/openxcom/libs/rapidyaml/c4/yml/parse_engine.hpp:767:21: note: previous declaration of ‘c4::yml::id_type c4::yml::estimate_tree_capacity(c4::csubstr)’
  767 | RYML_EXPORT id_type estimate_tree_capacity(csubstr src);
      |                     ^~~~~~~~~~~~~~~~~~~~~~

last one look like sloppy declaration in lib itself.

[ps]
some fixes:
https://github.com/Yankes/OpenXcom/tree/rapidyam

[ps2]
https://stackoverflow.com/a/55877109/1938348 probably best way to handle warnings like this in portable code.

[Delian]

So... Hedley?

Btw, there's one bug which was fixed in the commits that I made after you made the fork.