-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dexsearch optimization #10536
Dexsearch optimization #10536
Conversation
No behavioral changes (other than console.log spam), this commit is just to establish a baseline and pinpoint the slow portions
Based on preliminary benchmarks, most of the time was spent getting the move validator, even for queries that don't specify a move. More importantly, the parameters for fetching the move validator are known very early in the function and don't change during the loop. Pulling that portion out of the loop is an easy win.
- add subcategories for filtering on move (the next optimization target) - report unaccounted time amounts ('known unknowns') - make grand total stand out more
Move list depends on 'alts', which does not change in the loop over mons. Minor win, but simple.
Perf comparison for latest (minor) change. Before
After
|
Neither 'mod' nor 'altMoves' changes during the inner loop.
pokemonSources, which appears to act as a set, grows with each iteration, adding seemingly redundant items. Will need to look closely at TeamValidator to identify the problem. My guess is that it's putting object references into a Set, which, for objects other than strings, only cares about object identity.
Example of issue identified in previous commit about possible leak. when the imposter is sus
I'll tackle this next, could be another big, easy win. |
Also, semi-fix issue identified in prior commit - the endless growth of the restrictiveMoves list. Counting the calls to checkCanLearn helps us reason about whether the cost per call is reasonable.
use the cached ruleTable, save 100x.
Before and after switching to using the cached Before
After
|
@@ -2693,7 +2693,9 @@ export class TeamValidator { | |||
if (!setSources.restrictiveMoves) { | |||
setSources.restrictiveMoves = []; | |||
} | |||
setSources.restrictiveMoves.push(move.name); | |||
if (!setSources.restrictiveMoves.includes(move.name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to change restrictiveMoves
from an array to a Set. The lengths I've observed are 1 - 40, heavily weighted towards the low end. Linear scan on a small array isn't too bad, and sometimes better than a hashset. I haven't checked all the usages to see if switching to a Set is reasonable, and can't be sure that there isn't some scenario that grows restrictiveMoves
to hundreds or thousands of elements.
For queries that never mention a move, a considerable chunk of time is wasted getting the objects needed for 'checkCanLearn'. I measure ~1ms savings for the relevant queries, which is often a decent percentage.
stats for `npm test` after latest commit
|
Wow, I never realized how expensive |
This is to document that they are not the bottlenecks.
We've gotten to the point where we actually *are* measuring microseconds, so the frequent calls are too expensive. This is a good problem to have! What we can see from running npm test is that 'filters' and 'unaccounted' still account for a majority of the time. So, there's still room for improvement, if that's ever a serious concern. Personally, I think working on the moves API would be more fruitful.
No more console.log or performance.now() calls. Ready to merge.
I think It's perfectly fine to squash these commits, many were just intended as a trail of breadcrumbs so that one can follow the reasoning and try the experiments themselves. |
server/chat-plugins/datasearch.ts
Outdated
/** @ts-expect-error validator and pokemonSource won't be undefined if there's at least one move */ | ||
if (!validator.checkCanLearn(move, dex[mon], pokemonSource) === alts.moves[move.id]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** @ts-expect-error validator and pokemonSource won't be undefined if there's at least one move */ | |
if (!validator.checkCanLearn(move, dex[mon], pokemonSource) === alts.moves[move.id]) { | |
if (!validator.checkCanLearn(move, dex[mon], pokemonSource) === alts.moves[move.id]) { |
/** @ts-expect-error validator and pokemonSource won't be undefined if there's at least one move */ | |
if (!validator.checkCanLearn(move, dex[mon], pokemonSource) === alts.moves[move.id]) { | |
if (validator && !validator.checkCanLearn(move, dex[mon], pokemonSource) === alts.moves[move.id]) { |
We prefer not using ts-expect-error if we can avoid it. Which we can here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's understandable. Here, however, if it's not initialized and it was supposed to be, this would hide the logic error. So, my thinking was that if someone did mess up the validator initialization logic, this would fail early and loudly during tests.
But I guess that during a test, this would produce wrong values anyway, and it would eventually get caught.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case you would want to do
if (!validator!.checkCanLearn(...
which is what I would recommend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree that it's better to crash than to produce incorrect results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or the other approach is to lazily initialise validator
here if it hasn't already been initialised, something along these lines:
let validator;
...
if (!validator) {
...
validator = ...
}
if (!validator.checkCanLearn ...
Format the 'format' string in-line. Co-authored-by: Mia <[email protected]>
Ah, test is failing because I was trying to apply Mia's second set of changes through the web interface and forgot to add the |
In short, dexsearch
iswas abnormally slow. A simple query like/nds fire
took ~2 seconds and/nds pivot
~4 seconds on the prod server and my decent laptop.This PR applies a few extremely simple fixes that address ~99% of the performance problems. The aforementioned queries now take ~5 milliseconds and 30 milliseconds, respectively. Far more optimizations are possible, but won't be as simple or impactful.
Results
Below are the (very scarce) performance stats for
npm test
, running on ani7-9750h
.These are cheaper than
/nds
. The first test is especially slow because of the nature of JITs.Initial
Latest
(I go by 'ThereRNoNamesLeft' on Showdown)