AND SVN Build Thread

alberts2 said:
This change has a big performance impact, i ran 2 turns taking 90seconds and ~30second are used for CvBuildingInfo::isReplaceBuildingClass and CvPlayer::getBuildingClassCountWithUpgrades:eek:!
I wonder why nobody else noticed it because this is alot compared to e.g. the impact from trade routes.
Click to expand...

I am very doubtful of your findings. I am not saying you are wrong, but if I were a betting man, I would be betting against you heavily.

Why am I so dubious? Let's see the code:

Code: 
int CvPlayer::getBuildingClassCountWithUpgrades(BuildingClassTypes eBuildingClass) const
{
	int iCount;
	iCount = getBuildingClassCount(eBuildingClass);
	BuildingTypes eBuilding = (BuildingTypes)GC.getCivilizationInfo(getCivilizationType()).getCivilizationBuildings(eBuildingClass);
	if (eBuilding != NO_BUILDING && GC.getBuildingInfo(eBuilding).isReplaceBuildingClass(NO_BUILDINGCLASS))
	{
		for (int iI = 0; iI < GC.getNumBuildingClassInfos(); iI++)
		{
			if (GC.getBuildingInfo(eBuilding).isReplaceBuildingClass(iI))
			{
				iCount += getBuildingClassCount((BuildingClassTypes)iI);
			}
		}
	}
	return iCount;
}

If the building has no replacements (like 98% of buildings are never replaced) there is zero extra cost in terms of performance. The "GC.getBuildingInfo(eBuilding).isReplaceBuildingClass(NO_BUILDINGCLASS)" check will return false if the building has no replacements.

If it does have replacements, it scans all building classes, then adds the additional counts. getBuildingClassCount is a simple arrray access. There should be nearly no performance hit for this sort of calculation.

There are three possible reasons for your strange results:

1.) You tested the change on C2C and not RAND and didn't know I fixed a bug related to GC.getBuildingInfo(eBuilding).isReplaceBuildingClass(NO_BUILDINGCLASS)) that was causing it to not work as expected.
2.) You tested the change on RAND and your profiling results are wrong (miscalibration, wrong profiling scope?)
3.) You tested the change on RAND and your profiling results are correct, and I am wrong.

I am betting on #1. :p

Spoiler :

The fix I mentioned in #1 was a bug that caused the replacement building array in CvInfos to be assigned to garbage values (when it should have been NULL) when there was no XML tag. The fix is here. It occurs because Koshling (I assume, or someone else in C2C) tried to optimize replacement buildings by not allocating an array when there were no replacements, but didn't actually assign a valid NULL to the pointer in that scenario. InitList(...) was only called when there *was* an XML tag, so the XML tag remained the default uninitialized pointer. For added fun, this would not show up in a debug DLL, as the debug flag causes uninitialized pointers to be set to NULL by default.
 
Afforess said:
I am very doubtful of your findings. I am not saying you are wrong, but if I were a betting man, I would be betting against you heavily.

Why am I so dubious? Let's see the code:

Code: 
int CvPlayer::getBuildingClassCountWithUpgrades(BuildingClassTypes eBuildingClass) const
{
	int iCount;
	iCount = getBuildingClassCount(eBuildingClass);
	BuildingTypes eBuilding = (BuildingTypes)GC.getCivilizationInfo(getCivilizationType()).getCivilizationBuildings(eBuildingClass);
	if (eBuilding != NO_BUILDING && GC.getBuildingInfo(eBuilding).isReplaceBuildingClass(NO_BUILDINGCLASS))
	{
		for (int iI = 0; iI < GC.getNumBuildingClassInfos(); iI++)
		{
			if (GC.getBuildingInfo(eBuilding).isReplaceBuildingClass(iI))
			{
				iCount += getBuildingClassCount((BuildingClassTypes)iI);
			}
		}
	}
	return iCount;
}

