The modders' lair: (CIP) Code Improvement Project

Commander Bello · Dec 24, 2013

Hi and merry Christmas to all of you!

First, let me remind you that I am only a beginner in coding, so look at my remarks with some cautiousness. :mischief:

Intention:
Anyway, I want to start this thread as a point where modders (DLL and Python) can meet and exchange findings about weaknesses, errors, bugs and strangenesses in the vanilla code.
This shall give all of us the chance to mention such things and (in the best case) to find solutions to improve the code - which then in turn of course can be used in any mod.

It is important that this thread is not meant to be used for discussions of mod-specific functionality, but only for vanilla code as this (unless already changed in the mods) will impact all of us.
Furthermore, this is not meant to be a "wish list thread", except for the wish to improve the mentioned functionalities.

If someone finds a solution (correction, improvement, whatever) for any of the findings here, it would be very much appreciated if this solution would be released here or if a link would be placed to where such solution can be found.
Thanks in advance for contributing.

Findings:
Let me start with this:

In CvPlayer.cpp we have the doTurn() function which itself calls several other functions, one of them being the doGold()

Spoiler :

As far as I see it, this function just doesn't do anything (at least, it doesn't do anything meaningful except for consuming computing power).
From placing, naming and included functions we may assume that doGold() shall update the player's gold at the beginning of the turn - which actually it doesn't.

To me, it looks pretty much like this function could just be deactivated.

Commander Bello · Dec 25, 2013

The next item would be the function bool CvPlayerAI::AI_counterPropose(...) in which we find this:

Spoiler :

Obviously, the condition && iAIDealWeight > iHumanDealWeight of the for(...) statement will always be met and therefore is just obsolete.

raystuttgart · Dec 25, 2013

Commander Bello said:
Obviously, the condition && iAIDealWeight > iHumanDealWeight of the for {....} statement will always be met and therefore is just obsolete.

Sorry, but that is wrong.
Here you don't understand C++ coding correctly.

While this is the condition to enter some logic:

Code:

if (iAIDealWeight > iHumanDealWeight)

That condition might become false through the actions of the logic in the for loop.

Thus this condition (exit condition of for loop) might indeed become false afterwards:

Code:

for (pNode = pTheirInventory->head(); pNode && [COLOR="Red"]iAIDealWeight > iHumanDealWeight[/COLOR]; pNode = pTheirInventory->next(pNode))

So basically this says:

1) Enter the logic in the if block if the condition is met.
2) Exit the for loop (within the if block) if the condition of the for loop - which is checked every run through of the for loop - is not met anymore.

The condition will not always stay true within the for loop since the for loop itself will change it.

Edit:
Please be careful with statements like "obviously this code is wrong / obsolete" if you do not fully understand what is happening.
Other unexperienced modders might believe such statments and heavily damage their mod.

Commander Bello · Dec 25, 2013

raystuttgart said:
Thus this condition (exit condition of for loop) might indeed become false afterwards:

Code:

for (pNode = pTheirInventory->head(); pNode && [COLOR="Red"]iAIDealWeight > iHumanDealWeight[/COLOR]; pNode = pTheirInventory->next(pNode))

So basically this says:

1) Enter the logic in the if block if the condition is met.
2) Exit the for loop (within the if block) if the condition of the for loop - which is checked every run through of the for loop - is not met anymore.

While this is true, I actually cannot see any command within the loop which would change either, the iAIDealWeight nor the iHumanDealWeight.
I admit that I should have inserted the whole content of the for() loop, which I now have changed in the respective posting.

raystuttgart said:
The condition will not always stay true within the for loop since the for loop itself will change it.

(Marking of 'will' by myself)
And that is what I miss to see happening.

Spoiler :

raystuttgart said:
Edit:
Please be careful with statements like "obviously this code is wrong / obsolete" if you do not fully understand what is happening.
Other unexperienced modders might believe such statments and heavily damage their mod.

This is a good hint, but as I said, I don't see any changes for the respective part of the exit-clause.

raystuttgart · Dec 25, 2013

Yes, there are 3 for loops within the if.
(Just remembered that they do manipulate the deal weights.)

In the first for loop the condition "iAIDealWeight > iHumanDealWeight" could indeed be left out.
(The deal weight is change after the for loop and not inside it.)

Leaving it in though, will not really hurt either, since that condition is only comparing 2 integers.
(So actually that really is nothing to worry or bother about since its effect is really extremely minimal.
Comparing 2 integers 10 times or even a hundred times, will not even cost a microsecond.)

In the other 2 for loops however, it is very important.

So ok, sorry for the misunderstanding. :thumbsup:

(After you had posted the full code of the for loop you are talking about, it was much clearer.)

Commander Bello · Dec 25, 2013

raystuttgart said:
Yes, there are 3 for loops within the if.
(Just remebered that they do manipulate the deal weights.)

