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

Minor performance optimizations for InflictDamage/kills #15629

Merged
merged 6 commits into from Sep 29, 2018

Conversation

Projects
None yet
4 participants
@reaperrr
Copy link
Contributor

reaperrr commented Sep 20, 2018

I don't expect any of this to make a noticable difference, but combined it might still help a little bit in reducing #9142.

See individual commit messages for more details where the code might not be immediately self-explanatory.

@reaperrr reaperrr force-pushed the reaperrr:inflict-dmg-perf branch from f9d9b5e to d260b69 Sep 20, 2018

@Mailaender
Copy link
Member

Mailaender left a comment

Looks sane.


void INotifyCreated.Created(Actor self)
{
defenderStats = self.Owner.PlayerActor.Trait<PlayerStatistics>();

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 20, 2018

Member

Does it matter if the unit changes its owner?

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

It does, so we would need to account for that.

IMO it would be better to drop this commit. These traits are only used once (or never), so this is moving the overhead from death to creation, which is just as bad wrt trait lookups!

@@ -173,8 +199,7 @@ void ITick.Tick(Actor self)
{
if (hp > DisplayHP)
DisplayHP = hp;

if (DisplayHP > hp)
else if (DisplayHP > hp)

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

This can turn into just an else if you change the > above to a >=

@@ -95,11 +120,11 @@ public void Resurrect(Actor self, Actor repairer)
PreviousDamageState = DamageState.Dead,
};

foreach (var nd in self.TraitsImplementing<INotifyDamage>()
.Concat(self.Owner.PlayerActor.TraitsImplementing<INotifyDamage>()))
foreach (var nd in notifyDamage

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

We can avoid allocating an enumerator here by adding a second foreach loop for the player traits.

This comment has been minimized.

@reaperrr

reaperrr Sep 27, 2018

Contributor

Done. Assuming it would have the same effect there, I've done the same for the uncacheable AppliedDamage loops.

@@ -136,12 +161,12 @@ public void InflictDamage(Actor self, Actor attacker, Damage damage, bool ignore
PreviousDamageState = oldState,
};

foreach (var nd in self.TraitsImplementing<INotifyDamage>()
.Concat(self.Owner.PlayerActor.TraitsImplementing<INotifyDamage>()))
foreach (var nd in notifyDamage

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Likewise here

@@ -151,8 +176,8 @@ public void InflictDamage(Actor self, Actor attacker, Damage damage, bool ignore

if (hp == 0)
{
foreach (var nd in self.TraitsImplementing<INotifyKilled>()
.Concat(self.Owner.PlayerActor.TraitsImplementing<INotifyKilled>()))
foreach (var nd in notifyKilled

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Likewise here.

damageModifiersPlayer = newOwner.PlayerActor.TraitsImplementing<IDamageModifier>().ToArray();
notifyKilledPlayer = newOwner.PlayerActor.TraitsImplementing<INotifyKilled>().ToArray();
}

public void Resurrect(Actor self, Actor repairer)

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Note to self: this horrible method still exists N years later 😢

if (a.IsTraitDisabled || a.Info.Type == null || !Versus.ContainsKey(a.Info.Type))
continue;

armor.Append(Versus[a.Info.Type]);

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Because of the Enumerable.Empty above, this is actually handled by LINQ in a much less efficient way than we have above!

You make armor a list, but you still need to enumerate the collection twice (once to build the list, and then again inside ApplyPercentageModifiers), so better to drop this part of the commit.


void INotifyCreated.Created(Actor self)
{
defenderStats = self.Owner.PlayerActor.Trait<PlayerStatistics>();

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

It does, so we would need to account for that.

IMO it would be better to drop this commit. These traits are only used once (or never), so this is moving the overhead from death to creation, which is just as bad wrt trait lookups!

reaperrr added some commits Sep 17, 2018

Changed an 'if' to 'else' in Health.Tick
Since the two 'if's were mutually exclusive, the 2nd 'if' was turned into an 'else' to potentially skip that check if the 1st succeeds.
Migrate DamageState thresholds to integer
While avoiding divisions.

While there haven't been any desyncs to speak of recently (not in this part of the code, in any case), this still looks like an oversight from when we migrated away from using floats.
This also makes it easier to expose the thresholds to modders later.
Cache *Notify traits in Health where applicable
During heated battles, those TraitsImplementing look-ups in Health might cause bursty CPU load on warhead impacts. Caching the notify traits of the actor + owner can reduce the trait look-ups per impact by more than half.
Skip DamageWarhead armor lookups if no Versus defined
- directly return 100 when no Versus values are defined (meaning the warhead would have 100% efficiency vs. all armor types anyway)

@reaperrr reaperrr force-pushed the reaperrr:inflict-dmg-perf branch from d260b69 to bafbd0f Sep 27, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 27, 2018

Updated.

@pchote

pchote approved these changes Sep 29, 2018

@pchote pchote merged commit 83cd7cf into OpenRA:bleed Sep 29, 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:inflict-dmg-perf branch Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment