Pre allocate current action entities#52
Merged
Merged
Conversation
The first step in making action entities reusable is not creating an entity for each action queued up. This led me to take a look at the action/decision code and find a few ways to improve it. This change makes the decision struct smaller, moves it to `Types` since it's not a component, and removes an unnecessary component add to the battle handle. There was a slight increase from doing that first and third change. Also, I noticed the simulation options were not checked for remaining the same before and after every kind of simulation, so that's fixed. I did that while adding a check that will be useful soon for simulate turn battles staying mostly unchanged.
My changes from yesterday were not good from an API persepective. They were also not as good as they could be from a memory usage perspective. So, this change fixes that so each kind of decision has its own type to make it clear what actions a set of options can lead to, it's clearer how the each kind of decision map to those in Showdown, and they can all sit in an union smaller than the `SlotDecision` struct from before (6 bytes instead of 10). This didn't have much if any effect of benchmark results.
This commit changes how the action queue works so information about an action is only applied to an entity when the action is active. That allows for only allocating one entity per battle instead one per action. Actions are also now cleared using the `remove` function rather than `destroy`, which allows it to select the exact components to remove from it instead of looping over all possible components. These changes together made the simulate turn benchmarks ~10% faster and the ManageActionQueue.cpp code simpler.
The idea for this change came from me noticing the size of the components a move slot entity kept track of (a move name, pp, and max pp value) were the same size as an entity itself. The reason I’m making this change, especially now, is because move slots were a majority of the entities used per battle and I wanted to increase the max input count on the benchmarks. That’s important since I knew this entity decrease change was going to happen and I wanted to make sure the changes I make now would not worsen performance on large input counts with four moves per Pokemon. Move slots are now referenced in other components by their index in the Pokemon’s `MoveSlots` component. Since the one advantage of having a move slot be an entity, using a tag to mark if it’s disabled, is now not possible, there’s now a component that can be added to a Pokemon that stores what move slot indexes are disabled if any are. Reducing the number of entities this much led to some big performance wins. The main one is when simulation branching is enabled as those benchmarks are now 1.5 times faster. Analyze Effect also saw big improvements, in some cases 2.7 times faster. The turn simulations for random inputs also sped up by 1.6 times.
Turns out a reference was missing in the random input creation code, so the benchmarks for that were not as random as they ought to be. This commit updates the numbers with now more random inputs. There were no substancial differences except the simulate turn, one random battle, many random inputs test getting slower.
It's been so long between commits this time because, once I implemented the reuse of action move entities, there was a slowdown only in the many random battle and inputs simulate turn benchmark that I couldn't explain. Turns out it had nothing to do with the new code I wrote but rather how the battles were being created. Before this commit, using many simulate turn inputs with one battle used the cloning code to create a battle per input. That code create entities for all the clones of an original entity in order, keeping entities that would be in almost the same way close to each other. When many battles with one simulate turn input each was used, the code created all the entities for a battle before moving to the next one. This meant that, for example, all the Pokemon entities were spaced out between the battle, side, and action entities. The action move entities used to be created after the battle was set up, resulting in no other kind of entity being created in between them and them appearing sequentially. Because my first attempt at this change moved action move entity creation to battle setup and using many battles with different inputs didn’t use cloning to create those action move entities sequentially, the performance benefit from having data used at similar time close together was lost and the benchmark performed worse. I spent most of my time checking if the new code I wrote was the problem rather than if problems with old code were now more evident. Once I considered the benchmark itself could be wrong did I find my way to the solution. Because changing how battle setup works to improve entity data locality makes improvements on its own to performance, I’m splitting this change into two commits. This one changes the battle setup to create the entities for each kind of object all at once and set them up so entities that are clones of each other are also sequential.
This commit is the change I made before "part 1" that made the changes in "part 1" required to maintain performance. The main parts of this are creating the action move entities at battle setup, clearing action components instead of destroying the entities, and changing action move creations to happen one move at a time instead of jumping between different moves. That last change is the source of much of the speed up from this change.
Found some things while looking over the code before making the PR to fix. Removing that unused entity holder and that `removeFromEntities` shortcut resulted in small performance improvements.
Contributor
|
🤖 Passed All One Review Bot Checks ✔️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #51