Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make units cache promotions for fast looping #128

Closed
Nightinggale opened this issue Sep 25, 2018 · 8 comments
Closed

Make units cache promotions for fast looping #128

Nightinggale opened this issue Sep 25, 2018 · 8 comments
Labels
Code Efficiency Make code more efficient IDEA This is a game enhancement that is not accepted by the community yet.
Milestone

Comments

@Nightinggale
Copy link
Contributor

While working on CivEffects #93, I added some new array classes to the CivEffect branch. What's special about them is that they can take each other as arguments for certain tasks and are aimed at caching data for fast runtime execution while remaining simple to use for coding.

It suddenly occurred to me that those arrays allows us to cache the active promotions in a unit in a way aimed towards fast looping of all active promotions.

Note: this got a bit lengthy, but it explains what goes on behind the function calls. The whole idea in this is that it can use existing code for most of the tasks mentioned here, meaning it gets reduced to a few simple function calls.

How the cache is used:
We add an InfoArray of type JIT_ARRAY_PROMOTION to CvUnit. It stores the active promotions for the unit, not in a normal array, but rather it stores a sorted array of indexes of the promotions the unit has. This means code can be written like:

for (int i = 0; True; ++i)
{
    PromotionTypes ePromotion = array.get(i);
    if (ePromotion == NO_PROMOTION)  break;
    // whatever code we want to do with the promotions
}

If the unit has two promotions, that loop will run 2 iterations. Out of bounds arguments in get returns -1 (or NO_PROMOTION). Usually units have way less promotions than promotions are available in xml, hence they loop a lot of promotions where it goes "no, don't have that one".

int InfoArray::getLength() const exist too if you want that approach instead. The difference in performance for those two approaches seems to be non-existing.

How to set the cache:
Make a BoolArray of type JIT_ARRAY_PROMOTION. As the name indicates, it's an array containing true/false values and the JIT_ARRAY enum value gives it the length to match the promotion xml file. Set the default value to false (which is the default, no change here).

Loop all promotions, for each check the real and free promotion arrays in the unit and and for each, which is true in at least one of them, check that it can use the promotion according to xml (a unit can have a real promotion, change profession and not use it, but store it in case it changes back). For each promotion, which passes all those tests, the unit can use it and use BoolArray::set(index).

Once done, call the InfoArray with assign(const BoolArray*) and the into array will be created using only the indexes of the true values. The InfoArray is sorted. Calling on a non-empty InfoArray will clear the old one.

Recreate the InfoArray each time the unit gains a promotion (or loses a free promotion) or change profession.

Savegames:
No change. Just set the InfoArray on load. Not only will this approach avoid savegame compatibility issues, it also allows xml changes to apply to existing savegames. In other words it's using the savegame idea in #46.

Code readability note:
We could make some InfoArrayPromotions class, which inherits InfoArray, but with a default contructor without arguments (always promotions) and get returns PromotionTypes. In fact making a bunch of those should make the code using InfoArrays in general more clean.

@Nightinggale Nightinggale added IDEA This is a game enhancement that is not accepted by the community yet. Code Efficiency Make code more efficient labels Sep 25, 2018
@Nightinggale
Copy link
Contributor Author

Another place where an InfoArray could be used would be domestic market. At game startup, make an InfoArray in CvGlobals for Yields. Loop all units to make a BoolArray to tell which yields are possible to sell at the domestic market.

The end result: the domestic market gets hardcoded to skip yields we know will never be sold at the domestic market. However unlike a lot of other hardcoding, this one is adapting at startup, meaning it builds itself based on xml and will handle if somebody decides to make carpenters consume lumber or some other yield currently not sold at the domestic market.

@ShadesOT
Copy link
Contributor

Night, these are good ideas. Don't burry them as a reply to another issue. Make them their own issue.

@Nightinggale
Copy link
Contributor Author

Nightinggale commented Sep 25, 2018

I think of this as linked because it's a proposal for a coding concept. Once we agree on the concept and the coding too (it's currently only in an experimental branch), then we can start to make a number of tickets, one for each task. As long as the ideas are here, they aren't buried and it keeps the description of possibilities of the concept together.

If we post lots of tickets, which relies on code we haven't even agreed on adding, then we can quickly bloat the issue tracker.

@Nightinggale
Copy link
Contributor Author

New consideration:

Make a new version of the InfoArray, which stores pointers rather than indexes. This way looping active pointers would provide a pointer to CvPromotionInfo rather than the index, which would then require GC.getPromotionInfo(ePromotion).

It would require a new class rather than reusing, meaning we would need a new class name for such a cache array. What should it be?

If we send xml info class pointers around as arguments and return values instead of indexes, we would lose the awareness of indexes. This is fixable to adding some getIndex() to the xml info classes, which seems to be trivial (M:C has it for yields). Perhaps it would be a good idea to add and then start to use xml info class pointers in general instead of indexes when calling other functions.

@Nightinggale
Copy link
Contributor Author

The more I think about this, the more I lean towards a new class with pointers. It should be very simple, just assign, get and getLength. Info classes should have an inline getIndex. There should be a base class, which is inherited by a number of other classes like for instance a yield class. The base class does most of the work, but it avoids setting templates or constructor arguments and get returns const CvYieldInfo pointers. The promotion class does the same, but get returns const CvPromotionInfo pointers.

Another task where this would be beneficial would be yields from plots. Right now when the potential yields from a plot are calculated, it loops all of them. If we know from xml at startup that certain yields will always be 0, we should skip trying to calculate it. Stuff like YIELD_TOOLS.

Nightinggale added a commit that referenced this issue Oct 11, 2018
… of xml infos

The idea is that if we know certain loop iterations will never do anything, we can skip them for performance reasons without changing functionality.
Those arrays can be set up once or rarely and then looping them will provide info pointers or enum values respectively for the indexes where something can happen.

Added getIndex() to info classes for Professions, Promotions and Yields to allow getting the index from the info pointers without resorting to a lot of slow code.

As part of #128, yields consumed by units at domestic market are now set in a EnumTypeCacheArray array at part of post xml load.
This means with the current xml settings, handling doTurn for domestic market takes 19 loop iterations instead of 58 and it still produces the same result.
@Nightinggale
Copy link
Contributor Author

I started implementing this as part of the CivEffect #93 branch. The reason is that it would be useful for new CivEffect code.

@Nightinggale Nightinggale added this to the 2.8 milestone Sep 26, 2019
@Nightinggale
Copy link
Contributor Author

The 2.8 branch is now caching promotions quite aggressively in a way, which makes most looping avoidable.
I'm not closing this issue yet because it touch upon the savegame format change too, which is currently ongoing.

@Nightinggale
Copy link
Contributor Author

Implemented ages ago

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Efficiency Make code more efficient IDEA This is a game enhancement that is not accepted by the community yet.
Projects
None yet
Development

No branches or pull requests

2 participants