[Performance] Improve the performance of Radius.tsx by 2x

[rortan134]

Closes #9

I used the moveable function as a benchmark entry point so other results may vary.
Consistently achieved just a bit over a 2x improvement with few to no core changes.

Platform
Windows_NT 10.0.19045 x64
Node.JS: 20.12.2
V8: 11.3.244.8-node.19

fast moveable	0% (base)	(16.479 ops/sec)	(avg: 60μs)
regular moveable	-56,9%	(7102 ops/sec)	(avg: 140μs)

[rortan134]

All current tests pass but let me know if you want me to add a couple more to make sure

[cpojer]

Two notes on this:

• vectors are immutable objects that are all the same, so you can use them directly as keys for maps without stringify-ing them.
• We cannot cache at the module level a map (MapData instance) changes after mutations, or we might check the movement radius of different maps.

We need one map per MapData instance – and then the question is how we evict the memory since it might be running in a long running process on the client or server.

[rortan134]

and then the question is how we evict the memory since it might be running in a long running process on the client or server.

First thing that comes to mind is an LRU cache for each map instance in this case

[cpojer]

I would assume we could use a WeakMap?

[cpojer]

As in, a WeakMap<MapData, Map<Vector, boolean>>.

[rortan134]

I would assume we could use a WeakMap?

It's viable, although I suggest a more robust library. Let me know what you think about these latest changes

[cpojer]

That's awesome, thank you for the PR! I left a few comments to make sure we aren't breaking something across maps by caching data globally. Could you also share the benchmark you used?

[rortan134]

Could you also share the benchmark you used?

I used the benchmarkify library with the mock data from the first test in (Radius.test.tsx)

[cpojer]

Could you also share the benchmark you used?

I used the benchmarkify library with the mock data from the first test in (Radius.test.tsx)

That's great. Can you also share the code so we can repro it?

[rortan134]

That's great. Can you also share the code so we can repro it?

Absolutely, https://github.com/rortan134/athena-crisis/blob/radius-perf-bmark/athena/__tests__/RadiusBenchmark.test.tsx
Just run the test script

[cpojer]

Thanks for the updates!

I realized that we can actually avoid all of the extra complexity and don't need to add an LRUCache. The radius cache is specific to a MapData instance, and MapData is immutable. It's just a cache to speed things up, so how about we just add the cache interface to MapData directly? Then it will be garbage collected automatically.

However, this led me to uncover a gap in this optimization. calculateRadius takes four inputs: A map, a unit, a starting Vector and the radius. All four of those can be different at any time, like for example we might want to get a Flamethrower's radius at the center of a desert map, or a Sniper's radius in a volcano map at the left top. However, we might also check different units at one position on the same map, and each unit has different movement costs on the same field, like for example an ocean might be inaccessible to a Flamethrower but a Fighter Jet can just fly over it. This means that caching on a per-map and per-field basis will not work. Let me know if you'd like me to add a bunch of test cases to verify these behaviors.

[rortan134]

I see.

Let me know if you'd like me to add a bunch of test cases to verify these behaviors.

A couple test cases would be great

[cpojer]

Instead of changing these functions, let's add getTileInfoByIndex and maybeGetTileInfoByIndex functions.

[cpojer]

getMutableUnitAccessibilityCache

[cpojer]

Also, as shared earlier, we can just use Vector directly as keys. vec(1, 1) === vec(1, 1).

[cpojer]

Thanks for the updates, I think we are getting closer to something that can be merged. As for the benchmark, it's not super representative to run it on the same inputs – if that was a common case we could just cache the outputs of the function completely! I added one new test on main that will fail on this branch.

Here is a more representative benchmark that calculates the radius for each unit on the map:

test('benchmark moveable radius', async () => {
  const testMap = startMap.copy({
    units: startMap.units.delete(vec(11, 8)),
  });
  
  const benchmark = new Benchmark('benchmark moveable').printHeader();
  benchmark
    .createSuite('starting benchmark', { time: 10_000 })
    .add('moveable', () => {
      for (const [vector, unit] of testMap.units) {
        moveable(testMap, unit, vector, 7)
      }
    })
    .add('fast moveable', () => {
      for (const [vector, unit] of testMap.units) {
        fastMoveable(testMap, unit, vector, 7)
      }
    })
  await benchmark.run();
});

It might be worth randomizing the order at the beginning of the run, but this should be a good start.

[cpojer]

I added the benchmarks right to the repo using pnpm vitest bench: 5d3bc85

Feel free to extend them as well.

[cpojer]

@rortan134 are you planning on getting back to this?

[cpojer]

Closing due to inactivity. Happy to reopen if it's being picked up again.