BIG perormance isue in some recent changes

[Koshling]

Big performance issue in new code.

I just did a profile run to get a fresh baseline with the recent changes from various people, before I start more work on the city processing pipeline.

CvUnit::checkPromotionObsoletion() has gone from 3 seconds in the test turn I use to over 70 seconds, so the recent promotion checking changes are horrible from a performance standpoint.

I'll take a look at what changed and see what I can do...

 

[Thunderbrd]

I haven't recently made any adjustments to that. Well...I suppose recently is a relative term... I believe I did some debugging there a little while back but it's already long enough back I can't recall exactly what was done. I'm sure there are some places that could be improved on in that section - but are the two samples you mention even taken before and after those debuggings? Your first sample would need to have been ages ago!

I don't THINK it checks canAcquirePromotion, which would be where there were some 'code cleanups' attempted (not by me) and required a small fix. But in looking through those changes I'd think they would only speed things up rather than slow anything down...

 

[Koshling]

I haven't recently made any adjustments to that. Well...I suppose recently is a relative term... I believe I did some debugging there a little while back but it's already long enough back I can't recall exactly what was done. I'm sure there are some places that could be improved on in that section - but are the two samples you mention even taken before and after those debuggings? Your first sample would need to have been ages ago!

I don't THINK it checks canAcquirePromotion, which would be where there were some 'code cleanups' attempted (not by me) and required a small fix. But in looking through those changes I'd think they would only speed things up rather than slow anything down...

It's all spent in canAcquirePromotion() as it turns out. However, I think it's a red herring and not actually a problem. I think it's just an artifact of the fact that I am using quite an old save (Talin's 6 minute turn from a couple months ago) as my benchmarking test.

Because it's fairly old the promotion prereqs have changed quite a bit since it was saved (and fixes to the prereq checks have changed the results of them). This means that LOTS of units in the game have invalid promotions, so when it processes the turn ir strips them and lets them re-promote. This results in 375,000 calls to CvUnit::setHasPromotion() when processing the turn, and that accounts for the extra 70 or so seconds. This is a one-off because it's effectively bringing the save up-to-date with the asset changes.

I'm just verifying this theory now, by playing one turn and then resaving (I'll use the result as my benchmark from now on), to take the one-off conversion factors out of the equation. Assuming this verifies the theory there really isn't an issue (phew - false alarm), and I'll have a new save to use for benchmarking going forward.

HOWEVER, while reading through the code I did spot this, which looks to me like a bug:

	if (!(GC.getPromotionInfo(ePromotion).isAffliction()))
	{
		if (GC.getPromotionInfo(ePromotion).getTechPrereq() != NO_TECH)
		{
			if (!(GET_TEAM(getTeam()).isHasTech((TechTypes)(GC.getPromotionInfo(ePromotion).getTechPrereq()))))
			{
				return false;
			}
			if (GC.getPromotionInfo(ePromotion).getPromotionLine() != NO_PROMOTIONLINE)
			{
				if (GC.getPromotionLineInfo((PromotionLineTypes)GC.getPromotionInfo(ePromotion).getPromotionLine()).getPrereqTech() != NO_TECH)
				{
					if (!(GET_TEAM(getTeam()).isHasTech((TechTypes)(GC.getPromotionLineInfo((PromotionLineTypes)GC.getPromotionInfo(ePromotion).getPromotionLine()).getPrereqTech()))))
					{
						return false;
					}
				}
			}
		}
	}

Note that it only tests the promotion LINE tech prereqs if the promotion itself has its own tech prereq. This looks wrong to me - I think I should be:

	if (!(GC.getPromotionInfo(ePromotion).isAffliction()))
	{
		if (GC.getPromotionInfo(ePromotion).getTechPrereq() != NO_TECH)
		{
			if (!(GET_TEAM(getTeam()).isHasTech((TechTypes)(GC.getPromotionInfo(ePromotion).getTechPrereq()))))
			{
				return false;
			}
 		}
		if (GC.getPromotionInfo(ePromotion).getPromotionLine() != NO_PROMOTIONLINE)
		{
			if (GC.getPromotionLineInfo((PromotionLineTypes)GC.getPromotionInfo(ePromotion).getPromotionLine()).getPrereqTech() != NO_TECH)
			{
				if (!(GET_TEAM(getTeam()).isHasTech((TechTypes)(GC.getPromotionLineInfo((PromotionLineTypes)GC.getPromotionInfo(ePromotion).getPromotionLine()).getPrereqTech()))))
				{
					return false;
				}
			}
		}
	}

What do you think...?

 

[Thunderbrd]

You're absolutely right on that. Thanks for the spot!

Your evaluation of the 'false alarm' sounds about right to me. It won't run through so much if its not finding so much to update.

 

[Koshling]

You're absolutely right on that. Thanks for the spot!

Your evaluation of the 'false alarm' sounds about right to me. It won't run through so much if its not finding so much to update.

Yup, confirmed - it was a false alarm.