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

Eliminated LengthSquared > 0 for slight performance #13609

Closed
wants to merge 2 commits into from

Conversation

forcecore
Copy link
Contributor

It is common in current bleed that LengthSquared is compared with 0. In this case, it is faster to check if all elements are 0 than to perform sum of squared then compare with zero.

@pchote
Copy link
Member

pchote commented Jul 8, 2017

Do you have profiling numbers demonstrating improvement?

@forcecore
Copy link
Contributor Author

forcecore commented Jul 8, 2017

I know is extreme to have 50 rocket guys shooting all at the same time but:

https://www.youtube.com/watch?v=3LSQooExybM : Before - Max 5 ms per tick
https://www.youtube.com/watch?v=LFbwIdZNAiw : After - Doesn't reach 4 ms per tick

I think it makes difference.

@@ -68,7 +68,7 @@ public override Activity Tick(Actor self)
if (host.Type == TargetType.Invalid || health == null)
return NextActivity;

if (closeEnough.LengthSquared > 0 && !host.IsInRange(self.CenterPosition, closeEnough))
if (closeEnough.Length != 0 && !host.IsInRange(self.CenterPosition, closeEnough))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is LengthSquared replaced with Length here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Repair.cs, closeEnough is WDist class, who already has Length inside the class and computing LengthSquared is an extra work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. WDist is a struct actually (not that that matters here).
Maybe closeEnough != WDist.Zero should be used.

@@ -68,7 +68,7 @@ public override Activity Tick(Actor self)
if (host.Type == TargetType.Invalid || health == null)
return NextActivity;

if (closeEnough.LengthSquared > 0 && !host.IsInRange(self.CenterPosition, closeEnough))
if (closeEnough != WDist.Zero && !host.IsInRange(self.CenterPosition, closeEnough))
Copy link
Contributor

@reaperrr reaperrr Jul 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDist supports >, so you should revert the != to make sure there won't be any unwanted behavior.

Edit: Actually, looking at how the WDist operators work, I suspect just using .Length instead of LengthSquared will have the biggest benefit over bleed here.

@@ -573,7 +573,7 @@ int IncreaseAltitude(int predClfDist, int diffClfMslHgt, int relTarHorDist, int
{
// Aim for the target
var vDist = new WVec(-relTarHgt, -relTarHorDist, 0);
desiredVFacing = (sbyte)vDist.HorizontalLengthSquared != 0 ? vDist.Yaw.Facing : vFacing;
desiredVFacing = (sbyte)(vDist.HasNonZeroHorizontalLength ? vDist.Yaw.Facing : vFacing);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vDist.Z is guaranteed to be 0 here, so you can simply check for vDist != WVec.Zero instead.

@@ -666,7 +666,7 @@ int IncreaseAltitude(int predClfDist, int diffClfMslHgt, int relTarHorDist, int
{
// Aim for the target
var vDist = new WVec(-relTarHgt, -relTarHorDist, 0);
desiredVFacing = (sbyte)vDist.HorizontalLengthSquared != 0 ? vDist.Yaw.Facing : vFacing;
desiredVFacing = (sbyte)(vDist.HasNonZeroHorizontalLength ? vDist.Yaw.Facing : vFacing);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@@ -676,7 +676,7 @@ int IncreaseAltitude(int predClfDist, int diffClfMslHgt, int relTarHorDist, int
{
// Aim for the target
var vDist = new WVec(-relTarHgt, relTarHorDist, 0);
desiredVFacing = (sbyte)vDist.HorizontalLengthSquared != 0 ? vDist.Yaw.Facing : vFacing;
desiredVFacing = (sbyte)(vDist.HasNonZeroHorizontalLength ? vDist.Yaw.Facing : vFacing);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@@ -686,7 +686,7 @@ int IncreaseAltitude(int predClfDist, int diffClfMslHgt, int relTarHorDist, int
// Aim to attain cruise altitude as soon as possible while having the absolute value
// of vertical facing bound by the maximum vertical rate of turn
var vDist = new WVec(-diffClfMslHgt - info.CruiseAltitude.Length, -speed, 0);
desiredVFacing = (sbyte)vDist.HorizontalLengthSquared != 0 ? vDist.Yaw.Facing : vFacing;
desiredVFacing = (sbyte)(vDist.HasNonZeroHorizontalLength ? vDist.Yaw.Facing : vFacing);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@@ -702,7 +702,7 @@ int IncreaseAltitude(int predClfDist, int diffClfMslHgt, int relTarHorDist, int
// Aim to attain cruise altitude as soon as possible while having the absolute value
// of vertical facing bound by the maximum vertical rate of turn
var vDist = new WVec(-diffClfMslHgt - info.CruiseAltitude.Length, -speed, 0);
desiredVFacing = (sbyte)vDist.HorizontalLengthSquared != 0 ? vDist.Yaw.Facing : vFacing;
desiredVFacing = (sbyte)(vDist.HasNonZeroHorizontalLength ? vDist.Yaw.Facing : vFacing);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@reaperrr
Copy link
Contributor

reaperrr commented Jul 9, 2017

I've tested this myself, but couldn't reproduce those performance gains.

With 197 bazookas, all shooting at exactly the same time, both times on the same map, same spawnpoint, same starting units, I couldn't see a visible difference in Tick rate, both times it maxed out at 10.x ms/tick.

@RobotCaleb
Copy link
Contributor

This method of profiling is weird.

@forcecore
Copy link
Contributor Author

Closing, no useful gain with all the changes.

@forcecore forcecore closed this Jul 15, 2017
@forcecore forcecore deleted the LengthSqVs0 branch August 26, 2017 21:48
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

6 participants