Post by andrew-raphael-lukasik in Space Architect comments

Viewing post in Space Architect comments

andrew-raphael-lukasik5 years ago (7 edits) (+1)

(0.12.6) Bug: GeneralItemPanel.Update method is allocating >10MB of garbage memory PER SECOND.

This leads to GC.Collect() calls causing this pattern of CPU spikes:

This 180KB seems like nothing... until you actually multiply it by your framerate.
One probably won't even notice this bump on high-end machine but it can seriously throttle any weaker laptop or older pc. And for no valid reason really.

PS: I have no idea from where this Instantiate call is coming from but it looks like that is the biggest offender here allocation-wise (consider object pooling).

andrew-raphael-lukasik5 years ago (5 edits)

Few words of friendly comments:

void Update ()
{
    // *** - ToString allocates at least 2 bytes for every character
    // **** - adding strings together with '+' operator is more or less equivalet to ***
    ItemContainer container = SingletonMB<StationContent>.Instance.GetContainer();
    TextMeshProUGUI title = this.title;// this does nothing - remove line
    float num = container.CapacityUsed;
    string str1 = num.ToString("0");// ***
    num = container.Capacity;// reusing local variables is not needed for anything
    string str2 = num.ToString("0");// ***
    string str3 = "Items " + str1 + " / " + str2;// ****
    title.text = str3;
    this.sectionLines.RemoveAllLines();
    foreach( ItemStack itemStack in container.Items )// unfortunately, starting 'foreach' allocates memory - in Update methods try to use simpler old 'for' iterator instead of 'foreach'
    {
        PanelSectionLines sectionLines = this.sectionLines;// this does nothing - remove line
        ItemType type = itemStack.Type;
        string _identifier = type.ToString();// ***
        type = itemStack.Type;// this does nothing - remove line
        string _text = type.ToString() + " " + itemStack.Amount.ToString("##");// ****
        sectionLines.Set(_identifier, _text);
    }
}

Downsides of ToString() calls are not that simple to get rid of in context of UI. But it's better that you're aware of it and step-by-step will be trying to avoid them in the future (strings in general).

andrew-raphael-lukasik5 years ago (5 edits)

After cleaning code up just a bit it can be like this (cleaner to read/understand imho):

void Update ()
{
    ItemContainer container = SingletonMB<StationContent>.Instance.GetContainer();
    title.text = string.Format( "Items {0:0} / {1:0}" , container.CapacityUsed , container.Capacity );
    sectionLines.RemoveAllLines();
    int numContainerItems = container.Items.Count;
    for( int i=0 ; i<numContainerItems ; i++ )
    {
        ItemStack itemStack = container.Items[i];
        string identifier = itemStack.Type.ToString();
        string text = string.Format( "{0} {1:##}" , identifier , itemStack.Amount );
        sectionLines.Set( identifier , text );
    }
}

Note that 'string.Format()' is slightly better than '"strA"+"strB"'

Also System.Text.StringBuilder would be better (string.Format uses it internally).

andrew-raphael-lukasik5 years ago (3 edits)

To give you an example of how to get rid of string allocations think about creating lookup tables for your values (enum is easy and natural candidate for that). This is how you create an lookup table for enums:

public static readonly Dictionary<ItemType,string> ItemTypeStrings = new Dictionary<ItemType,string>{
            { ItemType.Metal, "Metal" } ,
            { ItemType.Food, "Food" } ,
            { ItemType.Water, "Water" } ,
            { ItemType.Mineral, "Mineral" } ,
            { ItemType.Dirt, "Dirt" } ,
            { ItemType.WasteWater, "Waste Water" } ,
            { ItemType.LOX, "LOX" } ,
            { ItemType.LH2, "LH2" } ,
            { ItemType.AsteroidaldWater, "Asteroidald Water" } ,
            { ItemType.MetalOre, "Metal Ore" } ,
            { ItemType.Component , "Component" } ,
            { ItemType.None , "None" } ,
        };

And that will give you an ability to replace every:

string myTypeString = itemStack.Type.ToString();//allocation

with:

string myTypeString = ItemTypeStrings[ itemStack.Type ];

which will allocate no memory at all (==what we want)

Tazoru5 years ago (4 edits) (+1)

This part of code is just my prototyping/lazy programming left for later :D I've wanted to base it on event instead of leaving it in update, but forgot about it when preparing actual event :/

Adding your suggestions (with exception for foreach, just for readability sake) and moving it to be based on event it now looks and perform a lot better :)

void Start()
{
    sectionLines = GetComponent<PanelSectionLines>();
    title = transform.Find("Title").GetComponentInChildren<TextMeshProUGUI>();
    StationContent.Instance.OnContentChanged += HandleStationContentChanged;
}
private void HandleStationContentChanged(StationContent _stationContent)
{
    ItemContainer container = _stationContent.GetContainer();
    title.text = string.Format("Items {0:0} / {1:0}", container.CapacityUsed, container.Capacity); 
    sectionLines.RemoveAllLines();
    foreach (ItemStack stack in container.Items)
    {
       sectionLines.Set(Lang.ITEM_TYPE[stack.Type], string.Format("{0} {1:##}", Lang.ITEM_TYPE[stack.Type], stack.Amount));
    }
}

andrew-raphael-lukasik5 years ago (5 edits)

That's great!

PS: Please don't over-rely on events (outside UI ofc). Those are 10x times slower and complicate future debugging considerably. If something can be just a method call - let it be just that, you will thank yourself for that later on (less time spent debugging==more productivity==happier you).

andrew-raphael-lukasik5 years ago (7 edits)

Easy (but lazy) solution to most of those allocations would be to use LazyCacheStrings and change your code into something like this:

public static LazyCacheStrings<ItemType> ItemTypeStrings = new LazyCacheStrings<ItemType>( (t)=>t.ToString() );
static LazyCacheStrings<System.ValueTuple<float,float>> TitleStrings = new LazyCacheStrings<System.ValueTuple<float,float>>( (kv)=>string.Format("Items {0:0} / {1:0}",kv.Item1,kv.Item2) );
static LazyCacheStrings<System.ValueTuple<ItemType,float>> SectionStrings = new LazyCacheStrings<System.ValueTuple<ItemType,float>>( (kv)=>string.Format("{0} {1:##}",ItemTypeStrings[kv.Item1],kv.Item2) );
void Update ()
{
    ItemContainer container = SingletonMB<StationContent>.Instance.GetContainer();
    title.text = TitleStrings[ ( container.CapacityUsed , container.Capacity ) ];
    sectionLines.RemoveAllLines();
    int numContainerItems = container.Items.Count;
    for( int i=0 ; i<numContainerItems ; i++ )
    {
        ItemStack itemStack = container.Items[i];
        sectionLines.Set(
            ItemTypeStrings[ itemStack.Type ] ,
            SectionStrings[ ( itemStack.Type , itemStack.Amount ) ]
        );
    }
}

This should limit worst of this runaway allocations just some seconds after game start (once caches fill up). It's not the best solution but certainly the simplest one to implement.

itch.io

Viewing post in Space Architect comments

(0.12.6) Bug: GeneralItemPanel.Update method is allocating >10MB of garbage memory PER SECOND.