WorldRenderer, replace ActorsWithTraits with ApplyToActorsWithTrait. #19557

anvilvapre · 2021-07-23T20:25:46Z

At least 2 to 4.2 times less ticks are used - by avoiding the allocation of Trait Actor pairs that use generics.

I.e. 2442 ticks before, 571 ticks after.

reaperrr

Didn't notice any issues

abcdefg30 · 2021-07-24T13:16:33Z

OpenRA.Game/Graphics/WorldRenderer.cs

+			World.ApplyToActorsWithTrait<IRenderShroud>((Actor actor, IRenderShroud trait) =>
+				trait.RenderShroud(this));


Suggested change

World.ApplyToActorsWithTrait<IRenderShroud>((Actor actor, IRenderShroud trait) =>

trait.RenderShroud(this));

World.ApplyToActorsWithTrait<IRenderShroud>((Actor actor, IRenderShroud trait) => trait.RenderShroud(this));

abcdefg30 · 2021-07-24T13:41:15Z

Did you measure the impact of #18798 (comment) (Action delegate allocations)?

anvilvapre · 2021-07-24T14:18:57Z

Did you measure the impact of #18798 (comment) (Action delegate allocations)?

Not in exact numbers. However there is an significant overhead. ActorsHavingTrait - that returns an enumerator not allocating a pair - is faster than calling a similar Apply function using an action. So enumerations have the preference.

pchote · 2021-07-24T22:48:14Z

@teinarss would you be able to do some of your benchmarking tests on this?

teinarss · 2021-07-24T23:29:12Z

Best way is to pull the specific code out and do a benchmark with https://benchmarkdotnet.org/index.html

…

On Sun, 25 Jul 2021, 00:48 Paul Chote, ***@***.***> wrote: @teinarss <https://github.com/teinarss> would you be able to do some of your benchmarking tests on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19557 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AESI7GENXQT7ISWWEY7X63DTZM7LTANCNFSM5A4UREBQ> .

anvilvapre · 2021-07-25T13:19:19Z

See also #18357 (comment)

pchote · 2021-07-27T20:58:55Z

I let the RA shellmap run for 5000 ticks for 3 runs, alternating pr/before/pr/before/pr/before and looked at the render_prepare (which is what calls Generate*Renderables) and also tick_time (on the assumption that any extra GC stalls from the Action allocations are most likely to be visible here).

The before/after results overlap within the scatter of the 3 runs, so there doesn't seem to be any significant impact either way and we can instead consider this on code style/consistency grounds.

pchote · 2021-07-27T21:04:46Z

OpenRA.Game/Graphics/WorldRenderer.cs

