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

Activity, fixes. #18792

Merged
merged 1 commit into from Dec 24, 2020
Merged

Activity, fixes. #18792

merged 1 commit into from Dec 24, 2020

Conversation

anvilvapre
Copy link
Contributor

Activity, fixes.

Do not call SkipDoneActivities method recursively via the
NextActivity property. Rather use the nextActivity member.
Avoiding additional functional calls and a recursively
growing stack.

Do not call ChildActivity and NextActivity properties
twice in a row. Once to test for null and after to access
it's value. It will cause the complete list of activities
to be traversed twice looking for non done activities.

Replace Queue method with a version that does not the
NextActivity property causing an extra call to
SkipDoneActivities. Avoid calling Queue recursively.

Similar replace QueueChild with a version that does
not call additional methods.

Note that ActivitiesImplementing returns only non
done activities. The method name does not suggest this.

Please consider making NextActivity a method to cleary indicate it
involves the logic of skipping Done activities. To let
the called know it is 'expensive'.

Please consider renaming the protected property ChildActivity to
FirstChildActivityNotDone to avoid it being used as being
used as childActivity.

Please consider maintaining a pointer to the first
non done activity. This avoids the need the each time find it.

Do not call SkipDoneActivities method recursively via the
NextActivity property. Rather use the nextActivity member.
Avoiding additional function calls and a recursively
growing stack.

Do not call ChildActivity and NextActivity properties
twice in a row. Once to test for null and after to access
it's value. It will cause the complete list of activities
to be traversed twice looking for non done activities.

Replace Queue method with a version that does not the
NextActivity property causing an extra call to
SkipDoneActivities. Avoid calling Queue recursively.

Similar replace QueueChild with a version that does
not call additional methods.

Note that ActivitiesImplementing returns only non
done activities. The method name does not suggest this.

Please consider making NextActivity a method to cleary indicate it
involves the logic of skipping Done activities. To let
the called know it is 'expensive'.

Please consider renaming the protected property ChildActivity to
FirstChildActivityNotDone to avoid it being used as childActivity.

Please consider maintaining a pointer to the first
non done activity. This avoids the need the each time find it.
@dnqbob
Copy link
Contributor

dnqbob commented Oct 31, 2020

After last engine upgrade the SP seems not good on performance. Will this PR be the cure?

@anvilvapre
Copy link
Contributor Author

See also #18797

Comment on lines -123 to +124
if (ChildActivity != null && ChildActivity.State == ActivityState.Queued)
var ca = ChildActivity;
if (ca != null && ca.State == ActivityState.Queued)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure that this is note done in the optimization? Ie have you checked the IL?

Copy link
Member

Choose a reason for hiding this comment

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

I'm far from an expert at understanding IL, but this does appear to be evaluating ChildActivity twice on bleed:

// if ChildActivity != null
	IL_00c2:  ldarg.0 
	IL_00c3:  call instance class OpenRA.Activities.Activity class OpenRA.Activities.Activity::get_ChildActivity()
	IL_00c8:  brfalse.s IL_0101

// if ChildActivity.State == ActivityState.Queued
	IL_00ca:  ldarg.0 
	IL_00cb:  call instance class OpenRA.Activities.Activity class OpenRA.Activities.Activity::get_ChildActivity()
	IL_00d0:  callvirt instance valuetype OpenRA.Activities.ActivityState class OpenRA.Activities.Activity::get_State()
	IL_00d5:  brtrue.s IL_0101

// if ChildHasPriority
	IL_00d7:  ldarg.0 
	IL_00d8:  call instance bool class OpenRA.Activities.Activity::get_ChildHasPriority()
	IL_00dd:  brfalse.s IL_00f9

@anvilvapre
Copy link
Contributor Author

I have not checked. But to me it seems impossible that IL optimization can optimize out SkipDoneActivities.
The difference between ChildActivity and childActivity. The last probably could have been optimized out.
(Unless properties work different from calling functions.)

@pchote pchote merged commit 78253ce into OpenRA:bleed Dec 24, 2020
@dnqbob
Copy link
Contributor

dnqbob commented Dec 25, 2020

I am going to have some tests on how much this branch can optimize to OpenRA later.

@dnqbob
Copy link
Contributor

dnqbob commented Dec 25, 2020

Feels better, at least 3 FPS improve, and

20 ms [10048] Activity: Move
20 ms [10048] Activity: AttackMoveActivity

12 ms [14665] Activity: Move
12 ms [14665] Activity: MoveWithinRange
12 ms [14665] Activity: Attack

which may still be some spike lag, but better than before.

shellmap of SP has noticeable performance improvement.

@Shkarlatov Shkarlatov mentioned this pull request Jul 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

5 participants