If the building has no replacements (like 98% of buildings are never replaced) there is zero extra cost in terms of performance. The "GC.getBuildingInfo(eBuilding).isReplaceBuildingClass(NO_BUILDINGCLASS)" check will return false if the building has no replacements.

If it does have replacements, it scans all building classes, then adds the additional counts. getBuildingClassCount is a simple arrray access. There should be nearly no performance hit for this sort of calculation.

There are three possible reasons for your strange results:

1.) You tested the change on C2C and not RAND and didn't know I fixed a bug related to GC.getBuildingInfo(eBuilding).isReplaceBuildingClass(NO_BUILDINGCLASS)) that was causing it to not work as expected.
2.) You tested the change on RAND and your profiling results are wrong (miscalibration, wrong profiling scope?)
3.) You tested the change on RAND and your profiling results are correct, and I am wrong.

I am betting on #1. :p

Spoiler :

The fix I mentioned in #1 was a bug that caused the replacement building array in CvInfos to be assigned to garbage values (when it should have been NULL) when there was no XML tag. The fix is here. It occurs because Koshling (I assume, or someone else in C2C) tried to optimize replacement buildings by not allocating an array when there were no replacements, but didn't actually assign a valid NULL to the pointer in that scenario. InitList(...) was only called when there *was* an XML tag, so the XML tag remained the default uninitialized pointer. For added fun, this would not show up in a debug DLL, as the debug flag causes uninitialized pointers to be set to NULL by default.
Click to expand...

I did not try this in C2C so #1 is of the table, the truth is somewhere between #2 and #3. In the past my results where good and i don't know if you even use a profiler so i'am betting against you.

  • CvPlayer::getBuildingClassCountWithUpgrades is called to much and takes almost 10 seconds.
  • This results in alot calls to CvBuildingInfo::isReplaceBuildingClass and it takes 22 seconds.
  • cvInternalGlobals::getBuildingInfo is also called to much and takes another 9.4 seconds.


Look at the usages of CvPlayer::getBuildingClassCountWithUpgrades it is called inside of loops in CvPlayer::canConstructInternal. I don't know how many Buildings or BuildingClasses this mod has but in the end it results in far to many calls to CvPlayer::getBuildingClassCountWithUpgrades

I used the AutoSave_AD-2185.CivBeyondSwordSave from here with rev779.
 
alberts2 said:
I did not try this in C2C so #1 is of the table, the truth is somewhere between #2 and #3. In the past my results where good and i don't know if you even use a profiler so i'am betting against you.

  • CvPlayer::getBuildingClassCountWithUpgrades is called to much and takes almost 10 seconds.
  • This results in alot calls to CvBuildingInfo::isReplaceBuildingClass and it takes 22 seconds.
  • cvInternalGlobals::getBuildingInfo is also called to much and takes another 9.4 seconds.


Look at the usages of CvPlayer::getBuildingClassCountWithUpgrades it is called inside of loops in CvPlayer::canConstructInternal. I don't know how many Buildings or BuildingClasses this mod has but in the end it results in far to many calls to CvPlayer::getBuildingClassCountWithUpgrades

I used the AutoSave_AD-2185.CivBeyondSwordSave from here with rev779.
Click to expand...

Let's look at the data here, assuming a worst case canConstructInternal gets called for every building (for easy math, 600, even though RAND has 587 exactly.) possible, for every player possible (for easy math, 50). So right there, 30000 calls to canConstructInternal. Then, lets assume that in this worst case, all the buildings do not return at the the if statements before this block of code:

Code: 
		if ( kBuilding.getPrereqNumOfBuildingClass(NO_BUILDINGCLASS) > 0 )
		{
			for (iI = 0; iI < numBuildingClassInfos; iI++)
			{
				//Afforess count upgraded buildings for the prerequisite
				if (getBuildingClassCountWithUpgrades((BuildingClassTypes)iI) < getBuildingClassPrereqBuilding(eBuilding, ((BuildingClassTypes)iI), ((bContinue) ? 0 : getBuildingClassMaking(eBuildingClass))))
				{
					if ( probabilityEverConstructable != NULL )
					{
						*probabilityEverConstructable = 20;
					}
					return false;
				}
			}
		}
