smogon / pokemon-showdown

Pokémon battle simulator.
https://pokemonshowdown.com
MIT License
4.79k stars 2.8k forks source link

Potential speed-up in `sim/battle.ts` #10629

Open larry-the-table-guy opened 2 weeks ago

larry-the-table-guy commented 2 weeks ago

A considerable amount of time is spent in sim/battle.ts during npm run full-test.

Profiling

Here are some simple profiling results taken during npm run full-test, on a branch with #10616 (because that removes most of the randbats time, which makes ExhaustiveRunner stand out).

v8 prof

I'm seeing 30-40 seconds worth attributed to findEventHandlers and findPokemonEventHandlers (the latter is the bulk of the former) in a v8 prof.

ad hoc profiling (performance.now() and counting):

findEventHandlers 6.5 million calls 42368 milliseconds Returned handlers length: frequency { '0': 6152113, '1': 335755, '2': 11933, '3': 188, '4': 10, '9': 1 } 94% of the time, no handlers.

findPokemonEventHandlers 38.5 million calls 37365 milliseconds Returned handlers length: frequency { '0': 38137046, '1': 348380, '2': 13907, '3': 617, '4': 48, '5': 2 } 99% of the time, no handlers.

Note: with call counts this high, my caveman profiling has noticeable overhead. I saw +4 seconds in findEventHandlers and +10 seconds in findPokemonEventHandlers. Nevertheless, findPokemonEventHandlers is a hotspot in ExhaustiveRunner during tests.


My suggestion

It's already ~1 microsecond per call. I don't think that can get radically faster in JS. So, call it less. I think a strategy like modData in sim/dex.ts can work here: Anything that wants to add a handler must call a designated method, and that will set a flag which sim/battle.ts checks before looking for handlers. Conversely, when a handler is removed, the remove method will check if there are any handlers left and update the flag accordingly.

Then, during findEventHandlers and findPokemonEventHandlers: If the flag is false, there are no handlers, return early. This is the 95-99% case. If the flag is true, there are handlers, run the method as normal.

Slayer95 commented 2 weeks ago

Anything that wants to add a handler must call a designated method, and that will set a flag which sim/battle.ts checks before looking for handlers.

That was one of the intended purposes of Battle#onEvent(). In my mind, even, Battle could be a subclass of EventEmitter, or very similar, anyway. However, after I got Battle#onEvent() working for unit tests, I deprioritized that plan.

Slayer95 commented 1 week ago

Anyone interested in tackling this would benefit from reading https://github.com/smogon/pokemon-showdown/pull/5439/

Documentation rot fix: s/Effect/Condition