AI Attack & getSpellAbilityToPlay Timeout #6577
Conversation
kevlahnota
changed the title
|
@kevlahnota about the AI timeout part, how about separating it into extra Threads per creature? threadSafeIterable would that change make problems for this ConcurrentModificationException? When Touching FCollection, how about looking at #3397 ? |
I changed the FCollection LinkedList to Lists.newCopyOnWriteArrayList() so it's already thread safe, threadSafeIterable returns the list directly from FCollection, also it uses Sets.newConcurrentHashSet(), (I think this is a shortcut to ConcurrentHashmap.newkeyset). |
|
I agree with @tool4ever's observations, and after these tweaks it looks like a very useful and nice option! :) |
I just thought we could clean up some code if we make: less own code means less errors ;P |
| synchronized (this) { | ||
| result = LIST; | ||
| if (result == null) { | ||
| result = Lists.newCopyOnWriteArrayList(); |
There was a problem hiding this comment.
by switching the default iterator you will produce crashes imo
because some code uses add() or remove()
and LinkedList did support it...
but read JavaDoc of CopyOnWriteArrayList
* The iterator will not reflect additions, removals, or changes to
* the list since the iterator was created. Element-changing
* operations on iterators themselves ({@code remove}, {@code set}, and
* {@code add}) are not supported. These methods throw
* {@code UnsupportedOperationException}.
There was a problem hiding this comment.
In most of our use cases, we need to make a copy via threadSafeIterable anyway,
otherwise, we get ConcurrentModificationException
And the Test seems to pass so far?
There was a problem hiding this comment.
I trust my brain/IDE more here ;)
Here's one example from PlayAi:
this will call remove() on a CardCollection, which is a FCollection
I don't see a way this doesn't crash now
There was a problem hiding this comment.
imo a better way would be to get the ones into a toRemove collection, and then remove them after the loop?
There was a problem hiding this comment.
Could rewrite individual cases to work with the changes, but dropping support for iterator.remove() across every FCollection still seems like a messy road to go down. I imagine you'd have developers still stepping on that rake for years afterward. Either because they're new and haven't yet learned FCollection's quirks, or because they changed an ArrayList<Card> into a CardCollection and now some List<Card> function downstream suddenly explodes.
There was a problem hiding this comment.
I trust my brain/IDE more here ;)
Here's one example from PlayAi:
this will call remove() on a CardCollection, which is a FCollection I don't see a way this doesn't crash now
Can you write a simple test so it will crash if the iterator add or remove is not supported?
There was a problem hiding this comment.
@Test
void testBadIteratorLogic() {
CardCollection cc = new CardCollection(new Card(1, null));
Iterator<Card> it = cc.iterator();
it.next();
it.remove();
}
There was a problem hiding this comment.
ah yes, i see it so does card collection use this? the docs says use list.remove() but I think i need to update those unless I can change to different concurrentlist which isn't available on default libs
There was a problem hiding this comment.
I suppose the direct calls have been avoided, I probably don't know enough about concurrency to consider any other implications from this 🤷♂️
Isn't that going to be a massive hit to performance for a lot of the single-threaded code? |
I did profile it and seems ok so far. I don't know any concurrent list that is available. How does the change react to your machine when using it with lots of token/card generation? |
|
@Hanmac I encountered concurrent modification on Card.class for the Iterable getchangedcardtypes. Is it safe to return ImmutableList.copyOf instead of Iterables.unmodifiableIterable? |
I saw the error too on sentry but I can't explain it yet Do you got a case to reproduce that? If yes I could make a short hot fix |
no but if it occurs before this change then the bug already exists before. No idea why it happens though |
|
@tool4ever I changed the ManaPool floatingmana -> arrayListMultimap to a custom ConcurrentMultimap implementation, seems to work fine now but the AI seems to pick or tap a single land at end of turn? I wonder why it does that and on the log it says AI picked {W}. it seems the opponent do this on end of turn and the end of the sa, upon checking, the spellabilities are both mana ability... do we need to filter out the mana ability and why the ai do that on end of turn? maybe remove if all of them are mana abilities? |
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.ConcurrentLinkedQueue; | ||
|
|
||
| public class ConcurrentMultiMap<K, V> { |
There was a problem hiding this comment.
Why exactly using your on class instead of using the collection4 package?
It wouldn't be much of a problem, I was probably going to use something of that package anyway
And should this implement https://guava.dev/releases/23.0/api/docs/com/google/common/collect/Multimap.html ?
There was a problem hiding this comment.
I tried implementing the guava multimap but you need to fulfill all those overrides (seems overkill for the manapool sake version), though the earlier version from collection4 package works too.
There was a problem hiding this comment.
To me the question is why does ManaPool need to be concurrent now?
If you're wanting to check multiple SA for p(l)ayability in some non-deterministic way won't some checks possibly fail if the pool is already being drained from another Future/Thread at the same time?!
There was a problem hiding this comment.
I thought we only get a single spellability to play, the only difference is they're computed concurrently (all possible SA) but AI will only play at a time. By the time the AI cast or activate the top SA, wouldn't the next SA checked/recomputed again if feasible to play? If you have better idea to access the manapool without concurrent modification then please enlighten.
There was a problem hiding this comment.
- checking SA includes mana test
- if more than one is available and the pool isn't empty then how are you keeping it synchronized when AI checks A + B at roughly the same time? maybe you didn't consider it but the pool is actively being modified/drained even when only testing
the whole AI logic isn't set up to jump around between threads even if the data structures might not crash from it now lol
There was a problem hiding this comment.
Would it be cleaner + efficient enough to implement threading through something more like the GameSimulator AI - copy the whole game state to separate threads and evaluate them without any risk of tampering with shared resources?
Yeah, i thought i could find a case to make the error reproduceable. @tool4ever for a smaller HOTFIX before this MR, what about using getchangedcardtypes a ImmutableList.copy? |
cause: tapped.addAll(tappedForMana) on AiCardMemory
| @@ -79,21 +79,21 @@ public enum MemorySet { | |||
| private final Set<Card> memRevealedCards; | |||
|
|
|||
| public AiCardMemory() { | |||
There was a problem hiding this comment.
@tool4ever @kevlahnota might it be cleaner if we change the structure into this?
Map<MemorySet, Set<Card>>, and have the whole logic inside?
Then we don't need extra properties for each MemorySet-Type
| SpellAbility root = sa.getRootAbility(); | ||
| // dumb check needed? don't really know why the AI tapped the land for nothing at EOT of his opponent. | ||
| // without this, you can see the weird tapping of single land -> mana ability. with this, it doesn't confuse human player | ||
| if (sa.isManaAbility() && game.getPhaseHandler() != null && game.getPhaseHandler().is(PhaseType.END_OF_TURN) && !game.getPhaseHandler().isPlayerTurn(player)) |
There was a problem hiding this comment.
are you saying it only happens on this branch?
then some logic got messed up somehow and should be fixed instead...
otherwise I can try and fix it on master
There was a problem hiding this comment.
I was thinking on the sorting of SA. On this branch I moved the sort after the SA is gathered with timeout since the order will not be the same and needs to be sorted again. Gonna try tomorrow again.
There was a problem hiding this comment.
I'm having a hard time to understand that reasoning.
Before:
it presorts with quicker heuristics and then grabs the first that passes API logic
Now:
you're making AI potentially wait (at least until timeout reached) to gather all API checks first (which are way more expensive on average!)
This is a completely different approach and has the potential to "dumb" the AI down by a ton depending on boardstate in a way that will be almost impossible to debug properly. :/
Sorry but if you feel this is somehow worth it I really think this needs to be way more of an isolated feature for speedy people.
There was a problem hiding this comment.
I don't want to argue on what you believe but the point of this change is to have a timeout regardless if the boardstate is complex or not. The current problem is where the number to process those objects using sequential approach will eventually be slower than processing them concurrently and anything that's within the timeout will have results than waiting forever to wait for the game to proceed. Also the threaded computation are only specifically selected on the most expensive computation so it will still have results if it will timeout or not. This was intended to make the game move on those state where it's hard to process all. I don't know how long you play the game but it's tiresome to wait for the AI if it's not producing results without timeout. All this change was to have a better play experience than having a perfect AI logic that will stall over time if the boardstate becomes to much to process.
There was a problem hiding this comment.
yes I understand all that and I'm not against it
frankly often when I debugged some slowdown it was at least partially caused by very few cards/API which run a badly optimized path (often custom written AILogic$)
but anyway my point was why another structural change on top of this?
why force check all API first when there's a good chance you could find a playable candidate way before the last element?
is it because you have no control over the order the Futures get calculated so you can't just return with one of the first matches?
There was a problem hiding this comment.
Because you don't know what will be finish first specially if the spellabilities is big. Example you have 10 creatures that have deals damage to target creature. And the opponent have a lot of possible target. If the first one didn't find good target, it will take the next one again and will have the same result as the first one leaving the computation to process all of them sequentially to just not find anything feasible. If you process them concurrently, you will have faster results. The cons of this approach is if those abilities are unique, you need to sort them after. unlike the previous implementation that is already sorted (if verify transitivity didn't occur) and if the logic passes, it will be returned, the cons of that approach is if the top sa will fail the logic, it will process the next one and wait if it passes or not and so on.
Nothing significant in gameplay, though it's hard to pin down what's causing sluggishness just by watching, and I haven't had much luck finding a reasonably priced profiler on Linux. I ran some basic benchmarks - creating and destroying a ton of tokens, shuffling a deck a bunch of times. Destroying things seems unaffected funnily enough, but other operations do experience exponential slowdown with higher numbers and the CopyOnWriteArrayList.
Obviously these numbers are far above what you could expect to have in an actual game, but Forge already has a number of issues with scaling to larger board states and this would compound that somewhat. A custom implementation may be better than the out-of-the-box list. You could also just make separate thread-safe collections for concurrent operations to minimize the impact, perhaps either a subclass of FCollection or a method that instructs it to switch its underlying list to a thread-safe one when it's needed. |
That's what the `threadSafeIterable`` function was for, while not changing the underlying structure, it still returns a new list that then can be safely iterated. |
Hmm FCollection needs List without duplicates (the cards has its own id, so they're all unique I guess), @Hanmac suggested the SetUniqueList but it's another addition that needs to be tested. Eitherway, the copyonwriteareaylist works fine on less than 1000 objects but will suffer when used on generating ai genetic decks (but that generation is slow even before the change of fcollection) |
I was misled by the name, since why it needs to return another list if the list structure is already threadsafe/concurrent. |
the fcollection will check if removing from set is successful, it will also remove the element from the list
The original purpose which is to create a copy of linkedlist to iterate to avoid concurrent modification, but we use copyonwritearray design and it is already thread safe and iteration while modification is handled internally.
No description provided.