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.

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

Ai improve large map performance #4764

Merged
merged 10 commits into from Mar 14, 2019

Conversation

Projects
None yet
3 participants
@ron-murhammer
Copy link
Member

ron-murhammer commented Mar 13, 2019

Overview

Functional Changes

  • Decrease random start delegate pause time from 250ms to 10ms
  • Add new get neighbors ignoring end methods to reduce trying to find routes for sea territories that either don't have a valid route or have a very long route
  • Update territory value utils to avoid ever checking every territory on the map and always pass in what territories to check. This essentially makes AI calculations more 'regional' instead of 'global' so that the AI only calculates things where it can move/place units. An example is avoid having Japan calculate a bunch of territory values for things in the European Theater when it doesn't have any units that can reach that.

Manual Testing Performed

  • Did functional and performance testing on World of War Heroes alpha, WaW, Dom NML, and Another World. Most large maps with Fast AI are about 2x as fast now but World of War or other sparse large maps are 5x-10x faster.
@@ -190,7 +192,7 @@ public Territory getTerritory(final String s) {

private Set<Territory> getNeighbors(final Set<Territory> frontier, final Set<Territory> searched, final int distance,
@Nullable final Predicate<Territory> cond) {
if (distance == 0) {
if (distance == 0 || frontier.isEmpty()) {

This comment has been minimized.

Copy link
@ron-murhammer

ron-murhammer Mar 13, 2019

Author Member

Add empty check here as once the frontier is empty, you won't ever find any more territories to search.

@@ -49,13 +49,6 @@ public static double findTerritoryAttackValue(final PlayerId player, final Terri
return value;
}

public static Map<Territory, Double> findTerritoryValues(final PlayerId player,

This comment has been minimized.

Copy link
@ron-murhammer

ron-murhammer Mar 13, 2019

Author Member

This is one of the main keys of this PR was to remove this method and refactor areas that used it as its very slow on large maps.

@@ -92,12 +84,12 @@ public static double findTerritoryAttackValue(final PlayerId player, final Terri
* Returns the value of each sea territory in {@link ProData#getData()}.
*/
public static Map<Territory, Double> findSeaTerritoryValues(final PlayerId player,
final List<Territory> territoriesThatCantBeHeld) {
final List<Territory> territoriesThatCantBeHeld, final List<Territory> territoriesToCheck) {

This comment has been minimized.

Copy link
@ron-murhammer

ron-murhammer Mar 13, 2019

Author Member

Always pass in a list of territories to check to avoid checking every territory on the map.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 13, 2019

Codecov Report

Merging #4764 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4764      +/-   ##
============================================
- Coverage     23.09%   23.06%   -0.03%     
- Complexity     6156     6157       +1     
============================================
  Files           868      868              
  Lines         70241    70325      +84     
  Branches      11225    11243      +18     
============================================
  Hits          16222    16222              
- Misses        51988    52072      +84     
  Partials       2031     2031
Impacted Files Coverage Δ Complexity Δ
...a/games/strategy/triplea/ai/pro/util/ProUtils.java 2.12% <ø> (ø) 8 <0> (ø) ⬇️
...a/games/strategy/triplea/ai/pro/ProPurchaseAi.java 0.31% <0%> (-0.02%) 1 <0> (ø)
...strategy/triplea/delegate/RandomStartDelegate.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...es/strategy/triplea/ai/pro/ProNonCombatMoveAi.java 0.21% <0%> (-0.01%) 1 <0> (ø)
...games/strategy/triplea/ai/pro/ProCombatMoveAi.java 0.35% <0%> (-0.01%) 1 <0> (ø)
.../main/java/games/strategy/engine/data/GameMap.java 59.23% <0%> (-8.89%) 43 <8> (ø)
...gy/triplea/ai/pro/util/ProTerritoryValueUtils.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...java/games/strategy/triplea/delegate/DiceRoll.java 64.42% <0%> (-0.32%) 139% <0%> (-1%)
.../org/triplea/lobby/server/EnvironmentVariable.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
.../strategy/triplea/odds/calculator/DummyPlayer.java 35.95% <0%> (+1.12%) 10% <0%> (+1%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f774589...9687fad. Read the comment docs.

@@ -302,7 +294,8 @@ private static double findWaterValue(final Territory t, final PlayerId player, f

// Determine value based on nearby territory production
double nearbyLandValue = 0;
final Set<Territory> nearbyTerritories = data.getMap().getNeighbors(t, 3);
final Set<Territory> nearbyTerritories =
data.getMap().getNeighborsIgnoreEnd(t, 3, ProMatches.territoryCanMoveSeaUnits(player, data, true));

This comment has been minimized.

Copy link
@ron-murhammer

ron-murhammer Mar 13, 2019

Author Member

This change is important because often you have cases where 2 sea territories where within 3 hops but they either aren't connected by sea territories at all or the route over sea territories is very long.

@ron-murhammer

This comment has been minimized.

Copy link
Member Author

ron-murhammer commented Mar 13, 2019

This pull request introduces 1 alert when merging afd24d2 into f774589 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

Comment posted by LGTM.com

@DanVanAtta

This comment has been minimized.

Copy link
Member

DanVanAtta commented Mar 13, 2019

Looks good @ron-murhammer . Left a few comments/suggestions, let me know what you think and when done iterating.

@ron-murhammer

This comment has been minimized.

Copy link
Member Author

ron-murhammer commented Mar 13, 2019

@DanVanAtta Addressed comments and made some updates. If you want to take a look at where the performance is slow and potentially where parallel streams could help it would be in some of the territory value util methods like this ProTerritoryValueUtils.findWaterValue: https://github.com/triplea-game/triplea/pull/4764/files#diff-0f55e4b4525775beb799afb6b97cc2b9L273. This is primarily slow because route and neighbor finding over larger distances is slow.

@ron-murhammer

This comment has been minimized.

Copy link
Member Author

ron-murhammer commented Mar 14, 2019

This pull request introduces 1 alert when merging d3e34b3 into f774589 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

Comment posted by LGTM.com

@ron-murhammer

This comment has been minimized.

Copy link
Member Author

ron-murhammer commented Mar 14, 2019

This pull request introduces 1 alert when merging a750a67 into f774589 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

Comment posted by LGTM.com

@DanVanAtta

This comment has been minimized.

Copy link
Member

DanVanAtta commented Mar 14, 2019

@ron-murhammer any idea if the LGTM alert is valid or should be suppressed?

This is looking pretty updated/addressed, I think that may be the last thing.

@ron-murhammer

This comment has been minimized.

Copy link
Member Author

ron-murhammer commented Mar 14, 2019

@DanVanAtta That is actually valid and a really good catch. Fixed. +1 for LGTM :)

@ron-murhammer ron-murhammer removed their assignment Mar 14, 2019

@DanVanAtta DanVanAtta merged commit 93d1a34 into master Mar 14, 2019

3 checks passed

LGTM analysis: Java No new or fixed alerts
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@DanVanAtta DanVanAtta deleted the AI_Improve_Large_Map_Performance branch Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.