Perf/Fix: spawn and other functions that need closest by unit#3243
Perf/Fix: spawn and other functions that need closest by unit#3243VariableVince wants to merge 14 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds two generic utilities, Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/Util.ts (1)
95-101:findClosestByis a thin alias overfindMinimumBy— consider whether both exports are needed long-term.Right now
findClosestByjust forwards tofindMinimumBywith zero added logic. The only difference is the parameter name (distancevsscore). This is fine for readability at call sites — "closest by distance" reads more naturally than "minimum by score" when dealing with spatial lookups. Just keep in mind that maintaining two nearly identical public exports adds surface area. IffindMinimumBynever gets used directly outside offindClosestBy, you could collapse them later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Util.ts` around lines 95 - 101, findClosestBy is a trivial wrapper around findMinimumBy with no added behavior (only a parameter rename), which risks duplicated public surface area; either remove the redundant export or make the intent explicit: search the repo for external usages of findClosestBy and findMinimumBy and if both are used keep both, otherwise collapse into a single export (prefer the clearer name for call sites, e.g., keep findClosestBy and remove or inline findMinimumBy), or add a short comment above findClosestBy documenting it's an intentional alias for readability; update any imports accordingly to use the chosen single function (references: findClosestBy, findMinimumBy).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/Util.ts`:
- Around line 95-101: findClosestBy is a trivial wrapper around findMinimumBy
with no added behavior (only a parameter rename), which risks duplicated public
surface area; either remove the redundant export or make the intent explicit:
search the repo for external usages of findClosestBy and findMinimumBy and if
both are used keep both, otherwise collapse into a single export (prefer the
clearer name for call sites, e.g., keep findClosestBy and remove or inline
findMinimumBy), or add a short comment above findClosestBy documenting it's an
intentional alias for readability; update any imports accordingly to use the
chosen single function (references: findClosestBy, findMinimumBy).
|
looks like a test is failing |
…redicate is provided
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/Util.ts (1)
110-116:findClosestByis a pure alias — consider whether it pulls its own weight.Right now
findClosestByadds no behaviour on top offindMinimumBy; they share the same signature and the body is a single delegation line. Two options worth considering:
- Keep as-is — the distinct name signals "this is a spatial distance query" at call sites, which is fine.
- Remove
findClosestByand have callers importfindMinimumBydirectly, reducing surface area.Either is valid; this is just a nudge to settle the "possible consolidation" point the PR already raises before more callers are added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Util.ts` around lines 110 - 116, findClosestBy is a no-op alias of findMinimumBy; decide whether to remove it or clarify intent: either delete the findClosestBy function and replace all its call sites/imports to use findMinimumBy (update tests and exports accordingly), or keep findClosestBy but add a brief doc comment above the function explaining it's a semantic alias for spatial/distance queries so callers understand the intent and keep the single-line delegation as-is; reference the symbols findClosestBy and findMinimumBy when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/Util.ts`:
- Around line 102-109: The JSDoc example for findClosestBy uses the incorrect
GameMap variable name "mg" — change it to "gm" to match the file's convention
and other helpers like distSort and distSortUnit; update the example line
containing mg.manhattanDist(...) to use gm.manhattanDist(...) so examples are
consistent and copy-pasteable for readers referencing findClosestBy.
---
Nitpick comments:
In `@src/core/Util.ts`:
- Around line 110-116: findClosestBy is a no-op alias of findMinimumBy; decide
whether to remove it or clarify intent: either delete the findClosestBy function
and replace all its call sites/imports to use findMinimumBy (update tests and
exports accordingly), or keep findClosestBy but add a brief doc comment above
the function explaining it's a semantic alias for spatial/distance queries so
callers understand the intent and keep the single-line delegation as-is;
reference the symbols findClosestBy and findMinimumBy when making the change.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/game/PlayerImpl.ts (1)
1183-1193:warshipSpawnstill usesthis.mginstead of a local cache.Not a bug, but
nukeSpawncachesthis.mginto a localmgfor speed, whilewarshipSpawndoes not. If this method is also on a hot path, the same local-cache pattern would be consistent. Low priority sincewarshipSpawndoes fewer lookups.Optional: cache mg locally for consistency
warshipSpawn(tile: TileRef): TileRef | false { - if (!this.mg.isOcean(tile)) { + const mg = this.mg; + if (!mg.isOcean(tile)) { return false; } - const bestPort = findClosestBy(this.units(UnitType.Port), (port) => - this.mg.manhattanDist(port.tile(), tile), + const bestPort = findClosestBy(this.units(UnitType.Port), (port) => + mg.manhattanDist(port.tile(), tile), ); return bestPort?.tile() ?? false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/PlayerImpl.ts` around lines 1183 - 1193, warshipSpawn currently accesses this.mg repeatedly; mirror the nukeSpawn optimization by caching this.mg into a local const (e.g., const mg = this.mg) at the top of the warshipSpawn method and then use mg.isOcean and mg.manhattanDist in the findClosestBy callback (leave findClosestBy, units(UnitType.Port), and UnitType.Port unchanged) so lookups are consistent and slightly faster on hot paths.src/core/Util.ts (1)
102-116:findClosestByis a pure alias forfindMinimumBy— consider if both are needed.Right now
findClosestByjust callsfindMinimumBywith identical signature/behavior. The only difference is the parameter name (distancevsscore). This is fine for readability at call sites, but be aware the two could drift apart if someone later adds logic to only one of them.If the intent is purely a naming alias, a brief inline comment like
// Semantic alias – keep in sync with findMinimumBywould help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Util.ts` around lines 102 - 116, findClosestBy currently just forwards to findMinimumBy and should be documented as an intentional semantic alias to avoid future drift; update the findClosestBy declaration to include a short inline comment like "// Semantic alias of findMinimumBy — keep behavior/signature in sync" (or alternatively remove the alias and replace call sites with findMinimumBy if duplication isn't desired), referencing the functions findClosestBy and findMinimumBy so readers know this is deliberate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 1183-1193: warshipSpawn currently accesses this.mg repeatedly;
mirror the nukeSpawn optimization by caching this.mg into a local const (e.g.,
const mg = this.mg) at the top of the warshipSpawn method and then use
mg.isOcean and mg.manhattanDist in the findClosestBy callback (leave
findClosestBy, units(UnitType.Port), and UnitType.Port unchanged) so lookups are
consistent and slightly faster on hot paths.
In `@src/core/Util.ts`:
- Around line 102-116: findClosestBy currently just forwards to findMinimumBy
and should be documented as an intentional semantic alias to avoid future drift;
update the findClosestBy declaration to include a short inline comment like "//
Semantic alias of findMinimumBy — keep behavior/signature in sync" (or
alternatively remove the alias and replace call sites with findMinimumBy if
duplication isn't desired), referencing the functions findClosestBy and
findMinimumBy so readers know this is deliberate.
…or map "ocean_and_land" which lead to NaN as outcome of manhattanDist. Until now we used distSortUnit which just returned the first and only port it was given in the test, even though manhattanDist both returned NaN. Now with findClosestBy we need it to have actual existing tiles because we need actual comparable number to come out of manhattanDist.) Also for TradeShipExecution use some already cached values as input for findClosestBy.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR introduces a single-pass “closest/minimum” selection utility to replace repeated sort(...)[0] patterns in hot paths, improving performance and (in some cases) tightening candidate filtering to avoid inactive/invalid spawn sources.
Changes:
- Added
findMinimumBy/findClosestByhelpers to select the best element without sorting. - Migrated several “pick nearest unit” call sites (core + client) from sort-based selection to
findClosestBy. - Tightened port/silo candidate selection in core logic and updated TradeShip tests to use valid
TileRefs and new unit predicates.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/Util.ts |
Adds findMinimumBy/findClosestBy single-pass selection helpers. |
src/core/game/PlayerImpl.ts |
Uses findClosestBy for upgrade/spawn selection; adds additional candidate checks for silos/ports. |
src/core/execution/TradeShipExecution.ts |
Replaces sort-based nearest-port selection with findClosestBy and adds candidate filtering. |
tests/core/executions/TradeShipExecution.test.ts |
Updates mocks and tile refs to support new candidate filtering and avoid NaN distances. |
src/client/graphics/layers/RadialMenuElements.ts |
Uses findClosestBy to pick nearest deletable unit. |
src/client/ClientGameRunner.ts |
Uses findClosestBy to pick nearest upgrade candidate without sorting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
github-project-automation
bot
moved this from Complete
to Triage
in OpenFront Release Management
github-project-automation
bot
moved this from Triage
to Complete
in OpenFront Release Management
github-project-automation
bot
moved this from Complete
to Triage
in OpenFront Release Management
would be nice to get what percentage of total cpu time was spent on that function. Otherwise we don't know if this is in the hot path or not |
…istingUnitToUpgrade
@evanpelle Do the newly added screenshots in the PR description help you in assessing this? Used a replay and played it on main and on this branch, there are small differences in recording time and game speed though but it comes closest to a good comparison. The replay was done with 60 Nations on World map. Nations call nukeSpawn and warshipSpawn from their executions. You see maybeSendNuke in the screenshots which calls canBuild which calls nukeSpawn. I imagine the more Nations (up to 400 now in v30, compared to the 60 in this replay) the more their Executions check to send Warships and especially Nukes, it could be seen as a hotpath. And as already shown in the screenshots in the earlier comment, when spamming Warships as the human player and hovering an Atom bomb over the map which calls nukeSpawn for each tile, we also get a nice - albeit small in terms of ms - reduction. |
Description:
Performance improvements.
Same for nukeSpawn silos, don't return inactive silo just to be sure now that we can easily add it to isCandidate predicate anyway. This costs no performance in the nukeSpawn benchmarks actually. This should as a by-effecft fix an edge case bug i have seen, where a nuke is sent from a phantom silo.
Some of this goes along with PR #3220 since playerImpl buildableUnits makes use of the underlying spawn functions via canBuild. Just like ConstructionExecution does. But i didn't want to add more to PR 3220 since there's already a lot in there.
The new function findClosestBy could also be applied to some other parts of code to benefit of it being faster, so i did that.
findClosestBy uses findMinimumBy, which is a little more generic in name. I think findMinimumBy could be used by other parts of code, while findClosestBy is more clear naming for what it does now. But we could ditch findMinimumBy and just leave findClosestBy?
Examples of synthetic benchmarks (not included in this PR):
BEFORE CHANGES (before Scamiv's PR #3241)
AFTER CHANGES (before Scamiv's PR #3241)
BEFORE CHANGES (after Scamiv's PR #3241)
AFTER CHANGES (after Scamiv's PR #3241)
Also see more BEFORE and AFTER in this comment:
#3243 (comment)
And here a comparison in the flame charts:
BEFORE flame chart with nukeSpawn (human player) and maybeSendNuke (Nation players, uses nukeSpawn via canBuild):
BEFORE flame chart with nukeSpawn (human player) and maybeSendNuke (Nation players, uses nukeSpawn via canBuild):
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33