Data: Fix huge perf bugs in randbat tests, part 1 #10616
Conversation
This file contains 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
larry-the-table-guy
changed the title
randomNPokemon
|
Awesome. By the way, I have been wondering whether switching |
|
I haven't looked too hard into the algorithms being used here. For randbats, it seems movesets are the next priority. Here's the new v8 prof for npm run full-testI wish I could get flamegraphs working on Node but I keep getting garbage output :/. |
Previously computed whole move pool just to see whether it's empty. It accounts for about 25% of the test suite runtime.
larry-the-table-guy
force-pushed
the
data/randomNPokemon-movepool-perf-bug
branch
from
e2f57ef
to
2843aa4
Compare
larry-the-table-guy
changed the title
randomNPokemon
|
Got some more stats for
^My counters don't include calls with early returns (because those don't tend to be the expensive paths).
Context: So, the hotspots are I tried replacing |
|
@Slayer95 I've now read more of
Death by a thousand cuts, basically. I think the next leap in performance for It'd be nice if the test suite got faster, but that alone isn't enough motivation to do all that. I'll give it a couple more days to see if I find another freebie, but it's not looking too hopeful. On the bright side, I am seeing ~10K fewer ticks (so, ~5%) when I merge in #10608 . So, maybe we can squeeze out a few more % with just |
In a context where there are already 2 actively used competing approaches to Random Battles (Battle Factory, and "CG Teams"), I don't know how to feel about this. |
|
Found a less invasive and more effective way to reduce the redundant work in Total ticks went from 185K to 170K. The short-circuiting I had added for I have no strong opinions on the code style for the caches. ATM I think you could break this by changing the New v8 prof snippets |
|
I estimate ~5 out of 59 seconds ( |
This is to prevent invalidating the caches derived from 'dex' at construction time
Let's Get This Merged 🚀 |
This was referenced Oct 22, 2024
Forgot to remove this when I reverted the sizeLimit commit.
Zarel
reviewed
Oct 31, 2024
data/random-battles/gen9/teams.ts
Outdated
| @@ -164,6 +164,12 @@ export class RandomTeams { | |||
| */ | |||
| moveEnforcementCheckers: {[k: string]: MoveEnforcementChecker}; | |||
|
|
|||
| /** Used by .getPools() */ | |||
| private poolsCacheKey: any[] | undefined; | |||
There was a problem hiding this comment.
It feels like this can be more specific than any[].
There was a problem hiding this comment.
[string, number, boolean, RuleTable] | undefined seems to be what it's supposed to be?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A lot of time in
npm run full-testis spent in randbats code.Fixes
RandomTeamsetc.testSetandtestTeampreviously constructed a generator every round, just to change the seed.getGeneratorpreviously called toID multiple times on the same format (least significant change)Times
added
--v8-profflag to mocha command forfull-testinpackage.json.npm run full-testsetSeedgeneratortoIDformat onceRandomTeamsnpm run full-test, just./test/random-battles/**/*.jsBaseline -> head: 223K -> 57K
old stats
Action:
npm run full-testBase commit: ca27f79
Reporting granularity:
randomNPokemon: 512 calls
getMovePool: 64K calls
2,841,615+ calls to
getMovePoolcame fromrandomNPokemon.console dump for pre times
unit: milliseconds.
each row is 512 or 64K calls.
Something to check later in
randomNPokemon:Array
.includescalled on 1000 element arrays of ints. On the order of a billion elements are visited duringnpm run full-test. Int ops are pretty fast, so probably not an issue. But there's potential for a speed-up here.size of pool in `randomNPokemon` when calling `.includes`
length: frequency