@@ -153,14 +153,14 @@ void GenerateRenderables()
 		// PERF: Avoid LINQ.
 		void GenerateOverlayRenderables()
 		{
-			foreach (var a in World.ActorsWithTrait<IRenderAboveShroud>())
+			World.ApplyToActorsWithTrait<IRenderAboveShroud>((Actor actor, IRenderAboveShroud trait) =>


And likewise here.

pchote · 2021-07-27T21:05:03Z

OpenRA.Game/Graphics/WorldRenderer.cs

@@ -195,14 +195,14 @@ void GenerateOverlayRenderables()
 		// PERF: Avoid LINQ.
 		void GenerateAnnotationRenderables()
 		{
-			foreach (var a in World.ActorsWithTrait<IRenderAnnotations>())
+			World.ApplyToActorsWithTrait<IRenderAnnotations>((Actor actor, IRenderAnnotations trait) =>


Its not necessary to specify all three types here. This could be either

World.ApplyToActorsWithTrait<IRenderAnnotations>((actor, trait) =>

or

World.ApplyToActorsWithTrait>((Actor actor, IRenderAnnotations trait) =>

IMO the first is easier to read.

pchote · 2021-07-27T21:05:45Z

OpenRA.Game/Graphics/WorldRenderer.cs

-			foreach (var a in World.ActorsWithTrait<IRenderAboveWorld>())
-				if (a.Actor.IsInWorld && !a.Actor.Disposed)
-					a.Trait.RenderAboveWorld(a.Actor, this);
+			World.ApplyToActorsWithTrait<IRenderAboveWorld>((Actor actor, IRenderAboveWorld trait) =>


And here, and below.

penev92 · 2021-08-10T12:15:48Z

Adding a Fixup requested label because of the above three comments.

anvilvapre · 2021-08-10T16:29:17Z

I let the RA shellmap run for 5000 ticks for 3 runs, alternating pr/before/pr/before/pr/before and looked at the render_prepare (which is what calls Generate*Renderables) and also tick_time (on the assumption that any extra GC stalls from the Action allocations are most likely to be visible here).

The before/after results overlap within the scatter of the 3 runs, so there doesn't seem to be any significant impact either way and we can instead consider this on code style/consistency grounds.

It may also be that on ``dotnetthis makes less of a difference then onmono`.

…hTrait. OpenRA#18798.

abcdefg30 · 2022-03-13T09:45:51Z

Changelog

reaperrr previously approved these changes Jul 24, 2021

View reviewed changes

reaperrr added the PR: Needs +2 label Jul 24, 2021

abcdefg30 reviewed Jul 24, 2021

View reviewed changes

anvilvapre dismissed reaperrr’s stale review via 40c9ba5 July 24, 2021 18:20

anvilvapre force-pushed the 20210723_actors_with_trait branch from e499b4f to 40c9ba5 Compare July 24, 2021 18:20

pchote reviewed Jul 27, 2021

View reviewed changes

penev92 added the PR: Fixup requested label Aug 10, 2021

anvilvapre force-pushed the 20210723_actors_with_trait branch from 40c9ba5 to ff0b756 Compare August 10, 2021 16:28

WorldRenderer, replace foreach ActorsWithTraits with ApplyToActorsWit…

3bc3555

…hTrait. OpenRA#18798.

anvilvapre force-pushed the 20210723_actors_with_trait branch from ff0b756 to 3bc3555 Compare August 19, 2021 19:52

RoosterDragon approved these changes Mar 12, 2022

View reviewed changes

abcdefg30 removed the PR: Fixup requested label Mar 13, 2022

abcdefg30 approved these changes Mar 13, 2022

View reviewed changes

abcdefg30 merged commit 83357af into OpenRA:bleed Mar 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WorldRenderer, replace ActorsWithTraits with ApplyToActorsWithTrait. #19557

WorldRenderer, replace ActorsWithTraits with ApplyToActorsWithTrait. #19557

anvilvapre commented Jul 23, 2021

reaperrr left a comment

abcdefg30 Jul 24, 2021

abcdefg30 commented Jul 24, 2021

anvilvapre commented Jul 24, 2021

pchote commented Jul 24, 2021

teinarss commented Jul 24, 2021 via email

anvilvapre commented Jul 25, 2021

pchote commented Jul 27, 2021

pchote Jul 27, 2021 •

edited

pchote Jul 27, 2021

pchote Jul 27, 2021 •

edited

penev92 commented Aug 10, 2021

anvilvapre commented Aug 10, 2021

abcdefg30 commented Mar 13, 2022

		World.ApplyToActorsWithTrait<IRenderShroud>((Actor actor, IRenderShroud trait) =>
		trait.RenderShroud(this));

WorldRenderer, replace ActorsWithTraits with ApplyToActorsWithTrait. #19557

WorldRenderer, replace ActorsWithTraits with ApplyToActorsWithTrait. #19557

Conversation

anvilvapre commented Jul 23, 2021

reaperrr left a comment

Choose a reason for hiding this comment

abcdefg30 Jul 24, 2021

Choose a reason for hiding this comment

abcdefg30 commented Jul 24, 2021

anvilvapre commented Jul 24, 2021

pchote commented Jul 24, 2021

teinarss commented Jul 24, 2021 via email

anvilvapre commented Jul 25, 2021

pchote commented Jul 27, 2021

pchote Jul 27, 2021 • edited

Choose a reason for hiding this comment

pchote Jul 27, 2021

Choose a reason for hiding this comment

pchote Jul 27, 2021 • edited

Choose a reason for hiding this comment

penev92 commented Aug 10, 2021

anvilvapre commented Aug 10, 2021

abcdefg30 commented Mar 13, 2022

pchote Jul 27, 2021 •

edited

pchote Jul 27, 2021 •

edited