That is where the upgraded buildings are counted. A total of 60 of the 600 buildings (I checked the XML) have a PrereqBuildingClasses XML tag, which will make them pass that first if statement. So now we are down from 30000 calls to 3000. Then, we have an inner loop, and immediately, I see a potential, simple optimization:

Code: 
		if ( kBuilding.getPrereqNumOfBuildingClass(NO_BUILDINGCLASS) > 0 )
		{
			for (iI = 0; iI < numBuildingClassInfos; iI++)
			{
				//Afforess count upgraded buildings for the prerequisite
				int iPrereqs = getBuildingClassPrereqBuilding(eBuilding, (BuildingClassTypes)iI, (bContinue ? 0 : getBuildingClassMaking(eBuildingClass)));
				if (iPrereqs > 0 && getBuildingClassCountWithUpgrades((BuildingClassTypes)iI) < iPrereqs)
				{
					if ( probabilityEverConstructable != NULL )
					{
						*probabilityEverConstructable = 20;
					}
					return false;
				}
			}
		}

Now, the building class upgrade count is only checked when there are any prereqs. So 3000 * 600 for the inner loop, and then 1 in 6 of those will have another inner loop of 600. So 3000 * 600 * 100. That does seem like a lot, and I think I see another easy optimization:

Code: 
		if ( kBuilding.getPrereqNumOfBuildingClass(NO_BUILDINGCLASS) > 0 )
		{
			for (iI = 0; iI < numBuildingClassInfos; iI++)
			{
				//Afforess count upgraded buildings for the prerequisite
				int iPrereqs = getBuildingClassPrereqBuilding(eBuilding, (BuildingClassTypes)iI, (bContinue ? 0 : getBuildingClassMaking(eBuildingClass)));
				if (iPrereqs > 0 && getBuildingClassCount((BuildingClassTypes)iI) < iPrereqs && getBuildingClassCountWithUpgrades((BuildingClassTypes)iI) < iPrereqs)
				{
					if ( probabilityEverConstructable != NULL )
					{
						*probabilityEverConstructable = 20;
					}
					return false;
				}
			}
		}

Now it checks the getBuildingClassCount (which is the old code) first, and if it there are enough of those without counting the upgrades, there is no point counting the upgrades. The upgraded building is now only counted when there are too few non-upgraded building classes to satisfy the prerequisite. So what, now down to 60 * 50 * 60 * 100?

Always good to have a fellow complainer poking holes at my code. Its all to easy to get sloppy. :p
 
All that's way over my head, but still so fascinating to read. I sort of envy people that know how to code things (Ruby, Python, C#, etc) but then when I think about how much work goes into debugging and tracking down seemingly random things... Sometimes I don't xD

Still is very impressive though.
 
Afforess said:
Let's look at the data here, assuming a worst case canConstructInternal gets called for every building (for easy math, 600, even though RAND has 587 exactly.) possible, for every player possible (for easy math, 50). So right there, 30000 calls to canConstructInternal. Then, lets assume that in this worst case, all the buildings do not return at the the if statements before this block of code:

Code: 
		if ( kBuilding.getPrereqNumOfBuildingClass(NO_BUILDINGCLASS) > 0 )
		{
			for (iI = 0; iI < numBuildingClassInfos; iI++)
			{
				//Afforess count upgraded buildings for the prerequisite
				if (getBuildingClassCountWithUpgrades((BuildingClassTypes)iI) < getBuildingClassPrereqBuilding(eBuilding, ((BuildingClassTypes)iI), ((bContinue) ? 0 : getBuildingClassMaking(eBuildingClass))))
				{
					if ( probabilityEverConstructable != NULL )
					{
						*probabilityEverConstructable = 20;
					}
					return false;
				}
			}
		}
