yairm210
commented
2 years ago Closed yairm210 closed 2 years ago
yairm210
commented
2 years ago 
itanasi
commented
2 years ago Haven't been able to replicate. My best guess based on the dump is something goes wrong in the sort step. Float math run amok? But I had tested previously both discounting the unit and having a global discount for type of unit combined.
yairm210
commented
2 years ago I also couldn't find the cause I'm keeping this on, hopefully someone who encountered this will send us their game data
SomeTroglodyte
commented
2 years ago Sounds like the "Comparison method violates its general contract" thingy - we had an example in our code a few months back I think. That IllegalArgumentException had no message to accompany it? The call stack is largely identical to search hits for that string I quoted.
Bottom line, comparing A with B must be consistent with comparing B with A and the comparison results must not change during the sort. Java 6 was more permissive allowing a kind of "don't care" within a sort.
If that's the root cause, then it may well be a float idiosyncrasy. If so, converting to some fixed-point notation should fix it, like sorting by (it.maintenance * 1000000.0).toLong()
itanasi
commented
2 years ago Stack Overflow of the dump seems to be mostly people overloading the comparison function though. For example https://stackoverflow.com/questions/11441666/java-error-comparison-method-violates-its-general-contract Worth a shot using fixed point though. Floats have bit me in the arse plenty of times.
will-ca
commented
2 years ago Still happening, it seems, in a branch based on the latest master (and no changes to the functions in the stack trace AFAIK).
…Actually, wait. There's a StackOverflowError the automated movement thread. Is that a different issue?
Exception in thread "Construction info gathering - Eregli" java.lang.IllegalArgumentException: Comparison method violates its general contract!
at java.base/java.util.TimSort.mergeHi(TimSort.java:903)
at java.base/java.util.TimSort.mergeAt(TimSort.java:520)
at java.base/java.util.TimSort.mergeCollapse(TimSort.java:448)
at java.base/java.util.TimSort.sort(TimSort.java:245)
at java.base/java.util.Arrays.sort(Arrays.java:1441)
at kotlin.collections.ArraysKt___ArraysJvmKt.sortWith(_ArraysJvm.kt:2557)
at kotlin.collections.CollectionsKt___CollectionsKt.sortedWith(_Collections.kt:1073)
at com.unciv.logic.civilization.CivInfoStats.getUnitMaintenance(CivInfoStats.kt:348)
at com.unciv.logic.civilization.CivInfoStats.getStatMapForNextTurn(CivInfoStats.kt:173)
at com.unciv.logic.civilization.CivilizationInfo.updateStatsForNextTurn(CivilizationInfo.kt:309)
at com.unciv.logic.city.CityStats.update(CityStats.kt:513)
at com.unciv.logic.city.CityConstructions.turnsToConstruction(CityConstructions.kt:298)
at com.unciv.logic.city.CityConstructions.getTurnsToConstructionString$core(CityConstructions.kt:181)
at com.unciv.ui.cityscreen.CityConstructionsTable.getConstructionButtonDTOs(CityConstructionsTable.kt:168)
at com.unciv.ui.cityscreen.CityConstructionsTable.access$getConstructionButtonDTOs(CityConstructionsTable.kt:30)
at com.unciv.ui.cityscreen.CityConstructionsTable$updateAvailableConstructions$1.invoke(CityConstructionsTable.kt:208)
at com.unciv.ui.cityscreen.CityConstructionsTable$updateAvailableConstructions$1.invoke(CityConstructionsTable.kt:206)
at kotlin.concurrent.ThreadsKt$thread$thread$1.run(Thread.kt:30)
Exception in thread "Construction info gathering - Marmaris" java.lang.IllegalArgumentException: Comparison method violates its general contract!
at java.base/java.util.TimSort.mergeLo(TimSort.java:781)
at java.base/java.util.TimSort.mergeAt(TimSort.java:518)
at java.base/java.util.TimSort.mergeCollapse(TimSort.java:448)
at java.base/java.util.TimSort.sort(TimSort.java:245)
at java.base/java.util.Arrays.sort(Arrays.java:1441)
at kotlin.collections.ArraysKt___ArraysJvmKt.sortWith(_ArraysJvm.kt:2557)
at kotlin.collections.CollectionsKt___CollectionsKt.sortedWith(_Collections.kt:1073)
at com.unciv.logic.civilization.CivInfoStats.getUnitMaintenance(CivInfoStats.kt:348)
at com.unciv.logic.civilization.CivInfoStats.getStatMapForNextTurn(CivInfoStats.kt:173)
at com.unciv.logic.civilization.CivilizationInfo.updateStatsForNextTurn(CivilizationInfo.kt:309)
at com.unciv.logic.city.CityStats.update(CityStats.kt:513)
at com.unciv.logic.city.CityConstructions.turnsToConstruction(CityConstructions.kt:298)
at com.unciv.logic.city.CityConstructions.getTurnsToConstructionString$core(CityConstructions.kt:181)
at com.unciv.ui.cityscreen.CityConstructionsTable.getConstructionButtonDTOs(CityConstructionsTable.kt:168)
at com.unciv.ui.cityscreen.CityConstructionsTable.access$getConstructionButtonDTOs(CityConstructionsTable.kt:30)
at com.unciv.ui.cityscreen.CityConstructionsTable$updateAvailableConstructions$1.invoke(CityConstructionsTable.kt:208)
at com.unciv.ui.cityscreen.CityConstructionsTable$updateAvailableConstructions$1.invoke(CityConstructionsTable.kt:206)
at kotlin.concurrent.ThreadsKt$thread$thread$1.run(Thread.kt:30)
Exception in thread "Move automated units" java.lang.StackOverflowError
at kotlin.Result$Failure.<init>(Result.kt)
at kotlin.ResultKt.createFailure(Result.kt:122)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:37)
at kotlin.sequences.SequenceBuilderIterator.hasNext(SequenceBuilder.kt:140)
at kotlin.sequences.TransformingSequence$iterator$1.hasNext(Sequences.kt:214)
at kotlin.sequences.SequencesKt___SequencesKt.toCollection(_Sequences.kt:786)
at kotlin.sequences.SequencesKt___SequencesKt.toMutableList(_Sequences.kt:816)
at kotlin.sequences.SequencesKt___SequencesKt.toList(_Sequences.kt:807)
at com.unciv.logic.civilization.CivInfoTransientUpdater.updateViewableInvisibleTiles(CivInfoTransientUpdater.kt:55)
at com.unciv.logic.civilization.CivInfoTransientUpdater.updateViewableTiles(CivInfoTransientUpdater.kt:15)
at com.unciv.logic.civilization.CivilizationInfo.updateViewableTiles(CivilizationInfo.kt:799)
at com.unciv.logic.map.MapUnit.updateVisibleTiles(MapUnit.kt:410)
at com.unciv.logic.map.MapUnit.moveThroughTile(MapUnit.kt:879)
at com.unciv.logic.map.MapUnit.putInTile(MapUnit.kt:892)
at com.unciv.logic.map.UnitMovementAlgorithms.moveToTile(UnitMovementAlgorithms.kt:493)
at com.unciv.logic.map.UnitMovementAlgorithms.moveToTile$default(UnitMovementAlgorithms.kt:403)
at com.unciv.logic.map.UnitMovementAlgorithms.headTowards(UnitMovementAlgorithms.kt:290)
at com.unciv.logic.automation.WorkerAutomation.automateWorkerAction(WorkerAutomation.kt:167)
at com.unciv.logic.automation.WorkerAutomation$Companion.automateWorkerAction(WorkerAutomation.kt:105)
at com.unciv.logic.map.MapUnit.doAction(MapUnit.kt:612)
at com.unciv.logic.automation.WorkerAutomation.automateWorkerAction(WorkerAutomation.kt:168)
at com.unciv.logic.automation.WorkerAutomation$Companion.automateWorkerAction(WorkerAutomation.kt:105)
at com.unciv.logic.map.MapUnit.doAction(MapUnit.kt:612)
at com.unciv.logic.automation.WorkerAutomation.automateWorkerAction(WorkerAutomation.kt:168)
at com.unciv.logic.automation.WorkerAutomation$Companion.automateWorkerAction(WorkerAutomation.kt:105)
at com.unciv.logic.map.MapUnit.doAction(MapUnit.kt:612)
at com.unciv.logic.automation.WorkerAutomation.automateWorkerAction(WorkerAutomation.kt:168)
at com.unciv.logic.automation.WorkerAutomation$Companion.automateWorkerAction(WorkerAutomation.kt:105)Last lines repeat maybe a hundred times.
yairm210
commented
2 years ago Do you have a saved game with this problem?!
will-ca
commented
2 years ago Do you have a saved game with this problem?!
I have a save from the turn it happened... But it's a massive game with probably close to a hundred units that all need orders, and I didn't end up reproducing the crash when I tried to play the turn again, unfortunately.
EDIT: Though IIRC I was trying to test guided missiles (for attack arrows). So maybe that's a prerequisite for the crash happening? (Makes sense I think as that's what was changed recently?)
itanasi
commented
2 years ago I'd say still post it and see if someone else can reproduce
will-ca
commented
2 years ago May have figured out the problem, actually.
CivInfoStats.getUnitMaintenance() both mutates MapUnit().maintenance and does the sort and sum.
But getUnitMaintenance() is called by getStatMapForNextTurn(), which is called by updateStatsForNextTurn()— Which seems to mean that getUnitMaintenance() can be called from basically anywhere, at any time, in any number of threads.
So there's a sort function seemingly dependent on a mutable property that can change at any time.
Looking at the Kotlin sources, it looks like the lambda passed to sort functions is probably evaluated once for both operands on each comparison, not just once per element— Which means that the order may change and sort may fail if it depends on a var that's mutated mid-sort.
Such a timing-dependent failure would explain why this crash hasn't been reliably reproduced yet.
Originally, I wanted to try running the sort on immutable primitives (raw Floats) instead of on MapUnits with mutable properties, to see if that would help. But if concurrent mutation is indeed happening, then there are probably also incorrect results occasionally being produced even when it doesn't crash.
Possible Solutions:
MapUnit().maintenance will only ever be mutated synchronously. setTransients()? updateTransients()?MapUnit().maintenance completely, at least in the current Set-And-Then-Mutate scratchpad-style use of it— Probably move the calculations into a MapUnit.getMaintenance() method that doesn't expose any intermediate calculations/state, and then cache the results locally in a Map<MapUnit, Float> or List<Pair<MapUnit, Float>> within getUnitMaintenance()— Or actually, it doesn't look like the MapUnit is actually used (and there's only one per-unit coefficient anyway on top of the empire-wide discount?), so could just do a List<Float> in getUnitMaintenance()— So there's no longer any chance of properties being shared or intermediate states being clobbered.Probably B IMO.
Unrelatedly, I can say with some confidence that raw unit count is not sufficient to make this crash happen:
(So crash probability probably doesn't scale with the number of Floats/Longs, lending further credence to concurrency/control flow as the culprit. Also, I was buying units in the City Screen— With its threaded(?) stat update calls— The one time I did accidentally manage to reproduce this crash.)
itanasi
commented
2 years ago Of course it's multithreading....
Nice work!
yairm210
commented
2 years ago That was it folks, the versions since this change show 0 of these errors Nice work :)