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

Cache ITargetablePositions in Actor to speed up Target.Positions #14913

Merged
merged 1 commit into from Apr 7, 2018

Conversation

Projects
None yet
4 participants
@reaperrr
Copy link
Contributor

reaperrr commented Mar 14, 2018

According to #13547 (comment), together with Production and 3 movement-related traits ITargetablePositions is one of the 5 most-looked-up traits (via Target.Positions, which on bleed performs a TraitsImplementing<ITargetablePositions>().(Exts.IsTraitEnabled) every time it is called).

Caching it on Actor should reduce those trait look-ups.

{
return TargetablePositionTraits.Where(Exts.IsTraitEnabled).ToArray();
}

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 16, 2018

Member

I would probably ditch this method - it's not doing anything the caller can't do.

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 16, 2018

Member

Also food for thought - the ToArray here might not be worthwhile - Any (inside the Positions property) isn't likely to enumerate the whole sequence. If I'm wrong and avoiding two enumerations is important - we're probably better dropping to a raw loop and avoiding LINQ.

@@ -51,6 +51,7 @@ internal struct SyncHash
public IEffectiveOwner EffectiveOwner { get; private set; }
public IOccupySpace OccupiesSpace { get; private set; }
public ITargetable[] Targetables { get; private set; }
public ITargetablePositions[] TargetablePositionTraits { get; private set; }

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 16, 2018

Member

Maybe name it TargetablePositions for consistancy with the other cached traits?

@reaperrr reaperrr force-pushed the reaperrr:cache-ITarPos branch from 42b2eee to dca4991 Mar 16, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 17, 2018

Updated.

@reaperrr reaperrr force-pushed the reaperrr:cache-ITarPos branch from dca4991 to aac015d Mar 18, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 18, 2018

Updated.

I came up with an idea that should (in theory) help even more, namely caching all targetable positions if the actor is immobile (no IMoveInfo) and all ITargetablePositions are always enabled (i.e. don't require any condition).
This should further improve performance when looking up positions of buildings and map decoration.

Unfortunately this invalidates @RoosterDragon's previous approval.

@reaperrr reaperrr removed the PR: Needs +2 label Mar 18, 2018

@@ -73,6 +73,8 @@ protected override void Created(Actor self)
base.Created(self);
}

bool ITargetablePositions.AlwaysEnabled { get { return Info.EnabledByDefault; } }

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 18, 2018

Member

EnabledByDefault doesn't necessarily mean always enabled - surly?

This comment has been minimized.

@reaperrr

reaperrr Mar 18, 2018

Author Contributor

At first I was wondering about that, too, but the base ConditionalTrait currently sets

EnabledByDefault = RequiresCondition == null || RequiresCondition.Evaluate(VariableExpression.NoVariables);

and HitShape doesn't override that. So at least in this context it should be correct.

This comment has been minimized.

@pchote

pchote Mar 18, 2018

Member

Please don't do this! EnabledByDefault is a horrible hack around the code not yet supporting arbitrary starting conditions (via ConditionInit). We really don't want to make our low level abstractions depend on this broken code.

This comment has been minimized.

@reaperrr

reaperrr Mar 18, 2018

Author Contributor

I see. Would it be ok to check

RequiresCondition == null || RequiresCondition.Evaluate(VariableExpression.NoVariables)

here directly?

This comment has been minimized.

@pchote

pchote Mar 18, 2018

Member

The null check is fine, but the .Evaluate isn't. The null check by itself should satisfy your goal here, I think?

@reaperrr reaperrr force-pushed the reaperrr:cache-ITarPos branch from aac015d to fc0f5e2 Mar 18, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 18, 2018

Updated.

@@ -366,6 +379,18 @@ public bool IsTargetableBy(Actor byActor)
return false;
}

public WPos[] GetTargetablePositions()

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 19, 2018

Member

It might be nice if this was still a lazily generated enumerable - Target.IsInRange uses Any for example.

This comment has been minimized.

@reaperrr

reaperrr Mar 20, 2018

Author Contributor

I have no experience using Lazy, so I'm not sure what the exact code changes would need to look like.

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 22, 2018

Member

Sorry, I simply mean:

  • Return IEnumerable<WPos>
  • Remove the two ToArray()s inside this method.

If the caller needs to enumerate the lot anyway, it can do so itself. If it can bail out partway - we've saved some work since we didn't need to enumerate the lot in order to shove it into an array.

This comment has been minimized.

@reaperrr

reaperrr Mar 22, 2018

Author Contributor

Oh, ok.

onlyStaticTargetablePositions = true;
staticTargetablePositions = targetablePositions.SelectMany(tp => tp.TargetablePositions(this)).ToArray();
}
});

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 19, 2018

Member

If we're going to deal in providing actual positions, I wonder if you can streamline the caching to focus on just that

  • Define IEnumerable<WPos> targetablePositions;
  • Set targetablePositions = TraitsImplementing<ITargetablePositions>().ToArray().Where(Exts.IsTraitEnabled).HandleTheAnyCheckThatOtherwiseReturnsTheCenterSomehow().SelectMany(tp => tp.TargetablePositions(this));
  • In the frame task, if the "won't ever change" condition is met, set targetablePositions = targetablePositions.ToArray();

You could then expose targetablePositions as a public property - and you don't need the other fields to track stuff or the method.

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 20, 2018

Member

If I'm drinking the kool-aid and the above suggestion is crazy - could you at least remove onlyStaticTargetablePositions and simply do null checks on staticTargetablePositions instead?

This comment has been minimized.

@reaperrr

reaperrr Mar 20, 2018

Author Contributor

I wouldn't call it crazy, but besides HandleTheAnyCheckThatOtherwiseReturnsTheCenterSomehow() there's also the issue that if the "won't ever change" condition is not met, we'd need to update TargetablePositions when called, I don't see how that's supposed to work if we drop both the boolean and the method.

@reaperrr reaperrr force-pushed the reaperrr:cache-ITarPos branch from fc0f5e2 to bec7da3 Mar 22, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 22, 2018

Updated, but only applied

If I'm drinking the kool-aid and the above suggestion is crazy - could you at least remove onlyStaticTargetablePositions and simply do null checks on staticTargetablePositions instead?

If I can get my head around the other suggestion or someone else feels like taking that on (whichever comes first), we can still do that in a follow-up.

@reaperrr reaperrr force-pushed the reaperrr:cache-ITarPos branch from bec7da3 to f1c0e97 Mar 22, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 22, 2018

Updated, applied

  • Return IEnumerable<WPos>
  • Remove the two ToArray()s inside this method.

too.

@@ -118,6 +120,13 @@ internal Actor(World world, string name, TypeDictionary initDict)
visibilityModifiers = TraitsImplementing<IVisibilityModifier>().ToArray();
defaultVisibility = Trait<IDefaultVisibility>();
Targetables = TraitsImplementing<ITargetable>().ToArray();
targetablePositions = TraitsImplementing<ITargetablePositions>().ToArray();
this.World.AddFrameEndTask(w =>

This comment has been minimized.

@pchote

pchote Mar 23, 2018

Member

Minor style nit: this could be world.AddFrameEndTask

targetablePositions = TraitsImplementing<ITargetablePositions>().ToArray();
this.World.AddFrameEndTask(w =>
{
// Caching this in a AddFrameEndTask, because trait construction order might cause problems if done directly at creation time

This comment has been minimized.

@pchote

pchote Mar 23, 2018

Member

Are we sure that this won't cause problems with any traits that try to query targetable positions during their own creation?

This comment has been minimized.

@reaperrr

reaperrr Mar 23, 2018

Author Contributor

I'm not, but I'm getting an NRE referring to HitShape line 89 when I try to do this outside an FET.

Edit: Which to me indicates that "traits that try to query targetable positions during their own creation" would run into problems on bleed, too.

this.World.AddFrameEndTask(w =>
{
// Caching this in a AddFrameEndTask, because trait construction order might cause problems if done directly at creation time
if (!this.Info.HasTraitInfo<IMoveInfo>() && targetablePositions.All(tp => tp.AlwaysEnabled))

This comment has been minimized.

@pchote

pchote Mar 23, 2018

Member

Can drop the this here too.

This comment has been minimized.

@pchote

pchote Mar 23, 2018

Member

Can we get a comment explaining the significance of the IMoveInfo here?

if (staticTargetablePositions != null)
return staticTargetablePositions;

var enabledTarPosTraits = targetablePositions.Where(Exts.IsTraitEnabled);

This comment has been minimized.

@pchote

pchote Mar 23, 2018

Member

Style nit: please use full words instead of trimming off letters: enabledTargetablePositions isn't much longer, but is much more readable.

Cache TargetablePositions in Actor
To speed up Target.Positions.

@reaperrr reaperrr force-pushed the reaperrr:cache-ITarPos branch from f1c0e97 to 23983a2 Mar 23, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 23, 2018

Updated.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 30, 2018

@RoosterDragon can you re-review this?

@pchote

pchote approved these changes Apr 7, 2018

Copy link
Member

pchote left a comment

LGTM

@pchote pchote merged commit 0c52ff3 into OpenRA:bleed Apr 7, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@reaperrr reaperrr deleted the reaperrr:cache-ITarPos branch May 7, 2018

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.