That is where the upgraded buildings are counted. A total of 60 of the 600 buildings (I checked the XML) have a PrereqBuildingClasses XML tag, which will make them pass that first if statement. So now we are down from 30000 calls to 3000. Then, we have an inner loop, and immediately, I see a potential, simple optimization:

Code: 
		if ( kBuilding.getPrereqNumOfBuildingClass(NO_BUILDINGCLASS) > 0 )
		{
			for (iI = 0; iI < numBuildingClassInfos; iI++)
			{
				//Afforess count upgraded buildings for the prerequisite
				int iPrereqs = getBuildingClassPrereqBuilding(eBuilding, (BuildingClassTypes)iI, (bContinue ? 0 : getBuildingClassMaking(eBuildingClass)));
				if (iPrereqs > 0 && getBuildingClassCountWithUpgrades((BuildingClassTypes)iI) < iPrereqs)
				{
					if ( probabilityEverConstructable != NULL )
					{
						*probabilityEverConstructable = 20;
					}
					return false;
				}
			}
		}

Now, the building class upgrade count is only checked when there are any prereqs. So 3000 * 600 for the inner loop, and then 1 in 6 of those will have another inner loop of 600. So 3000 * 600 * 100. That does seem like a lot, and I think I see another easy optimization:

Code: 
		if ( kBuilding.getPrereqNumOfBuildingClass(NO_BUILDINGCLASS) > 0 )
		{
			for (iI = 0; iI < numBuildingClassInfos; iI++)
			{
				//Afforess count upgraded buildings for the prerequisite
				int iPrereqs = getBuildingClassPrereqBuilding(eBuilding, (BuildingClassTypes)iI, (bContinue ? 0 : getBuildingClassMaking(eBuildingClass)));
				if (iPrereqs > 0 && getBuildingClassCount((BuildingClassTypes)iI) < iPrereqs && getBuildingClassCountWithUpgrades((BuildingClassTypes)iI) < iPrereqs)
				{
					if ( probabilityEverConstructable != NULL )
					{
						*probabilityEverConstructable = 20;
					}
					return false;
				}
			}
		}

Now it checks the getBuildingClassCount (which is the old code) first, and if it there are enough of those without counting the upgrades, there is no point counting the upgrades. The upgraded building is now only counted when there are too few non-upgraded building classes to satisfy the prerequisite. So what, now down to 60 * 50 * 60 * 100?

Always good to have a fellow complainer poking holes at my code. Its all to easy to get sloppy. :p
Click to expand...

Thats much better:goodjob:
This mod has only 600 Buildings, compared to the ~3100 in C2C this is nothing. Now imagine the same code used there:eek:

Here is a better version of CvPlayer::getBuildingClassCountWithUpgrades
Code: 
int CvPlayer::getBuildingClassCountWithUpgrades(BuildingClassTypes eBuildingClass) const
{
	int iCount;
	iCount = getBuildingClassCount(eBuildingClass);
	BuildingTypes eBuilding = (BuildingTypes)GC.getCivilizationInfo(getCivilizationType()).getCivilizationBuildings(eBuildingClass);
	if (eBuilding != NO_BUILDING)
	{
		CvBuildingInfo& kBuilding = GC.getBuildingInfo(eBuilding);
		if (kBuilding.isReplaceBuildingClass(NO_BUILDINGCLASS))
		{
			int numNumBuildingClassInfos = GC.getNumBuildingClassInfos();
			for (int iI = 0; iI < numNumBuildingClassInfos; iI++)
					{
						if (kBuilding.isReplaceBuildingClass(iI))
						{
							iCount += getBuildingClassCount((BuildingClassTypes)iI);
						}
					}
		}
	}
	return iCount;
}

Now here are the results for those two turns
  1. We started with 92 seconds
  2. After your changes it drops to 63 seconds
  3. With my changes we are down to 52 seconds
 
From the Bleacher section: Go alberts2 go! :clap::yeah:

@Afforess,
If alberts2 stays around and wants to help, you will find him very helpful. I give his work 2 :thumbsup: ! And a resounding endorsement just like Vokarya.

JosEPh :)
 
alberts2 said:
Now here are the results for those two turns
  1. We started with 92 seconds
  2. After your changes it drops to 63 seconds
  3. With my changes we are down to 52 seconds
Click to expand...

Looks good. Out of that time, how much of it is now related to canConstruct?
 
From the 52 seconds turn time 14 seconds come from CvCity::canConstruct.
From those 14 seconds CvPlayer::getBuildingClassCountWithUpgrades uses 8.5 seconds.
From those 8.5 seconds 6 go to CvBuildingInfo::isReplaceBuildingClass.
 
Revision 780

  • Remove Reeducation center, Checkpoint buildings
  • Try to re-assign population when city shrinks to avoid turning off employed buildings
  • Update connectedness when plundered
  • Give 33% of foreign connectedness commerce as plunder gold, 20% of domestic connectedness
  • Remove recalled ambassador penalty when AI ceases relations with you
  • Remove unused variables in CvPlot, should slightly reduce memory footprint
  • Performance improvements with counting replaced buildings for prerequisites
  • Turn on random number generator verbose logging in DEBUG dlls
  • Lay groundwork for research cost scaling based on map yields (unfinished and de-activated at present)
 
Afforess said:
Revision 780

  • Remove Reeducation center, Checkpoint buildings
  • Try to re-assign population when city shrinks to avoid turning off employed buildings
  • Update connectedness when plundered
  • Give 33% of foreign connectedness commerce as plunder gold, 20% of domestic connectedness
  • Remove recalled ambassador penalty when AI ceases relations with you
  • Remove unused variables in CvPlot, should slightly reduce memory footprint
  • Performance improvements with counting replaced buildings for prerequisites
  • Turn on random number generator verbose logging in DEBUG dlls
  • Lay groundwork for research cost scaling based on map yields (unfinished and de-activated at present)
Click to expand...


Aahhhh nuts... I was afraid that was going to happen :sad:
Being a big fan of Espionage (And how ridiculously expensive it is to force Civic changes, this is especially painful) anything that adds to your espionage production is a bonus to me, and having two major providers of it being taken out is gonna hurt quite a bit :lol:

Really, the only thing keeping me afloat in this current game I'm in is espionage. Isabella is a full generation of military ahead of me, perhaps two - and if it weren't for Tech Stealing she'd have been a full era above me. She switched into Secular of all things and relations dropped to Cautious from Friendly. She's running around with Tanks and Marines, I'm still using Rifles (Only just now got to Infantry) She *refuses* to switch out of Secular no matter what now, and it took the Facism tech to get her out of it previously. To force her out of it via espionage, I could steal three techs or a border city - and she'd just switch back a few turns later. Hardly worth it in my opinion.

I have no idea why civic changes are more expensive than stealing multiple Modern-era techs. Religion swaps I can understand, but the AI can just switch back to their old civic mere turns later and don't really seem to suffer from the forced change at all from what I've seen.

The other revision changes are great though, but it's disappointing to see these two buildings get taken out :(
I guess this also means that Junta is now an entirely useless civic, since the Military Support and its former Civic Building are now gone, and it doesn't provide Happiness to the Garrison or Army Base :sad:
 
Afforess said:
The fix I mentioned in #1 was a bug that caused the replacement building array in CvInfos to be assigned to garbage values (when it should have been NULL) when there was no XML tag. The fix is here. It occurs because Koshling (I assume, or someone else in C2C) tried to optimize replacement buildings by not allocating an array when there were no replacements, but didn't actually assign a valid NULL to the pointer in that scenario. InitList(...) was only called when there *was* an XML tag, so the XML tag remained the default uninitialized pointer. For added fun, this would not show up in a debug DLL, as the debug flag causes uninitialized pointers to be set to NULL by default.
Click to expand...

