1

I've got a method that I'm calling ... well, very often. And now the GC is becoming a problem, because apparently this method is creating garbage, but I can't seem to figure out why.

        Item item;

        foreach (Point3D p in Point3D.AllDirections)
        {
            item = grid[gridPosition + p];

            if (item != null && item.State != State.Initialized)

            else
                return;
        }

        OtherMethod(this);

This method is inside the Item class and figures out the state of neighbouring items. The gridPosition variable is obviously the position of this item inside the grid and the Point3D is a struct that just contains 3 ints, one for each axis. It also contains a few predefined static arrays such as AllDirections which itself only contains Point3Ds.

Now, the state of items can change at any time and once all neighbours have a specific state (or rather aren't in the Initialized state) I do some other stuff (OtherMethod), which also changes the state of the item so this method is no longer called. As such the produced garbage is only a problem when neighbours don't change their state (for example when there is no user input). I guess you could think of this as a substitution for an event driven system.

I admit that I have a rather limited understanding of memory management and I tend to only really understand things by doing them, so reading up on it didn't really help me much with this problem. However, after reading about it I got the impression that every value type, that is defined within a method and doesn't leave it, will be collected upon leaving the method. So I have a little trouble figureing out what's actually left after the return.

Any ideas on what's actually generating the garbage here ? Of course the best case would be to produce no garbage at all, if at all possible.

(Just in case this is relevant, I'm working in Unity and the method in question is the Monobehaviour Update)

Edit for all the comments: Holy cow you guys are quick. So... @ Jeppe Stig Nielsen: There is nothing between if and else, there was once but I didn't need that anymore. It's just a quick way to leave if no neighbour was found. Also, Indexer ! Completely forgot about that, here it is:

public Item this[Point3D gridPosition]
{
    get
    {
        Item item;
        items.TryGetValue(gridPosition, out item);
        return item;
    }
}

Could the TryGetValue be the reason ?

@ Luaan: I already tried a normal for loop, didn't change anything. That looked something like this:

        for (int i = 0; i < 6; i++)
        {
            item = grid[gridPosition + Point3D.AllDirections[i]];
        }

If it was the enumerator, that should've fixed it, no ?

@ Rufus: Yes that's a better way to write the if, will fix that. The whole thing is a work in progress right now, so that's just a remnant of past experimentation. There is currently is nothing else in the if statement.

@ Tom Jonckheere: Well there isn't really much else. It's just Unitys Update() followed by two if statements for the State. And that's really just because I currently only work with two states but have already setup more, so someday there will probably be a switch instead of ifs.

@ Adam Houldsworth: Well yes, the problem is certainly that I call it so often. The odd thing is, the amount of garbage produced varies wildly. As far as I can tell it's in the range of 28 bytes to 1.8KB. As for the whole red herring thing, I don't think that's the case. The profiler points to the Update method, which only contains two ifs, one which is the code from above, the other one isn't being used when no state changes occur. And when testing, there are no state changes, except for intial setup.

Also, sorry for the wrong tag, first time I've visited this site. Looks like I'll have some reading to do :)

Sentrigal
  • 61
  • 1
  • 4
  • What is between `if` and `else`? – Jeppe Stig Nielsen Apr 29 '15 at 14:03
  • 3
    `foreach` is one obvious case, and `AllDirections` might also be if it's a property. Also, it seems like you have a custom indexer on `grid` - that might also be the case. But above all, just fire up the memory profiler :) – Luaan Apr 29 '15 at 14:03
  • Can you show us the full method (signature also pls) – Tom Jonckheere Apr 29 '15 at 14:04
  • I also wanted to see the indexer `get` accessor for `grid[gridPosition + p]`. – Jeppe Stig Nielsen Apr 29 '15 at 14:05
  • 1
    an arguably more correct way to write your `if` statement would be to remove the `else` and reverse the conditions: `if (item == null || item.State == State.Initialized) return;`. Unless there is more code you aren't showing us. – Rufus L Apr 29 '15 at 14:10
  • Please, don't use `unity` tag for questions related to Unity game engine. Generally, it's a good idea to read the tag description before using it. – Max Yankov Apr 29 '15 at 14:16
  • 1
    @Sentrigal Value types won't put pressure on the GC, only new reference types. Is anything being created here other than potentially the iterator from the `foreach`? Also, Gen0 stuff is relatively cheap to collect so long as you aren't creating huge quantities in tight loops. The later gens start to get expensive. There is a lot of code hidden here that can be introducing the issue, and the profiler highlighting this method could be a red herring purely because this method highlights a problem elsewhere by being called so much. – Adam Houldsworth Apr 29 '15 at 14:27
  • Answered the comments in an edit. If that's not the usual way to do it I'm sorry, not really familiar with the protocol around here. – Sentrigal Apr 29 '15 at 14:53

2 Answers2

0

foreach calls GetEnumerator, which, in turn, creates a new instance of Enumerator every time it's called.

Community
  • 1
  • 1
Max Yankov
  • 12,551
  • 12
  • 67
  • 135
  • We would need to look at the IL to confirm that, the compiler has an optimisation in it to convert simple `foreach` calls to a `for` loop if the thing it is iterating is an array (not sure about `IList`), which then means no enumerator is created. – Adam Houldsworth Apr 29 '15 at 14:23
  • @AdamHouldsworth Yes, and the question seems to indicate that this is an array. Even with other types, there is often a `public` method `GetEnumerator` whose return type is a value type. So the enumerator "instance" will simply be a struct on the call stack in those cases, which again leads to no pressure on the garbage collector. – Jeppe Stig Nielsen Apr 29 '15 at 14:32
  • @JeppeStigNielsen Interesting, didn't know that. – Adam Houldsworth Apr 29 '15 at 14:32
  • Fair. However, getting the IL code out of C# compiler that Unity uses internally can be a little tricky; I'd suggest changing the loop type and comparing the profiler results as a much quicker way to determine that. – Max Yankov Apr 29 '15 at 14:33
  • @MaxYankov The IL will be of the user code, not the third party library. Assuming the above is user code. I refer to the IL of the `foreach` call in the written method, not of the enumerator. That said, if the property is an array you can make a very good guess that it is being optimised, and yes a quick way of determining that is to translate it to a `for` loop manually. – Adam Houldsworth Apr 29 '15 at 14:34
  • @AdamHouldsworth As an example, the public instance method `List<>.GetEnumerator` returns a value type. And `foreach` must use that (unless it cheats and converts to a `for` loop like you say), not the explicit interface implementation which returns a reference to a box with the same struct in it. *Edit:* Actually the last of the links in the answer above goes exactly to that method which returns a value of a struct, not a reference to a new heap object! – Jeppe Stig Nielsen Apr 29 '15 at 14:34
  • @AdamHouldsworth Unity uses an old version of Mono compiler, so, in order to get IL of that code, you'll have to get it out of it somehow. How do you suggest to do that? – Max Yankov Apr 29 '15 at 14:39
  • @MaxYankov No idea. I am just making the assumption that the C# code is being compiled into IL and then JITd. Either way this is moot as my assertion on the compiler optimisation is from the MS compiler, not the Mono one, so I cannot say with confidence that the same applies. – Adam Houldsworth Apr 29 '15 at 14:43
  • Actually, it's only jitted for some platforms — it was aot-compiled for iOS and Android before and translated to cpp for them now. – Max Yankov Apr 29 '15 at 18:30
0

So after Jeppe Stig Nielsen and Luaan made me remember that I'm infact using an indexer I did some googleing and found out that this is not a new issue. Apparently Dictonary.TryGetValue works by calling Equals(object o) on the keys. Now these have to be unboxed which, as far as I understand it, creates the garbage. I now followed the advice I found on Google and implemented the IEquatable interface in my Point3D struct, which does not requiere any boxing. And as it turns out, a big chunk of the garbage is gone.

So the generated garbage per call went from ~28B-1.8KB down to ~28B-168B. Which is pretty good, no more GC spikes. Though I'm still curious where the rest of the garbage is coming from.

Anyway, this was already a big help, so thanks for your answers guys.

Edit: Figured out where the rest of the garbage was comeing from. I just called base.GetHashCode, because I didn't know any better. I did some research into pretty much everything related to hash codes and then implemented it correctly. And now I'm down to exactly 0 bytes of garbage. Pretty neat.

Sentrigal
  • 61
  • 1
  • 4