Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec09e18
improve perf
rortan134 Jul 7, 2024
eeca5a9
fix: use vector coordinates as cache id
rortan134 Jul 7, 2024
7a236de
rename length variables
rortan134 Jul 8, 2024
ddae0b7
rename to previousTileInfo
rortan134 Jul 8, 2024
e929a36
update cache impl
rortan134 Jul 8, 2024
c24ec81
update cache impl
rortan134 Jul 9, 2024
04f09d4
Merge remote-tracking branch 'upstream/main' into radius-perf
rortan134 Jul 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions athena/MapData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,19 +271,18 @@ export default class MapData {
: null;
}

getTileInfo(vector: Vector, layer?: TileLayer) {
getTileInfo(vector: Vector, layer?: TileLayer, index?: number) {
if (!this.contains(vector)) {
throw new Error(
`getTileInfo: Vector '${vector.x},${vector.y}' is not within the map limits of width '${this.size.width}' and height '${this.size.height}'.`,
);
}

return getTileInfo(this.map[this.getTileIndex(vector)], layer);
return getTileInfo(this.map[index ?? this.getTileIndex(vector)], layer);
}

maybeGetTileInfo(vector: Vector, layer?: TileLayer) {
maybeGetTileInfo(vector: Vector, layer?: TileLayer, index?: number) {
if (this.contains(vector)) {
return getTileInfo(this.map[this.getTileIndex(vector)], layer);
return getTileInfo(this.map[index ?? this.getTileIndex(vector)], layer);
}
}
Comment on lines +287 to 300
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Expand Down Expand Up @@ -417,7 +416,8 @@ export default class MapData {
value: T,
): T {
const { map, size } = this;
for (let i = 0; i < map.length; i++) {
const len = map.length;
rortan134 marked this conversation as resolved.
Show resolved Hide resolved
for (let i = 0; i < len; i++) {
value = fn.call(this, value, indexToVector(i, size.width), i);
}
return value;
Expand Down Expand Up @@ -451,7 +451,8 @@ export default class MapData {
value: T,
): T {
const { map, modifiers, size } = this;
for (let i = 0; i < map.length; i++) {
const len = map.length;
for (let i = 0; i < len; i++) {
const field = map[i];
if (typeof field === 'number') {
value = fn.call(
Expand Down Expand Up @@ -503,7 +504,8 @@ export default class MapData {
value: T,
): T {
const { decorators, size } = this;
for (let i = 0; i < decorators.length; i++) {
const len = decorators.length;
for (let i = 0; i < len; i++) {
const decorator = getDecorator(decorators[i]);
if (decorator) {
value = fn.call(
Expand Down
136 changes: 76 additions & 60 deletions athena/Radius.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import Vector from './map/Vector.tsx';
import MapData from './MapData.tsx';

type RadiusConfiguration = {
getCost(map: MapData, unit: Unit, vector: Vector): number;
getCost(map: MapData, unit: Unit, vector: Vector, index?: number): number;
getResourceValue(unit: Unit): number;
getTransitionCost(
info: UnitInfo,
Expand All @@ -37,38 +37,54 @@ export const RadiusItem = (
vector,
});

function isAccessibleBase(map: MapData, unit: Unit, vector: Vector) {
if (!map.contains(vector)) {
return false;
const cacheMap = new Map();

function getCostBase(map: MapData, unit: Unit, vector: Vector, index?: number) {
const tileInfo = map.maybeGetTileInfo(vector, undefined, index);
return tileInfo ? tileInfo.getMovementCost(unit.info) : -1;
}

function getTransitionCostBase(
info: UnitInfo,
current: TileInfo,
parent: TileInfo,
) {
if (parent.group !== current.group) {
return parent.getTransitionCost(info) + current.getTransitionCost(info);
}
return 0;
}

function isAccessibleBase(map: MapData, unit: Unit, vector: Vector) {
const unitB = map.units.get(vector);
if (unitB && map.isOpponent(unitB, unit)) {
return false;
}

const building = map.buildings.get(vector);
if (building && !building.info.isAccessibleBy(unit.info)) {
return false;
}
return !(building && !building.info.isAccessibleBy(unit.info));
}

return true;
function isAccessible(map: MapData, unit: Unit, vector: Vector) {
const key = vector.toJSON();
let accessible = cacheMap.get(key);
if (accessible != null) {
return accessible;
}
accessible = isAccessibleBase(map, unit, vector);
cacheMap.set(key, accessible);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume we could use a WeakMap?

Copy link
Contributor

@cpojer cpojer Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return accessible;
}

export const MoveConfiguration = {
getCost: (map: MapData, unit: Unit, vector: Vector) =>
map.maybeGetTileInfo(vector)?.getMovementCost(unit.info) || -1,
getCost: getCostBase,
getResourceValue: (unit: Unit) => unit.fuel,
getTransitionCost: (info: UnitInfo, current: TileInfo, parent: TileInfo) =>
(current.group !== parent.group &&
parent.getTransitionCost(info) + current.getTransitionCost(info)) ||
0,
isAccessible: isAccessibleBase,
getTransitionCost: getTransitionCostBase,
isAccessible,
} as const;

const VisionConfiguration = {
getCost: (map: MapData, unit: Unit, vector: Vector) =>
map.maybeGetTileInfo(vector)?.configuration.vision || -1,
getCost: (map: MapData, unit: Unit, vector: Vector, index?: number) =>
map.maybeGetTileInfo(vector, undefined, index)?.configuration.vision || -1,
getResourceValue: () => Number.POSITIVE_INFINITY,
getTransitionCost: () => 0,
isAccessible: (map: MapData, unit: Unit, vector: Vector) =>
Expand All @@ -81,67 +97,63 @@ function calculateRadius(
start: Vector,
radius: number,
{
getCost,
getResourceValue,
getTransitionCost,
isAccessible,
}: RadiusConfiguration = MoveConfiguration,
): Map<Vector, RadiusItem> {
const { info } = unit;
const closed = new Array(map.size.width * map.size.height);
const closed: { [key: number]: 1 } = {};
cpojer marked this conversation as resolved.
Show resolved Hide resolved
const paths = new Map<Vector, RadiusItem>();
const queue = new FastPriorityQueue<RadiusItem>((a, b) => a.cost < b.cost);
const minRadius = Math.min(radius, getResourceValue(unit));
queue.add(RadiusItem(start));

while (!queue.isEmpty()) {
let index: number = map.getTileIndex(start);
do {
const { cost: parentCost, vector } = queue.poll()!;
const index = map.getTileIndex(vector);
index = map.getTileIndex(vector);
if (closed[index]) {
continue;
}
closed[index] = true;
closed[index] = 1;

const vectors = vector.adjacent();
for (let i = 0; i < vectors.length; i++) {
const currentVector = vectors[i];
const parentTileInfo = map.getTileInfo(vector, undefined, index);
for (const currentVector of vectors) {
if (!map.contains(currentVector)) {
continue;
}
const currentIndex = map.getTileIndex(currentVector);
if (closed[currentIndex]) {
continue;
}
const cost = getCost(map, unit, currentVector);
const currentTileInfo = map.getTileInfo(
currentVector,
undefined,
currentIndex,
);
const cost = currentTileInfo.getMovementCost(unit.info);
if (cost < 0 || !isAccessible(map, unit, currentVector)) {
closed[currentIndex] = true;
closed[currentIndex] = 1;
continue;
}
const nextCost =
parentCost +
cost +
getTransitionCost(
info,
map.getTileInfo(vector),
map.getTileInfo(currentVector),
);
getTransitionCost(unit.info, parentTileInfo, currentTileInfo);
if (nextCost > minRadius) {
continue;
}
const previousPath = paths.get(currentVector);
if (
nextCost <= radius &&
(!previousPath || nextCost < previousPath.cost) &&
nextCost <= getResourceValue(unit)
) {
const item = {
cost: nextCost,
parent: vector,
vector: currentVector,
};
if (!previousPath || nextCost < previousPath.cost) {
const item = RadiusItem(currentVector, nextCost, vector);
paths.set(currentVector, item);
if (nextCost < radius) {
queue.add(item);
}
}
}
}
} while (!queue.isEmpty());
return paths;
}

Expand Down Expand Up @@ -177,6 +189,7 @@ export function getPathCost(
const seen = new Set([start]);
let previousVector = start;
let totalCost = 0;
const previousVectorTileInfo = map.getTileInfo(previousVector);
rortan134 marked this conversation as resolved.
Show resolved Hide resolved

for (const vector of path) {
if (seen.has(vector) || !map.contains(vector)) {
Expand All @@ -195,11 +208,7 @@ export function getPathCost(

totalCost +=
cost +
getTransitionCost(
info,
map.getTileInfo(vector),
map.getTileInfo(previousVector),
);
getTransitionCost(info, map.getTileInfo(vector), previousVectorTileInfo);

if (totalCost > radius || totalCost > getResourceValue(unit)) {
return -1;
Expand All @@ -212,29 +221,35 @@ export function getPathCost(
return !unitB || canLoad(map, unitB, unit, previousVector) ? totalCost : -1;
}

function getVisionRange(
map: MapData,
unit: Unit,
start: Vector,
radius: number,
) {
const range = unit.isUnfolded()
? 2
: unit.info.type === EntityType.Infantry &&
map.getTileInfo(start).type & TileTypes.Mountain
? 1
: 0;
return radius + range;
}

export function visible(
map: MapData,
unit: Unit,
start: Vector,
radius: number = unit.info.configuration.vision,
): ReadonlyMap<Vector, RadiusItem> {
const vision =
radius +
(unit.isUnfolded()
? 2
: unit.info.type === EntityType.Infantry &&
map.getTileInfo(start).type & TileTypes.Mountain
? 1
: 0);

const vision = getVisionRange(map, unit, start, radius);
cpojer marked this conversation as resolved.
Show resolved Hide resolved
const visible = calculateRadius(
map,
unit,
start,
vision,
VisionConfiguration,
);

const player = map.getPlayer(unit);
const canSeeHiddenFields =
player.activeSkills.size &&
Expand Down Expand Up @@ -339,7 +354,8 @@ export function attackable(
}

const vectors = parent.vector.adjacent();
for (let i = 0; i < vectors.length; i++) {
const len = vectors.length;
for (let i = 0; i < len; i++) {
const vector = vectors[i];
if (map.contains(vector)) {
const itemB = attackable.get(vector);
Expand Down
2 changes: 1 addition & 1 deletion athena/info/Tile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ export class TileInfo {
}

getTransitionCost({ movementType }: { movementType: MovementType }): number {
return this.configuration.transitionCost?.get(movementType) || 0;
return this.configuration.transitionCost?.get(movementType) ?? 0;
}

isInaccessible() {
Expand Down
5 changes: 3 additions & 2 deletions athena/map/Vector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export default abstract class Vector {

adjacent() {
return (
this.vectors ||
this.vectors ??
(this.vectors = [
this.up(),
this.right(),
Expand Down Expand Up @@ -130,7 +130,8 @@ export function decodeVectorArray(
array: ReadonlyArray<number>,
): ReadonlyArray<Vector> {
const result = [];
for (let i = 0; i < array.length; i += 2) {
const len = array.length;
for (let i = 0; i < len; i += 2) {
result.push(vec(array[i], array[i + 1]));
}
return result;
Expand Down