It looks to me that those pointers are initialized to NULL in the e.g. CvBuildingInfo constructor. That should set them to NULL, if it doesn't maybe they should be initialized in the constructor body insted of the initializer list.
 
alberts2 said:
It looks to me that those pointers are initialized to NULL in the e.g. CvBuildingInfo constructor. That should set them to NULL, if it doesn't maybe they should be initialized in the constructor body insted of the initializer list.
Click to expand...

I'd need to go check the code again but I was seeing mini dumps where those arrays were set to garbage non null values on buildings I knew had no replacements (like building 0...the palace IIRC). Accessing the pointers cause the game to throw an exception for trying to read outside the memory.

Since that revision I have not seen that crash again.

Since you are here, I noticed a CvPlot had quite a few unused variables on it. I removed a few of them for a tiny, but noticeable memory savings. I've thought that I could move the 3 landmark variables into a struct and store just a pointer to that, instead of the 2 pointers + 1 int it uses now. That would cut down memory by 8 bytes for most tiles.

A larger project might also work for units. Most of the fields on units are actually for promotions and could be made into a separate struct - saving quite a bit, but I am not sure there are ever enough units to make the amount of effort worthwhile.

Thoughts?
 
Afforess said:
I'd need to go check the code again but I was seeing mini dumps where those arrays were set to garbage non null values on buildings I knew had no replacements (like building 0...the palace IIRC). Accessing the pointers cause the game to throw an exception for trying to read outside the memory.

Since that revision I have not seen that crash again.

Since you are here, I noticed a CvPlot had quite a few unused variables on it. I removed a few of them for a tiny, but noticeable memory savings. I've thought that I could move the 3 landmark variables into a struct and store just a pointer to that, instead of the 2 pointers + 1 int it uses now. That would cut down memory by 8 bytes for most tiles.

A larger project might also work for units. Most of the fields on units are actually for promotions and could be made into a separate struct - saving quite a bit, but I am not sure there are ever enough units to make the amount of effort worthwhile.

Thoughts?
Click to expand...

I don't know how the memory situation is in AND, i just know that in C2C it's easy to get MAF's or CTD's because of to much used memory. There are alot places to cut down memory but i never had time to look into this, but your idea to reduce the CvPlot memory usage sounds good.
 
alberts2 said:
I don't know how the memory situation is in AND, i just know that in C2C it's easy to get MAF's or CTD's because of to much used memory. There are alot places to cut down memory but i never had time to look into this, but your idea to reduce the CvPlot memory usage sounds good.
Click to expand...

The memory situation is acceptable in RAND. Players with weaker computers will struggle with large or bigger maps but otherwise the memory footprint is maybe only 1.5-2x BTS.

However that does not mean it could not be better.

Other than CvPlot, do you happen to know where the other large memory allocations are? I've never really done any analysis of it.
 
Afforess said:
The memory situation is acceptable in RAND. Players with weaker computers will struggle with large or bigger maps but otherwise the memory footprint is maybe only 1.5-2x BTS.

However that does not mean it could not be better.

Other than CvPlot, do you happen to know where the other large memory allocations are? I've never really done any analysis of it.
Click to expand...

Ya at a minimum saving memory in one place you can use it somewhere else.
 
You must log in or register to reply here.

Similar threads

Thunderbrd
Categories Project Thread for Modders and Players
Replies
10
Views
2K
Thunderbrd
Thunderbrd
Browd
  • Locked
Ryika v. leif erickson
Replies
9
Views
9K
Browd
Browd
Askthepizzaguy
Conservative politics and assumption
8 9 10
Replies
194
Views
19K
Mouthwash
Mouthwash
45°38'N-13°47'E
  • Sticky
AND2 and SVN Bug Reports - A New Dawn 2 ONLY
372 373 374
Replies
7K
Views
839K
Gameboy2396
Gameboy2396
S
To the Developers, and Everyone Else!
Replies
3
Views
3K
Sturmir
S
Back
Top Bottom