Search Unity

  1. Megacity Metro Demo now available. Download now.
    Dismiss Notice
  2. Unity support for visionOS is now available. Learn more in our blog post.
    Dismiss Notice

ForEach creates garbage, doesn't work with ComponentGroup, interrupts method execution

Discussion in 'Entity Component System' started by e199, Mar 18, 2019.

  1. e199

    e199

    Joined:
    Mar 24, 2015
    Posts:
    101
    I tick my systems manually.
    Here is the code I have:
    Code (CSharp):
    1.  
    2.         protected override void OnCreateManager()
    3.         {
    4.             _group = Entities.WithAll<Id, NewEntity>().WithNone<IdMapped>().ToComponentGroup();
    5.         }
    6.  
    7.         protected override void OnUpdate()
    8.         {
    9.             Entities.With(_group).ForEach((Entity e, ref Id id) =>
    10.             {
    11.                 Debug.Log("Wooooo");
    12.             });
    13.             Debug.Log("WO");
    14.             TrackedUpdate();
    15.         }
    16.  
    Neither of those Debugs are triggered.
    BUT, when I use this:

    Code (CSharp):
    1.  
    2.         protected override void OnUpdate()
    3.         {
    4.            Entities.WithAll<Id, NewEntity>().WithNone<IdMapped>().ForEach((Entity e, ref Id id) =>
    5.             {
    6.                 Debug.Log("Wooooo");
    7.             });
    8.             Debug.Log("WO");
    9.             TrackedUpdate();
    10.         }
    11.  
    Debugs are triggered as they should.
    Why?

    Old ForEach is broken too.
    How to use ForEach without creating garbage?

    2019.1.0b6, Entities preview 29
     
    phobos2077 likes this.
  2. MostHated

    MostHated

    Joined:
    Nov 29, 2015
    Posts:
    1,235
    Are you sure it isn't the Debug.Log that is producing the garbage?
     
    Antypodish and dzamani like this.
  3. e199

    e199

    Joined:
    Mar 24, 2015
    Posts:
    101
    LoL

    Debug.logs were added after I changed api to use explicit groups and saw that systems were not actually executing
     
    phobos2077 likes this.
  4. yossi_horowitz_artie

    yossi_horowitz_artie

    Joined:
    Jan 30, 2019
    Posts:
    87
    To avoid generating garbage, try creating a delegate member variable in your system class and initializing it in OnCreateManager() -- then pass that into ForEach instead of passing in a lambda.

    I've seen it indicated elsewhere on these forums that eventually the compiler will be modified to prevent garbage allocation in this specific case.
     
  5. scobi

    scobi

    Unity Technologies

    Joined:
    May 14, 2014
    Posts:
    32
    Several things:

    * I don't know why `With(group)` doesn't match - or possibly it is throwing which skips both logs, or the update is not called in the first place. There are tests around scenarios with and without groups passed in, not sure what's going on here. I could use a repro project via "Help | Report a Bug...". Let me know the ID and I'll treat that as my pri 1 ok?
    * The `ComponentGroup` is constructed within the `ForEach` as the combination of the `With*()` queries and the delegate param types. So if you have `WithAll<Id>.ForEach(ref Id...)`, it is redundant and possibly will give unexpected results in some cases.
    * Yes, this ^ should be doc'd and validated with an exception. It's on my list to do next - I have a series of PR's planned for improving ForEach. :D
    * It should only generate garbage on the first frame. Subsequent frames should not alloc, unless it's a closure because you're capturing 'this' or locals or whatever.
    * Yes, this ^ should be warned about when detected. It's on my list to do third. Also needs tests to validate memory behavior matches first-frame-only expectations (which is my current task).
    * It's also true that we will be eliminating the lambda alloc at some point but I don't think it will be soon. Also not sure whether it will come through Burst or the planned buildtime codegen.
     
    Last edited: Mar 20, 2019
  6. e199

    e199

    Joined:
    Mar 24, 2015
    Posts:
    101
    @scobi thanks for clarification about proper use of ForEach
    Sadly, I was not able to reproduce it in 2019.1.8

    Can you propose a way to add/remove components without closures?
    PostUpdateCommands - makes a closure on `this`, EntityManager would do the same, but it can't be used while doing ForEach

    That system from screenshot makes 144B alloc for 2 closures, in the profiler you can see 112B alloc for one closure (PostUpdateCommands)
     
    Last edited: Mar 20, 2019
    phobos2077 likes this.
  7. runner78

    runner78

    Joined:
    Mar 14, 2015
    Posts:
    789
    Closures could maybe replace with local functions, so far i now, local function can use local variables but they are alloc free.
     
    Last edited: Mar 20, 2019
    e199 likes this.
  8. Orimay

    Orimay

    Joined:
    Nov 16, 2012
    Posts:
    304
    It seems like local functions can do the trick
     
    e199 likes this.
  9. Orimay

    Orimay

    Joined:
    Nov 16, 2012
    Posts:
    304
    Nope, seems it will be wrapped in a delegate to get passed to ForEach anyway :(
     
    phobos2077 likes this.
  10. scobi

    scobi

    Unity Technologies

    Joined:
    May 14, 2014
    Posts:
    32
    Currently ForEach is painful to use outside of pure ECS data transformation scenarios. My plan is to support delegates that can receive 'this' (which I can just send through) or receive an arbitrary userdata T, which can be routed through the delegate and provided via a WithData() type operator. This creates/worsens other problems, though, so I need to solve those at the same time. Will probably be more than a few weeks to get it all sorted.

    In the meantime, best you can do is a combination of several suggestions above:

    * Declare a delegate matching your lambda signature and add an instance of it as a class field. It must be a delegate, not an Action<>, because the parameterized type system does not support `ref`. (This is why we have to receive delegates and codegen all the variations.)
    * Move your lambda out to a local function right above your ForEach. But do not access any data in the outer function or you'll get a closure. You can only access `this`.
    * Call `Entities.ForEach(m_Delegate = m_Delegate ?? MyLocalFunc)`;

    You can use sharplab.io to rapidly run through scenarios and confirm the code is doing what you want. If you see a `newobj` that is not gated by a branch test (usually from a static field cache the c# compiler generates) then you know you'll be generating garbage every OnUpdate.

    Anyways...wow, holy boilerplate code, Batman. It sure loses a lot of the benefits of nice fluent and lambda syntax, and is trivially easy to screw up and generate garbage from. I don't consider the API done until I've solved that.
     
  11. Orimay

    Orimay

    Joined:
    Nov 16, 2012
    Posts:
    304
    It doesn't have to be a local function, though, does it?
     
  12. scobi

    scobi

    Unity Technologies

    Joined:
    May 14, 2014
    Posts:
    32
    Sure, it can also be a static or instance method. It just can't be a lambda. I personally would prefer the local function because then I could keep the code right next to its use in the ForEach, but it doesn't matter to the runtime.
     
    phobos2077 and Orimay like this.
  13. e199

    e199

    Joined:
    Mar 24, 2015
    Posts:
    101
    Thanks for detailed responses! Great to know there are improvements on the road.
     
  14. runner78

    runner78

    Joined:
    Mar 14, 2015
    Posts:
    789
    Its only work with the original delagates (EntityQueryBuilder.F_DD as example) with an custom delagate (1:1 copy fromEntityQueryBuilder.F_DD) i get a not matching error.
     
  15. GilCat

    GilCat

    Joined:
    Sep 21, 2013
    Posts:
    676
    This is something that will come very handy :).
    Right now my only source of garbage is when i need to use PostUpdateCommands in ForEach. What would be the appropriate way of addressing this right now?

    Update: I am creating a static EntityCommandBuffer right now so the lambda doesn't allocate anything else.
     
    Last edited: May 4, 2019
  16. JooleanLogic

    JooleanLogic

    Joined:
    Mar 1, 2018
    Posts:
    447
    Is this the idea being proposed here? I have a fair few of these closure situations and this appears to work.
    Code (CSharp):
    1. public class MyComponentSystem : ComponentSystem
    2. {
    3.     Unity.Entities.EntityQueryBuilder.F_D<MyComponent> _delegate;
    4.     int _groupValue;
    5.  
    6.     void OnCreate() {
    7.         _delegate = forEachFunc;
    8.     }
    9.  
    10.     void forEachFunc(ref MyComponent comp)
    11.     {
    12.         comp.value = this._groupValue;
    13.     }
    14.  
    15.     void OnUpdate() {
    16.         _groupValue++;
    17.      
    18.         Entities.ForEach(_delegate);
    19.     }
    20. }
     
    phobos2077 and Orimay like this.
  17. scobi

    scobi

    Unity Technologies

    Joined:
    May 14, 2014
    Posts:
    32
    Yes that will work. Though it can be a bit more compact if you remove OnCreate() and use `Entities.ForEach(_delegate = _delegate ?? forEachFunc);`

    The options I can think of are:

    * Use statics, as @GilCat proposes
    * Use the "cached delegate" trick that @jooleanlogic shows
    * Accept tiny amount of garbage per frame/system and mitigate with the incremental GC
    * Switch to IJobForEach if it's crucial

    This level of the API is aimed at "get things working easily and now" and so we've pushed it out with some flaws that we know about and plan to address. If you really are strict about your performance/mem requirements, you may not have a choice other than putting plain ForEach aside until we Make It Better.

    Note also that ForEach is currently main-thread-only and non-burstable, which means perf is severely limited right out of the gate with this API. A lot of work stands between us and getting it to IJobForEach-level perf. (It's on the map, but underneath some dense fog of war..)
     
    phobos2077 and SergeyRomanko like this.
  18. GilCat

    GilCat

    Joined:
    Sep 21, 2013
    Posts:
    676
    I would keep away from ComponentSystems and ForEach if it was possible but unfortunately I have to deal with a few MonoBehaviours based components (UI Text mainly) and that is the bottleneck in my whole development using DOTS.
    I wish there was another way to address this but I couldn't find any reliable way.
    Leaving that side and going back to ForEach i went into EntityQueryBuilder source and forked my alternate version that follows @scobi intents and added a .WithContext() that receives data that will then be passed into a .ForeachWithContext() relieving me from any garbage generation. I know this generates too many combinations of delegates so I go and add them as I need.
    I've also added some other extensions limit traversing entities as much as possible.

    I think that will always be the case when accessing Monobehaviours? Right?
    I would love to see ways of natively accessing unity legacy components properties just like we do with Transform where we have TransformAccess.
    Is there anyway of access them through unity engine dll (eg. accessing RecTransform Rect or any other main thread restricted property)? I haven't seen nothing about it anywhere.

    Thanks for the awesome job you are all doing.
     
  19. scobi

    scobi

    Unity Technologies

    Joined:
    May 14, 2014
    Posts:
    32
    > forked my alternate version

    That both warms my heart and makes me a little sad. I'm happy because -- yes!! -- you can do what you need because we don't have the code all locked away in nativeland or precompiled. Yay packages. But I'm a little sad because now you have to migrate this work forward as we release new stuff. But that's gamedev..ain't got time to bleed.

    Regarding MB's, yeah.. Any optimizations we add to ForEach will need to be automatically reduced to what the more restricted UnityEngine objects can support. But the world of GameObjects already has performance caps on it anyway, which is why DOTS got started. Main-thread-or-not of MB data is just one of many architectural limitations that DOTS eliminates by design and by default.
     
    Orimay, GilCat and optimise like this.
  20. Orimay

    Orimay

    Joined:
    Nov 16, 2012
    Posts:
    304
    I use the same approach. Just came here to post about it :D
     
  21. Justin_Larrabee

    Justin_Larrabee

    Joined:
    Apr 24, 2018
    Posts:
    106
    This would be so wonderful. I've jobified everything I can but it's going to be a long time before we can discard a hybrid architecture. Having useful, read-only access to the physics engines within a job would be *wonderful*. The existing ray/sphere/etc command stuff is a start but not enough. Having the ability to do it within the job itself and not have to be pre-scheduled would be ideal.
     
    GilCat likes this.
  22. eizenhorn

    eizenhorn

    Joined:
    Oct 17, 2016
    Posts:
    2,683
    For that we have DOTS Physics preview
     
  23. YurySedyakin

    YurySedyakin

    Joined:
    Jul 25, 2018
    Posts:
    62
    Looks like there is more allocations in foreach besides the ones of the delegate. Here, I've got something allocating inside ForEach implementation. (Unity 2019.1.4f1)
     

    Attached Files:

  24. GilCat

    GilCat

    Joined:
    Sep 21, 2013
    Posts:
    676
    Is that generating garbage every frame? I believe the first time the query builder is evaluated it generates garbage but only on that moment.
     
  25. YurySedyakin

    YurySedyakin

    Joined:
    Jul 25, 2018
    Posts:
    62
    It is every frame, at least in editor. With leaks detection turned off.
    Also: I see a call to ReferenceEquals(m_Query, other.m_Query); inside EntityQueryBuilder.ShallowEquals. Looks like those 40 bytes of allocation stem from boxing. Is it really necessary every frame?
     
    Last edited: Jun 3, 2019
    scobi likes this.
  26. JooleanLogic

    JooleanLogic

    Joined:
    Mar 1, 2018
    Posts:
    447
    EntityQueryCache.ValidateMatchesCache(..) is inside ENABLE_UNITY_COLLECTIONS_CHECKS directive.
    If you're checking for garbage, do it in a release build.
     
    scobi likes this.
  27. scobi

    scobi

    Unity Technologies

    Joined:
    May 14, 2014
    Posts:
    32
    @YurySedyakin even though @jooleanlogic is right about checking in release builds, you did find a fairly dumb oversight on our part (it's enum-to-enum Equals, which boxes), and the extra gc is a regression from a recent change. I'll fix and add some tests to prevent future regressions.
     
    phobos2077, JesOb and GilCat like this.
  28. Opeth001

    Opeth001

    Joined:
    Jan 28, 2017
    Posts:
    1,112
    hi Everyone!
    how do you create EntityQueryBuilder containing ISystemStateComponentData ?
     
  29. elcionap

    elcionap

    Joined:
    Jan 11, 2016
    Posts:
    138
    The same way you do with IComponentData.

    Entities.WithAll<MySystemStateComponentData>().ForEach(...);
    Entities.ForEach((ref MySystemStateComponentData foo) => { ... });

    Any ISystemState* inherits their counterparts. They only behave different internally. So you should deal with them the same way you deal with IComponentData/IBufferElement/ISharedComponentData

    []'s
     
  30. Opeth001

    Opeth001

    Joined:
    Jan 28, 2017
    Posts:
    1,112
    hi elcionap,
    im not looking to create a simple Entities.ForEach loop, i already did it and it's creating a lot a GC ( this Thread is about this specific problem ) the way to solve it, is using local functions instead of the delegates.

    Code (CSharp):
    1.  
    2. EntityQueryBuilder.F_EDDD<myComponentData ,Rotation,Translation>  LocalFunctionDelegate;
    3.  
    4.  
    5.  protected override void OnCreate()
    6.         {
    7.             LocalFunctionDelegate= LocalFunction;
    8.         }
    9.  
    10.   private void LocalFunction(Entity entityProjectile, ref myComponentData data1, ref Rotation rotation, ref Translation translation)
    11.         {
    12.          // code
    13.         }
    14.  
    15. then:
    16. Entities.
    17.              WithAllReadOnly<myComponentData , Rotation, Translation>().
    18.              ForEach(LocalFunctionDelegate);
    19.  

    this way im not getting GC problems anymore, but some other systems are using SystemState and EntityQueryBuilder do not contain overloads for them.
     
  31. elcionap

    elcionap

    Joined:
    Jan 11, 2016
    Posts:
    138
    It doesn't matter if your are using ForEach with an inline function, a Delegate reference, chunk iteration, IJobForEach etc, IComponentData and ISystemStateComponentData are expressed the same way.

    You don't need a different overload. As I said ISystemStateComponentData inherits IComponentData. It's the same overload for both. Any ISystemStateComponentData is an IComponentData too. It's the same with ISystemStateBufferElementData/IBufferElementData and ISystemStateSharedComponentData/ISharedComponentData.

    The example bellow will work the same way for Foo being ISystemStateComponentData or IComponentData. They will only differ internally when an entity is being destroyed.
    Code (CSharp):
    1. using Unity.Entities;
    2.  
    3. struct Foo : ISystemStateComponentData {
    4.     public int Value;
    5. }
    6.  
    7. class FooSystem : ComponentSystem {
    8.     EntityQueryBuilder.F_D<Foo> m_OnUpdateFunc;
    9.  
    10.     protected override void OnCreate() {
    11.         base.OnCreate();
    12.  
    13.         m_OnUpdateFunc = OnUpdate;
    14.     }
    15.  
    16.     void OnUpdate(ref Foo foo) {
    17.         //
    18.     }
    19.  
    20.     protected override void OnUpdate() {
    21.         Entities.ForEach(m_OnUpdateFunc);
    22.     }
    23. }
    24.  
    []'s
     
    Opeth001 likes this.
  32. Opeth001

    Opeth001

    Joined:
    Jan 28, 2017
    Posts:
    1,112
    awesome ^_^
    i tried it before and i got an error i certainly wrongly wrote something :D

    Thank you !!!
     
  33. elcionap

    elcionap

    Joined:
    Jan 11, 2016
    Posts:
    138
    Did work it?

    []'s
     
    Opeth001 likes this.
  34. Opeth001

    Opeth001

    Joined:
    Jan 28, 2017
    Posts:
    1,112
    Yes !!! ^_^
    my System managing Particale Systems was creating 0.7ko GC cause it's using 5 foreach loops, now it's creating 0 GC :p
     
  35. optimise

    optimise

    Joined:
    Jan 22, 2014
    Posts:
    2,114
    Hi @scobi. Looks like ForEach API garbage generation bug will be fixed in next new entities package. Will it fixed the performance issue in mobile device (Android, IOS etc) too? If not, what's the ETA?
     
  36. scobi

    scobi

    Unity Technologies

    Joined:
    May 14, 2014
    Posts:
    32
    I don't know what performance issue on mobile you're referring to.
     
  37. optimise

    optimise

    Joined:
    Jan 22, 2014
    Posts:
    2,114
  38. scobi

    scobi

    Unity Technologies

    Joined:
    May 14, 2014
    Posts:
    32
    Overhead of systems is a totally separate thing from this. A lot of work has been done there - maybe @LunaMeierUnity can pop in and fill us in on progress there.
     
    optimise likes this.