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

Improve Health.InflictDamage performance #19173

Merged
merged 2 commits into from
Mar 21, 2021

Conversation

reaperrr
Copy link
Contributor

@reaperrr reaperrr commented Feb 20, 2021

While this is not responsible for the occasionally huge cost of InflictDamage (that's usually caused by INotify* calls to ScriptTriggers), profiling has shown that filtering out 100 damage modifiers (no damage change) early is cheaper than applying those anyway.
Additionally, profiling also showed foreach loops to be cheaper than LINQ here as well.

On tran.husk2 on the RA shellmap, which loses health every 8 ticks, the cost for applying damage modifiers went down from ~180 to just ~9 stopwatch ticks (and from several thousand to just 50 on world tick 1). Other reproducable cases I compared showed improvements as well.

Also moved up the Undamaged check in DamageState since it should be cheaper than those Critical/Heavy/Medium checks, so in my opinion it makes sense to perform that first. More of an extra, though.

@pchote
Copy link
Member

pchote commented Feb 20, 2021

ApplyPercentageModifiers has a comment saying

// See the comments of PR#6079 for a faster algorithm if this becomes a performance bottleneck

which I assume is referring to #6079 (comment). Could you please try profiling this to see how it compares?

@teinarss
Copy link
Contributor

teinarss commented Feb 21, 2021

Would be interesting to test if going span and remove the heap allocation would gain us even more perf.

Something like this.

				Span<int> modifiers = stackalloc int[damageModifiers.Length + damageModifiersPlayer.Length];

				for (var i = 0; i < damageModifiers.Length; i++)
				{
					modifiers[i] = damageModifiers[i].GetDamageModifier(attacker, damage);
				}

				for (var i = damageModifiers.Length; i < damageModifiersPlayer.Length + damageModifiers.Length; i++)
				{
					modifiers[i] = damageModifiersPlayer[i - damageModifiers.Length].GetDamageModifier(attacker, damage);
				}

				damage = new Damage(Util.ApplyPercentageModifiers(damage.Value, modifiers), damage.DamageTypes);

And update

public static int ApplyPercentageModifiers(int damageValue, Span<int> percentages)

@pchote
Copy link
Member

pchote commented Feb 21, 2021

I realize we're now in bike-shedding territory, but it seems to me that the best option would be to avoid allocations and multiple enumerations completely:

if (!ignoreModifiers && damage.Value > 0)
{
	// PERF: Util.ApplyPercentageModifiers has been manually inlined to
	// avoid unnecessary loop enumerations and allocations
	var appliedDamage = (decimal)damage.Value;
	foreach (var dm in damageModifiers)
	{
		var modifier = dm.GetDamageModifier(attacker, damage);
		if (modifier != 100)
			appliedDamage *= modifier / 100m;
	}

	foreach (var dm in damageModifiersPlayer)
	{
		var modifier = dm.GetDamageModifier(attacker, damage);
		if (modifier != 100)
			appliedDamage *= modifier / 100m;
	}

	damage = new Damage((int)appliedDamage, damage.DamageTypes);
}

Profiling has shown that filtering them out early is cheaper
than applying those percentage modifiers anyway.

Additionally, profiling shows foreach loops to be cheaper
than LINQ here as well.
A mere int comparison is obviously cheaper than
a comparison between two multiplications,
so pulling this above the checks of other damage states
allows us to bail early for undamaged actors.
@reaperrr
Copy link
Contributor Author

Updated as suggested above. It is a tad faster than what I was doing, though in terms of absolute gains we're already down to low µs.

@teinarss teinarss merged commit e2a6b55 into OpenRA:bleed Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants