Battlecalc is Noticeably Slower In New Stable Compared to 1.9.0.0.3635

[ron-murhammer]

Pretty much start any new game or open a save then try using the battle calc. Its about 2-3 times slower in the new stable then 1.9.0.0.3635.

[DanVanAtta]

Can confirm about a 3x increase in some quick experimentation.

---

(0.56s -> 1.2s)

3635:

latest:

battle.zip

---

[prastle]

Yup and calcs 4 threads in tww he was using old stable i am on new

[prastle]

Also to clarify that was with a cell connection at 75 mbs not my normal redneck speed ;)

[ron-murhammer]

Yeah, it has nothing to do with connection or lobby games as its slower in just regular local games.

[DanVanAtta]

@ron-murhammer I'm curious more about the 10x difference, I'm wondering what may be different as I'm not yet able to repro the drastic performance difference.

[ron-murhammer]

@DanVanAtta It appears my run counts were different as well. I see more around what your test result is now that I adjusted that to be equal (2-3x longer).

[ssoloff]

I tried bisecting to which build the problem first appeared. Let me say first that there appears to be two separate performance degradations. The first one occurred sometime between 3635 and 4684, and accounts for about a 25% increase in calc time. Since everyone seems to be observing a 2-3x increase between 7062 and 3635, I didn't pursue that further.

The second increase adds about another 75% (i.e. I'm consistently observing a 2x increase between 7062 and 3635). I bisected that down to sometime between 6590 and 6597. There aren't too many commits during that time:

$ git lg 1.9.0.0.6590..1.9.0.0.6597
*   ef4b7dc (tag: 1.9.0.0.6597) Merge pull request #2362 from triplea-game/Fix_Compile_Errors
|\  
| * dc6c928 Fix compile errors from refactoring
|/  
*   b1112f6 Merge pull request #2360 from ssoloff/checkstyle-fix-rval-local-variable-name-violations-part-3
|\  
| * db5e4fb Checkstyle: Fix local variable name violations
*   5204ba2 Merge pull request #2080 from simon33-2/miniRemovePrompts
|\  
| * dabfd9c Refactoring upgrade
| * b1bee6c Fix up and reinstate #1646 to not remove infrastructure units
* 0e138ad (tag: 1.9.0.0.6591) Bot: Update Checkstyle thresholds after build 6590

Maybe focus on those commits first, and if nothing obvious pops out, time to fire up the profiler. Hopefully my bisecting-fu is not a red herring. 😄

[ssoloff]

It appears the MustFightBattle#getBattlePower() method added in #2080 is getting pounded during a battle calc. I confirmed that if I remove the code added in #2080, I get back to 6590 times (well, within 10% of 6590 times; that might be due to me running this latest test from the IDE, whereas all the other tests I ran from an installed distro). This method might be a candidate for optimization.

[prastle]

Would that explain a 1.5 min batle calc? In lobby? In Tww?

[prastle]

you guys are talking seconds
I am talking huge

[ron-murhammer]

@prastle You might want to check your run count. It appeared to reset to 5000 for me in the new stable.

[ron-murhammer]

@ssoloff Hmm. I think we should just revert that commit (again) for now.

But yeah, in general the battle calc and MustFightBattle could use some optimization (helps the AI as well).

[prastle]

I am using new stable
And you were in room
I cancelled the battle calc

Think we need a lobby test from someone other than me

[ssoloff]

@ron-murhammer Can you confirm that you observe a similar improvement by commenting out that code? Just want to make sure I'm not the outlier here.

[prastle]

4 peeps in lobby hosted room Tww vs Hepps

Ran a battle calc on a small battle

Locked

no result

they were able to keep chatting

finally cancelled the battle calc min half later no result

Hepps using old stable me using new

nidea what ron or other gent were using

end of story never did get my calc result

[prastle]

Also would like to add this
which is actually important
I could read their chats while they waited for me in my own host but could not respond until I hit cancel

[ron-murhammer]

@ssoloff Yeah, that seems to get us back to close to the performance of .3635.

After commenting it out, I only see about 33% increase from .3635 to latest (1.5s -> 2s).

[ssoloff]

@prastle If you can reproduce the long calc time consistently, can you post a screenshot of your battle calc window so we can try to run it locally with the same parameters?

[prastle]

join lobby we test
take a copy yourself
ron was there

[prastle]

Rons still there we can test

[ron-murhammer]

@ssoloff It appears to be that the new default LL run count is 5000 and I think the settings refactor reset everyone. That should probably be changed to more like 500.

[ssoloff]

@ron-murhammer Yup, confirmed the old default was 500:

71bcf7a#diff-9b89a4a639024ac31252e44ba37d2967L8

Probably just a fat finger during the settings migration. I'll submit a PR for that separately.

[ron-murhammer]

@ssoloff Yeah, I figured that was where the mistake happened. We should probably make dice default to something like 200.

I'm gonna call it a night but can either make the changes tomorrow or review any PRs you put up.

[prastle]

Yup

[prastle]

Good stuff guys we getting closer :)

[ssoloff]

#2531 reduces the dice and LL run counts to 200 and 500, respectively.

I'm going offline soon, so I'll just tag @simon33-2 to possibly take a look at the #2080 changes to see if there are any obvious optimizations that can be applied before it gets reverted.

[RoiEXLab]

Closing per #2531

[ssoloff]

Going to leave this open until the performance issues associated with #2080 are resolved.

[simon33-2]

#2080 only changed MustFightBattle. Does the odds calculator use that? I guess it might.

It seems clear that such a change would slow things down a little, repeated 2000 times it could become significant. I don't see an agreeable change though. Perhaps have a flag for "Are we battle calculator"?

[ssoloff]

Does the odds calculator use that?

Indirectly, yes.

[ron-murhammer]

@simon33-2 Yeah MustFightBattle is a very core class to the engine since its used by regular battles, battle calc, and AI. So performance of it is pretty critical so we need to either revert that change or optimize it to have a minimal impact on performance.

If you have ideas to optimize it and have time the next day or so I'm open to that otherwise I'd like to revert it for now and reopen an issue to track re-implementing it so we can push a new version out to resolve the battle calc performance issues.

[simon33-2]

Unless you like the idea of having a flag "are we battleCalculator", I can't see a solution that is all things to all people here. I still think that it would have been adequate and still would be adequate to just look at the Raw Attack and defense values as I originally coded it, which would have been somewhat faster than the solution adopted.

There's a couple of suggestions but I don't think you're going to like either.

[ron-murhammer]

@simon33-2 Yeah, neither of those solutions works well. I think I'd prefer to revert the change until we have more time to think about how it can be optimized to avoid impacting battle calc and AI performance.

[DanVanAtta]

@ron-murhammer any examples of how to repro the 10x difference? Again, I can see a 2x or 3x, but not 10x. There is likely something different going on for the calculator to be yet that much slower.

[prastle]

@DanVanAtta Try it with TWW. That was where we noticed it the most.

[ssoloff]

The performance fixes are available in 1.9.0.0.7309 and later, if anyone wants to give them a spin. However, based on developer testing, you should still expect a performance degradation of anywhere between 25-50% from 1.9.0.0.3635.

(Note that if you overrode the dice or LL simulation counts in Engine Settings, you'll have to Reset To Default to pick up the new values.)

[ron-murhammer]

@ssoloff Yeah, seems reasonable now after reverting that. I think we should look to test the latest and update the download in the next few days.

[simon33-2]

I think I'd prefer to revert the change until we have more time to think about how it can be optimized to avoid impacting battle calc and AI performance.

Is there a reasonable way to just remember the previous calculations done in #2080 rather than do them again 2000 times? Seems like it ought to work on paper, although looking at the code I think it would require an incompatible release.

[ssoloff]

@simon33-2 That's actually a really good idea. There's probably plenty of code the battle calculator runs that is independent of dice rolls and only dependent on the state of the map. However, separating that stuff out so that it can be cached between runs sounds like a huge task.

[simon33-2]

You would be limited to remembering only the unchanging data like the UnitBattleComparator because m_attackingUnits and m_defendingUnits can actually change with each call. Perhaps it might not get enough of an improvement.

[ron-murhammer]

@simon33-2 @ssoloff Actually most of the heavy lifting is around things like casualty selection which is dependent on remaining attacker/defending units that obviously change based on rolls and previous battle rounds. I believe it already does some caching around casualty selection.

[ssoloff]

Closing as no one has reported any further issues in the eight days since the fixes were merged.