Path finding optimizations. Skip (15%) closed cells early. #19245

anvilvapre · 2021-03-14T19:05:18Z

In path finding GetConnections considers connections to already closed cells and calculates the cost for them. The caller afterwards ignores them. These are 15% of all connections.

Add CPos.ToOffset to avoid MPos allocation in CellLayer.

Additional small changes with minimal performance benefit which I can imagine you won't find 'pretty'. Feel free to reject.

Use PathGraph directly over IGraph to allow non virtual index operator access. There will likely be no alternative implementations - and switching back is easy once there are.

Copies CellInfo to a local variable. The trade off between copying CellInfo versus calculating a cell index offset - and accessing an array twice.

Tested by running a replay of 20 mins.

Suggestion: remove most properties in IGraph and IPathSearch and pass them as constructor variables i.e. PathQuery as they are often only set once and not used outside the implementing classes.

abcdefg30 · 2021-03-14T19:13:17Z

OpenRA.Game/Map/CellLayer.cs

-		// Resolve an array index from cell coordinates
-		int Index(CPos cell)
-		{
-			return Index(cell.ToMPos(GridType));


Can't we do here what you currently do in ToOffset? I'm not sure if we want to expose that ToOffset logical to everything when it is only used here.

I first implemented it there. But then it seemed cleaner to keep all ToMPos logic in CPos - not spread duplicated code. ToOffset also seemed generic enough. I don't mind where.

pchote · 2021-03-14T20:34:35Z

OpenRA.Game/CPos.cs

@@ -42,6 +42,7 @@ public CPos(int x, int y, byte layer)
 		}

 		public static readonly CPos Zero = new CPos(0, 0, 0);
+		public static readonly CPos Invalid = new CPos(-2048, -2048, 255);


Using magic values on CPos has caused many headaches in the past. Could we instead change CPos IPathSearch.Expand to bool IPathSearch.TryExpand(out CPos) and avoid this completely?

pchote · 2021-03-14T20:36:52Z

I would support removing IGraph and IPathSearch completely. These were introduced a long time ago by someone who was trying to push for unit testing everywhere, but this did not end well and most things except for these were eventually removed.

teinarss · 2021-03-21T18:40:14Z

OpenRA.Mods.Common/Pathfinder/PathGraph.cs

+		// PERF: Use fields over virtual properties
+		readonly Actor actor;
+		readonly World world;
+		Func<CPos, bool> customBlock;
+		Func<CPos, int> customCost;
+		int laneBias;
+		bool inReverse;
+		Actor ignoreActor;


I did some testing and it looks like the jit will inline these correctly.

using System; public interface Apa { int Prop { get; } } public class C : Apa { public int Prop { get; } = 10; private int field = 10; public int PropAccess() { var x = Prop + 10; return x; } public int FieldAccess() { var x = field + 10; return x; } }

; Core CLR v5.0.421.11614 on x86 C..ctor() L0000: mov dword ptr [ecx+4], 0xa L0007: mov dword ptr [ecx+8], 0xa L000e: ret C.get_Prop() L0000: mov eax, [ecx+4] L0003: ret C.PropAccess() L0000: mov eax, [ecx+4] L0003: add eax, 0xa L0006: ret C.FieldAccess() L0000: mov eax, [ecx+8] L0003: add eax, 0xa L0006: ret

Also in class `class D : C'? Also when there is a setter? Because with a setter i can imagine it no longer 'dares' to?
I read somewhere C# wouldn't/couldn't. Good to know.

In both those cases its able to inline it proper.

So shall I, some time later, remove both interfaces and change the fields back to properties? Or only the last.

OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs

teinarss · 2021-04-04T17:38:05Z

OpenRA.Mods.Common/Pathfinder/CellInfoLayerPool.cs

@@ -15,7 +15,7 @@

 namespace OpenRA.Mods.Common.Pathfinder
 {
-	sealed class CellInfoLayerPool
+	public class CellInfoLayerPool


think it best to keep it sealed.

Not possible as Graph is public that is used by a PathSearch which is also public.

public sealed

teinarss · 2021-04-04T17:41:12Z

OpenRA.Mods.Common/Pathfinder/PathCacheStorage.cs

+		UnitPathToRange
+	}
+
+	public struct PathCacheKey


this should be readonly othewise a copy will be made when using the in param, see https://docs.microsoft.com/en-us/dotnet/csharp/write-safe-efficient-code

teinarss · 2021-04-04T17:44:56Z

OpenRA.Mods.Common/Pathfinder/PathGraph.cs

+		// PERF: Use fields over virtual properties
+		readonly Actor actor;
+		readonly World world;
+		Func<CPos, bool> customBlock;
+		Func<CPos, int> customCost;
+		int laneBias;
+		bool inReverse;
+		Actor ignoreActor;


anvilvapre · 2021-04-06T16:15:59Z

Additional changes in the last push:

As requested I've refactored BasePathSearch, PathSearch from a semi builder pattern - where input and state was spread across multiple classes - to a simpler model.

BasePathSearch and PathGraph now take a PathQuery as constructor argument that defines the search to perform. Leaving only the state to be maintained in BasePathSearch itself. This should allow for alternative implementations more easily in future. PathQuery unfortunately has many fields.

While making changes it seemed to no longer make sense to keep the PathSearch.FromPoints and Search functions. FromPoints became PathQueryType.Position*idirectional and Search PathQueryType.ConditionUnidirectional.

Removed BasePathSearch.StartsPoints as it was not used.

IPathSearch and IPathGraph have been removed as requested. BasePathSearch and PathSearch could now also be optionally merged. Although it may make more sense to move PathSearch.TryExpand to PathGraph rather than BasePathSearch.

Pulled some of the calculation in some CustomCost functions for path searching outside the functions.

Note that by not adding self.Location twice as start point for path searches results in rare conditions in a slightly different path. Which impacts Sync. This is correct.

No functional changes (intended). Apart from the impact on sync mentioned above.

Tested by running a 7 minute replay that was recorded on bleed (with the one change that impacts sync temporary disabled).

teinarss · 2021-04-06T17:29:35Z

OpenRA.Mods.Common/Activities/FindAndDeliverResources.cs

@@ -162,12 +162,14 @@ public override bool Tick(Actor self)
 			// Harvesters should respect an explicit harvest order instead of harvesting the current cell.
 			if (orderLocation == null)
 			{
-				if (harv.CanHarvestCell(self, self.Location) && claimLayer.CanClaimCell(self, self.Location))
+				if (harv.CanHarvestCell(self, self.Location)
+						&& claimLayer.CanClaimCell(self, self.Location))


Only formatting changes?

teinarss · 2021-04-06T17:29:49Z

OpenRA.Mods.Common/Activities/FindAndDeliverResources.cs

 					return self.Location;
 			}
 			else
 			{
-				if (harv.CanHarvestCell(self, orderLocation.Value) && claimLayer.CanClaimCell(self, orderLocation.Value))
+				if (harv.CanHarvestCell(self, orderLocation.Value)
+						&& claimLayer.CanClaimCell(self, orderLocation.Value))


teinarss · 2021-04-07T11:59:19Z

OpenRA.Mods.Common/Traits/Harvester.cs

+				.Where(r => r.Actor != ignore
+					&& r.Actor.Owner == self.Owner
+					&& IsAcceptableProcType(r.Actor))


Please keep this in one line.

.github/workflows/ci.yaml

teinarss · 2021-04-08T15:21:41Z

OpenRA.Mods.Common/Traits/World/ActorMap.cs

-			var layer = a.Layer == 0 ? influence : customInfluence[a.Layer];
+			var layerIndex = a.Layer;
+			var layer = layerIndex == 0 ? influence : customInfluence[layerIndex];


Fun fact: It did look like the jit compiler was able to do this optimization by itself.

Should we drop these then?

pchote

A couple of small comments i noticed while skimming the commit below.

My overall feedback is that this is trying to do way too many different things within a single commit. This makes it very difficult to understand how the different changes relate to eachother and overall difficult to review. Can you please split this up into individual commits (they can all still be in this PR) for each logicial change?

e.g. introducing PathQuery should have its own commit that switches the code to the new class without changing any behaviour, the harvester pathfinding improvements should have another, the different micro-optimizations each get their own, and so on.

pchote · 2021-04-08T15:35:27Z

OpenRA.Game/CPos.cs

@@ -85,6 +85,11 @@ public MPos ToMPos(MapGridType gridType)
 			return new MPos(u, v);
 		}

+		public CPos AtLayer(byte layer)


Can we please call this ToLayer?

Yes. But it will look similar to ToMPos.

Ah, thats a good point. How about WithLayer then.

AtLayer sounds like it should be returning true/false if the given layer matches...

pchote · 2021-04-08T15:37:57Z

OpenRA.Game/Primitives/PriorityQueue.cs


-			while (addLevel >= 1 && comparer.Compare(Above(addLevel, addIndex), item) > 0)
+			while (addLevel >= 1 && comparer.Compare((above = Above(addLevel, addIndex)), item) > 0)


We generally try to avoid using inline assignments (not sure the correct term) like this. Can we restructure the loop so that above is assigned on its own line?

pchote · 2021-04-08T15:38:56Z

OpenRA.Game/Primitives/PriorityQueue.cs

+		{
+			if (level == 0)
+			{
+				item = default(T);


This can now be just item = default;, likewise in TryPop.

pchote · 2021-04-08T15:40:19Z

OpenRA.Game/Primitives/PriorityQueue.cs

+
+		public bool TryPop(out T item)
+		{
+			if (level == 0)


Pop also checks index. Is it safe to ignore that here?

Empty only uses level. So I assumed it would be safe.

pchote · 2021-04-08T17:16:55Z

Can you please try to add the changes that use ToLayer, TryPeek, TryPop into the same commits that add them? The value in splitting things into different commits is that we can see each set of changes in isolation and the evolution of the code, rather than have a diff that simply swaps one implementation for another.

anvilvapre · 2021-04-08T17:58:25Z

Can you please try to add the changes that use ToLayer, TryPeek, TryPop into the same commits that add them? The value in splitting things into different commits is that we can see each set of changes in isolation and the evolution of the code, rather than have a diff that simply swaps one implementation for another.

I basically can't. I rebased everything quite often because I thought you wanted it. I made changes before and after committing PathQuery and they spread multiple files. After i renamed PathQuery where it first had a different name. Evolution took side paths ; )

I agree PathQuery could have better been in a separate pull request. Should have suggested this.

Let me one day find the patience to reapply all changes manually across files and turn them into multiple pull requests as well.

anvilvapre · 2021-04-08T19:31:05Z

Current split up seems the most fine grained I can make it without having to rewrite code and unravel multiple changes per file even per line. Started with a mergetool but it really means rewriting all changes manually.

Note that CPos.WithLayer as a change is not related to PriorityQueue.TryPop. They are to optimize PathGraph and PathSearch independantly.

pchote · 2021-04-08T19:38:16Z

Let me one day find the patience to reapply all changes manually across files and turn them into multiple pull requests as well.

This shouldn't be necessary!

Here is the workflow I usually use in this situation, which is faster in practice than my list might imply:

Check out a duplicate copy of the branch (e.g. pathfinder-temp
Decide on what subset of changes I want to go in the first commit
Reset all changes that aren't related to those changes (using a graphical tool like gitg makes it a lot easier to unstage individual lines)
Fix any compile errors and test that the subset of features works as intended
Create the first commit
Switch back to the first branch and rebase on the temp branch. Resolve any rebase issues and if needed test anything that you suspect might have regressed.
Repeat steps 1-5 as many times as needed.

A lot of the changes end up being obviously distinct, which means that you can make several commits in a single pass. Its only the core parts where different features overlap where the full loop is needed.

anvilvapre · 2021-04-09T15:51:38Z

A lot of the changes end up being obviously distinct, which means that you can make several commits in a single pass. Its only the core parts where different features overlap where the full loop is needed.

It was the process that i started. I've extracted the distinct. Because I changed a lot of class field members I can no longer easily pick distinct changes without breaking the code.

It means that the last commit now contains three changes: PathQuery, a one liner for skipping 15% of closed cells early and some optimizations for custom goals functions that were passed to path searches.

AspectInteractive · 2021-08-03T12:39:58Z

IPathSearch and IPathGraph have been removed as requested.

This seems reasonable since BasePathSearch is the only class that uses these interfaces.

BasePathSearch and PathSearch could now also be optionally merged.

I am quite new and not that familiar with the code base, but would this not prevent alternative PathSearch implementations from being used? I can understand removing the interface, as it has many implementation details that are unlikely to be used by a second class, however BasePathSearch appears general enough - or at least should be - to take several implementations. PathSearch directly implements the A* Algorithm, meaning all pathfinding would need to use this A* algorithm from this point onwards. Given that pathfinding is still not an entirely solved problem, I don't think hardcoding the A* algorithm here (or any specific pathfinding algorithm) is a wise choice.

pchote · 2021-08-03T13:02:54Z

Activities are currently hardcoding PathSearch directly, so even with all the interfaces it wasn't possible to switch out any implementations. If we want to fix this, this can be done by adding path search wrappers to IPathfinder and changing activities to call these instead.

I'm not convinced we win much by fixing this, however, since these activities all make strong assumptions about the cell and pathfinding system and likely need to be completely replaced by any mod that wants to change the way the grid movement system works.

AspectInteractive · 2021-08-03T13:08:42Z

I'm not convinced we win much by fixing this, however, since these activities all make strong assumptions about the cell and pathfinding system and likely need to be completely replaced by any mod that wants to change the way the grid movement system works.

There are still other grid-based path finders you may wish to use in the future. A basic implementation of the flow field algorithm for instance is compatible with grid-based movement. For this reason I agree with your initial point of fixing the iPathFinder interface (and eventually decoupling Activities) rather than removing it entirely.

penev92 · 2021-08-10T12:08:11Z

I believe this is the original/base PR that is getting split over multiple others with the intent of getting closed in the end so to mitigate (my own) confusion I'll convert this into a Draft PR for the time being.

anvilvapre · 2021-09-24T13:49:50Z

Closing to reduce number of open PRs. This pull request was split up in many smaller ones. Some merged, some waiting for approval.

abcdefg30 reviewed Mar 14, 2021

View reviewed changes

pchote reviewed Mar 14, 2021

View reviewed changes

anvilvapre force-pushed the 20210302_pathfinder branch from 84b9cf9 to cef16f0 Compare March 14, 2021 21:55

teinarss reviewed Mar 22, 2021

View reviewed changes

teinarss reviewed Apr 4, 2021

View reviewed changes

anvilvapre mentioned this pull request Apr 6, 2021

FindAndDeliverResources, do not add self location twice to path search. #19268

Closed

anvilvapre force-pushed the 20210302_pathfinder branch from cef16f0 to b0d0c6a Compare April 6, 2021 15:31

anvilvapre force-pushed the 20210302_pathfinder branch from b0d0c6a to a47816f Compare April 6, 2021 17:21

teinarss reviewed Apr 7, 2021

View reviewed changes

anvilvapre force-pushed the 20210302_pathfinder branch 3 times, most recently from 14de522 to 530700b Compare April 7, 2021 21:01

reaperrr reviewed Apr 7, 2021

View reviewed changes

.github/workflows/ci.yaml Show resolved Hide resolved

teinarss reviewed Apr 8, 2021

View reviewed changes

pchote reviewed Apr 8, 2021

View reviewed changes

pchote added the PR: Changes requested label Apr 8, 2021

anvilvapre added 7 commits April 8, 2021 18:46

Fix github Windows build issue.

6001578

CPos.ToLayer.

af08ae7

CellLayer inline CPos.ToMPos.

d5ff38a

PathCache use PathCacheKey over string as key.

e8cfeda

PriorityQueue, TryPeek, TryPop.

a4f69e7

Do no lookup cell layer twice.

a9eccef

Avoid unnecessary cast.

d2d56f1

anvilvapre force-pushed the 20210302_pathfinder branch from 530700b to a2c9125 Compare April 8, 2021 17:06

anvilvapre force-pushed the 20210302_pathfinder branch from 8a2222c to e70198a Compare April 8, 2021 17:40

Split PathSearch in PathQuery and PathSearch.

8963ac3

anvilvapre force-pushed the 20210302_pathfinder branch from e70198a to 8963ac3 Compare April 8, 2021 20:34

PriorityQueue, minor optimizatoin.

9e4b251

teinarss mentioned this pull request Jun 28, 2021

Better path finder #19470

Closed

penev92 marked this pull request as draft August 10, 2021 12:08

penev92 added the PR: Has dependencies label Aug 10, 2021

anvilvapre closed this Sep 24, 2021


		while (addLevel >= 1 && comparer.Compare(Above(addLevel, addIndex), item) > 0)
		while (addLevel >= 1 && comparer.Compare((above = Above(addLevel, addIndex)), item) > 0)

Path finding optimizations. Skip (15%) closed cells early. #19245

Path finding optimizations. Skip (15%) closed cells early. #19245

Conversation

anvilvapre commented Mar 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote commented Mar 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anvilvapre commented Apr 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote commented Apr 8, 2021

anvilvapre commented Apr 8, 2021

anvilvapre commented Apr 8, 2021

pchote commented Apr 8, 2021

anvilvapre commented Apr 9, 2021 • edited

AspectInteractive commented Aug 3, 2021 • edited

pchote commented Aug 3, 2021 • edited

AspectInteractive commented Aug 3, 2021 • edited

penev92 commented Aug 10, 2021

anvilvapre commented Sep 24, 2021

pchote commented Mar 14, 2021 •

edited

anvilvapre commented Apr 9, 2021 •

edited

AspectInteractive commented Aug 3, 2021 •

edited

pchote commented Aug 3, 2021 •

edited

AspectInteractive commented Aug 3, 2021 •

edited