In the first for loop the condition "iAIDealWeight > iHumanDealWeight" could indeed be left out.
(The deal weight is change after the for loop and not inside it.)

Leaving it in though, will not really hurt either, since that condition is only comparing 2 integers.
(So actually that really is nothing to worry or bother about since its effect is really extremely extremely minimal.)

In the other 2 for loops however, it is very important.

So ok, sorry for the misunderstanding.
(After you had posted the full code, it was much clearer.)

No sweat

I should have posted the full content from the beginning. :sad:

But thanks for mentioning the other two for() loops, as they are all (so three in total) content of the one and same if() statement, which makes me thinking that this whole branch could have been coded in a more efficient way.
To me, at the moment it looks pretty much like running three times through the same conditions (as stated in the first lines of these for() loops) where just one loop would have served the same purpose.

raystuttgart · Dec 25, 2013

Commander Bello said:
But thanks for mentioning the other two for() loops, as they are all (so three in total) content of the one and same if() statement, which makes me thinking that this whole branch could have been coded in a more efficient way.

Is it worth spending hours of work into recoding something that works and thus risk introducing bugs, just to save a microsecond (or a few mircroseconds) every turn ? :dunno:

Commander Bello · Dec 25, 2013

raystuttgart said:
Is it worth spending hours of work into recoding something that works and thus risk introducing bugs, just to save a microsecond (or a few mircroseconds) every turn ?

Depends, as I would say.

As long as the recoding doesn't take too much time AND it will make the code faster, I would always do it.
I have come across many of such cases and finally, they all will add up.

The example we have been talking about very well may not be the most important one, I give you that.

But my idea was that if we who are dealing with the code would collect all such occurences, we all would have the chance to improve the codes of the respective mods, thus making any of our mods a bit faster.
This is meant to be a long time effort, of course.

But we are all adding functionality which consumes computing power. And as we know, players don't like to wait for the game to respond. So, the faster a combined effort can make the code, the better for all of us. :thumbsup:

raystuttgart · Dec 25, 2013

Commander Bello said:
Depends, as I would say.

Well, it is your time and effort. :thumbsup:

But removing an unnecessary safety check like in this example is not really changing anything.
You could remove 100 of such unneccessary safety checks and would probably not notice anything.

On the other hand, there is logic that really has a lot of effect on performance.
(AI for Units, City AI for Profession, Improvments to Caching, ...)

For real performance improvments it might even be a good idea to do some profiling to find out which methods really cost performance.
Then, once you have identified these methods, you could try to improve those.

Personally I believe that the following factors should be in a good relation:
benefits -- efforts -- risk

Hours of work for saving a few mircoseconds each turn and even risk introducing bugs is not necessarily the most efficient modding.

Commander Bello · Dec 25, 2013

I completely agree.

Yet, profiling is way beyond my skills. All I can do up to now is to have look at the code and to try to identify such occurrences. And when I'm changing the code anyway, then I can do the small changes too, can't i?

And be assured, there will be other examples coming which - as far as I see - have a much higher impact on game speed.

Commander Bello · Dec 27, 2013

Next item, as far as I see it, would be CvPlayerAI::AI_doCounter()

Spoiler :

It is important to know that this function is called by CvPlayer::doTurn() calling CvPlayerAI::AI_doTurnPost() calling AI_doCounter()

As far as I see it, the blue part can easily be put behind the green part, thus saving NUM_PLAYERS^2 times the check for isAlive() - per turn.

Commander Bello · Dec 27, 2013

And while I am at it, I had a look at CvPlayer::doTurn() calling CvPlayer::doUpdateCacheOnTurn()

Here's the result:

Code:

void CvPlayer::doUpdateCacheOnTurn()
{
	// add this back, after testing without it
	// invalidateYieldRankCache();
}

Commander Bello · Dec 27, 2013

Long time no see

Once again a function called by CvPlayer::doTurn(), this time it is CvPlayer::doCrosses()

Spoiler :

As doCrosses() is only called by CvPlayer::doTurn() I would propose to have the check about the player's nature put into doTurn() and to enhance it like this:

Code:

if (!isNative() && !isEurope() && isAlive())
{
    doCrosses();
}

On second thought, it may be shorter to replace the
~~if (!isNative() && !isEurope() && isAlive())~~
with the red one from the original doCrosses():
if (getParent() == NO_PLAYER)

Commander Bello · Dec 31, 2013

And another function indirectly called by CvPlayer::doTurn()

GC.getGameINLINE().verifyDeals();

If we look at verifyDeals(), we find the following:

Spoiler :

Looking at verify(), we find this code here:

Code:

void CvDeal::verify()
{
	bool bCancelDeal = false;

	if (isCancelable(NO_PLAYER))
	{
		if (isPeaceDeal())
		{
			bCancelDeal = true;
		}
	}

	if (bCancelDeal)
	{
		kill(true, NO_TEAM);
	}
}

which, as far as I see it, can be rephrased to:

Code:

void CvDeal::verify()
{
	if (isCancelable(NO_PLAYER) && isPeaceDeal())
	{
		kill(true, NO_TEAM);
	}
}

raystuttgart · Dec 31, 2013

Commander Bello said:
On second thought, it may be shorter to replace the
~~if (!isNative() && !isEurope() && isAlive())~~
with the red one from the original doCrosses():
if (getParent() == NO_PLAYER)

Careful.
Don't get confused.

If at all you could replace

Code:

if (!isNative() && !isEurope())

with

Code:

if (getParent() [COLOR="Red"]![/COLOR]= NO_PLAYER)

Because colonial powers do have a parent.
(Natives and Kings don't.)

But that would still not check for the Player still being alive.

Commander Bello · Dec 31, 2013

raystuttgart said:
Careful.
Don't get confused.

If at all you could replace

Code:

if (!isNative() && !isEurope())

with

Code:

if (getParent() [COLOR="Red"]![/COLOR]= NO_PLAYER)

Because colonial powers do have a parent.
(Natives and Kings don't.)

You're right, of course.

Don't know, what I was thinking in that moment. Most probably, just nothing. :crazyeye:

raystuttgart said:
But that would still not check for the Player still being alive.

Right again.

Here I wanted to have a look at when CvPlayer::doTurn() is called.
For the moment, I assumed that since the other functions called by doTurn() don't have an "alive"-check, it is only called for alive players.

Nightinggale · Jan 1, 2014

If you want a faster vanilla, then look here: https://github.com/Nightinggale/Rel...mmit/9e433ec2932d81e54175ea796261a271369c8537

This cache is designed in RaRE and then moved to both RaR and M:C. However it should work in vanilla as it improves on vanilla code. This commit reduced waiting time on the AI (next turn wait) by 17.5% in RaRE meaning it is likely the greatest performance improvement you can get anywhere near based on what you tell about your skills. It does rely on JustInTimeArray.h, which is a file I created for RaRE. If you want to use it, then I recommend getting the newest version of this file from M:C as I improve on it from time to time.

raystuttgart said:
For real performance improvments it might even be a good idea to do some profiling to find out which methods really cost performance.
Then, once you have identified these methods, you could try to improve those.

I completely agree. Get my makefile, make a profile DLL and play with that one. While playing, start the app "Very Sleepy" (link in makefile thread) and start it when selecting all threads for colonization. Once you press stop, it will tell how long it has spent in each line of the code.

While it might sound a bit complex when reading this, it really isn't. Go ahead and try and you will figure it out. There is one sideeffect from this though. It will profile non-DLL code, which mean you have to pay attention to the column telling what it profiled. Only the DLL will provide the code meaning the risk of mistakes here is really low.

raystuttgart said:
Personally I believe that the following factors should be in a good relation:
benefits -- efforts -- risk

Hours of work for saving a few mircoseconds each turn and even risk introducing bugs is not necessarily the most efficient modding.

I started improving performance before I profiled and I ended up with no real change. Then I worked hard on changing the makefile to make it possible to profile and then the results were quite good. Trying to improve performance without profiling is not really rewarding.

One good way to profile is to start when you hit end turn and stop when you arrive at next turn. This will tell you which part of the code you were waiting on. This is a lot more interesting than if you use 20% or 25% CPU during your own turn.

One thing which you can improve on without profiling and (presumably) without breaking stuff is inlining of simple functions. Say we have

Code:

int CvCity::getHealRate() const
{
	return m_iHealRate;
}

Delete this one from CvCity.cpp and write in CvCity.h after the class declaration:

Code:

[COLOR="Red"]inline [/COLOR]int CvCity::getHealRate() const
{
	return m_iHealRate;
}

This will make the compiler try to copy in the code wherever it is called and avoid a function call at runtime. It slows down the game if the function is too big though meaning it leaves a large gray area where it is a bit tricky to see if a function should be inlined or not. However this example reveals the most simple case you can imagine, which is just returning a member variable. This will always benefit from inlining. Vanilla has quite a lot of "return member variable" functions you can move if you like.

M:C also has quite a lot of those left, ready for whoever wants to work on this specific task. I thought about doing this a few times, but I always hoping that somebody will show up and do this pretty strait forward task. As a result I work on stuff, which are much less likely to be picked up by other people.

However I do have one question regarding this thread: why would you want to improve on vanilla? What are the odds that a new mod will start today based on vanilla? Don't get me wrong. I think it is fine that for once somebody actually cares for performance. However I wonder if time would be better spend doing the very same in a mod. It doesn't really matter how fast vanilla is if nobody plays vanilla anymore.

Commander Bello · Jan 1, 2014

Nightinggale said:
However I do have one question regarding this thread: why would you want to improve on vanilla? What are the odds that a new mod will start today based on vanilla? Don't get me wrong. I think it is fine that for once somebody actually cares for performance. However I wonder if time would be better spend doing the very same in a mod. It doesn't really matter how fast vanilla is if nobody plays vanilla anymore.

First of all, a happy new year.

Second, thanks for your valuable remarks and hints. :thumbsup:

Third, my idea was that in any mod parts of the vanilla code are left. So, if somebody finds something in vanilla code, chances are good that it may help other modders in other mods, too.
Mod-specific slowdowns can be mentioned in the respective mod threads, of course.

As I am doing my attempts for my own modmod, I come across a lot of vanilla code of which many parts - as I think - can be speeded up a bit or at least can be written in a slightly better way.
All, what I can do at the moment is to mention this code sniplets and maybe propose changes.

But if we all would contribute to this thread, it would finally hold a lot of parts of code, which can be improved. Not only for our own codes in our mods, but for anybody who comes along and does changes in the code.

That's the whole idea

Nightinggale · Jan 2, 2014

New info for modders: (in fact both civ4 and civ4col)
I uploaded a new version of the makefile with a decent list of improvements.

I also wrote a guide on header includes because Firaxis did a decent job at including everything everywhere to ensure as many unneeded recompilations as possible when modifying header files. If you want to only recompile the files, which needs to be recompiled, you better read this guide.

Follow the makefile link in my signature for more info on both.

Commander Bello · May 7, 2014

It's me again

This time I've found something in void CvGame::assignStartingPlots() what I actually don't get.

Code:

		// PatchMod: Random Start Locs START
		// Randomise start locations amongst water starters (Human Europeans)

		int iWaterCount = 0;
		int iCounter = 0;
		[B][COLOR="Red"]for (iI = 0; iI < MAX_PLAYERS; iI++)
		{
			if (GET_PLAYER((PlayerTypes)iI).isAlive())
			{
				if (GC.getCivilizationInfo(GET_PLAYER((PlayerTypes)iI).getCivilizationType()).isWaterStart())[/COLOR][/B]
				{
					iWaterCount++;
				}
			}
		}
		int iRandStart = 0;
		CvPlot* pTempPlot = NULL;
		[B][COLOR="Red"]for (iJ = 0; iJ < MAX_PLAYERS; iJ++)
		{
			if (GET_PLAYER((PlayerTypes)iJ).isAlive())
			{
				if (GC.getCivilizationInfo(GET_PLAYER((PlayerTypes)iJ).getCivilizationType()).isWaterStart()[/COLOR][/B] && GET_PLAYER((PlayerTypes)iJ).isHuman())
				{
					iRandStart = getSorenRandNum(iWaterCount, "");
					iCounter = 0;
					[B][COLOR="Red"]for (iI = 0; iI < MAX_PLAYERS; iI++)
					{
						if (GET_PLAYER((PlayerTypes)iI).isAlive())
						{
							if (GC.getCivilizationInfo(GET_PLAYER((PlayerTypes)iI).getCivilizationType()).isWaterStart())[/COLOR][/B]
							{
								iCounter++;
							}
						}
						if (iCounter == iRandStart)
						{
							pTempPlot = GET_PLAYER((PlayerTypes)iJ).getStartingPlot();
							GET_PLAYER((PlayerTypes)iJ).setStartingPlot(GET_PLAYER((PlayerTypes)iI).getStartingPlot(), true);
							GET_PLAYER((PlayerTypes)iI).setStartingPlot(pTempPlot, true);
							iI = MAX_PLAYERS;
						}
					}
				}
			}

First of all, I don't get the necessity to "randomize" the starting locations, as these should be randomized anyway.

Second, as we see there are three almost identical for-loops iterating over MAX_PLAYERS, which in the case of RaR equals 48, making the total of all iterations not less than "just" ~~110592~~... Edit: I've done the math wrong. It is 48 plus 48², which totals up to 2352 (I hope this time I am right

)

And all this just to switch the initial starting location of the human player against another European?

As I said, I may very well have missed something but so far, I think this is a complete overkill.
Any ideas, why it has been done this way or better, how to simplify things?

The modders' lair: (CIP) Code Improvement Project

Say No 2 Net Validations

Say No 2 Net Validations

Civ4Col Modder

Say No 2 Net Validations

Civ4Col Modder

Say No 2 Net Validations

Civ4Col Modder

Say No 2 Net Validations

Civ4Col Modder

Say No 2 Net Validations

Say No 2 Net Validations

Say No 2 Net Validations

Say No 2 Net Validations

Say No 2 Net Validations

Civ4Col Modder

Say No 2 Net Validations

Deity

Say No 2 Net Validations

Deity

Say No 2 Net Validations

Similar threads