Excessive memory allocation in MyLargeTurretTargetingSystem

Guest shared this bug 2 years ago
Solved

Game version: 1.200.032

Both server and client, but worse on multiplayer servers.

Problem: Memory allocations in frequently called methods of MyLargeTurretTargetingSystem.

How to reproduce:

  • Download and extract the test world (OneDrive, due to its size):
    https://1drv.ms/u/s!AqEgz8G_d8TSh8wPzLoPp_dQDBRb7g?e=eBhsuv
  • Load the test world into DS or client.
  • Use dotTrace to capture a timeline snapshot for 10 seconds.
  • Select .NET Memory Allocations in the Events filter.
  • The turret targeting system methods listed below should be at or near the top of the hotspot list.

Affected methods

  • SortTargetRoots
  • UpdateVisibilityCache and the whole visibility cache logic

Please see the memory profiler screenshot attached, see the top 2 items listed.

Remarks

  • Do not use ArrayExtensions.EnsureCapacity if you don't need to keep the existing array items. Whenever it needs to "grow" the array, it creates a new one and copies all items over! We don't need it if the goal is only to reuse the array.
  • Reuse collections as much as possible. Some of them reused, some others are missed and a new one is always allocated. It imposes GC pressure, causes slowness and lag.
  • Do not use ConcurrentDictionary, it has horrible performance. You are way better off by just using a simple Dictionary<K, V> generic type and lock the whole collection on access.

Please see the related patches in the Performance Improvements plugin on how to fix the above.

Search for: MyLargeTurretTargetingSystemPatch

Replies (4)

photo
1

Hello, Viktor!

Sorry, you're experiencing these issues. I will admit I don't have a huge amount of knowledge of dotTrace but have followed your instructions. When viewing the hotspots afterwards, I'm not able to see a mention of the targeting system that you have in yours. Should I be doing something in-game to activate this? I apologise for what is probably a silly question. I'd like to get this reproduced but so far after multiple attempts, this is not showing for me. Any tips in the right direction would be excellent.

Kind Regards

Laura, QA Department

photo
1

The turret controller's target visibility cache logic is slow, primarily due to allocating too much memory. You don't need to do anything in the world, it is enough to have a lot of AI controlled turrets with other grids in their range (they don't have to be enemy ones).

For dotTrace settings see the attached screenshot on the visibility cache update method. Follow the yellow highlighting.

As I already described in the ticket's description (please read the whole text), the cache is based on ConcurrentDictionary, which has horrible performance. Also, when the code calls Clear on such a dictionary it just re-allocates its hash table. So clearing does not actually reuse the data structure, your optimization attempt was futile. Always verify the library implementation instead of making assumptions about its time/memory complexity. Otherwise you can run into surprises, like this one.

Know what is the goal of your target visibility cache.

Each target for each turret controller can be in one of the following 3 states:

  • Not cached (visibility is unknown)
  • Cached as visible
  • Cached as not visible

Define clear expiration times in simulation ticks for each of the cached states. Do not use a frequent full scan of the dict to remove expired item, just sore an expiration timestamp (future tick count) within the item and ignore expired items on lookup. Then you can do the cleanup rarely and at a limited rate.


Please see how I implemented it using the above strategy with a single dictionary and R/W locking, which is much faster and seems to work well in practice: https://github.com/viktor-ferenczi/performance-improvements/tree/main/Shared/Patches/TargetingSystem

The IL files are there only for comparison, to see how the transpiler patches change the code to call my implementation. The relevant part is in the cs file and outside in the Share.Tools namespace where the UintCache object is defined. I use a custom RwLock, but it could be some other locking you're using.

Also, try to avoid memory allocations at performance critical parts which are called very frequently. They are evil, because they not only consume CPU power, but summons a GC collection run later causing lag, even if running in parallel.

You can find SortTargetRoots as well in a similar way, please see under Remarks in the ticket description on what's the problem there with array resizing.

Please leave a comment if something is not clear.

photo
photo
1

You changed the logic in game version 1.202.048 (Automaton Beta), but it is still using ToArray() and EnsureCapacity, making it slow due to excess memory allocation.


Please re-read the ticket and the comments above above, then optimize to allocate as little memory as possible the least often as possible. Object reuse can do wonders to reduce/eliminate lag. Otherwise combat on multiplayer servers will be sluggish.


I give up patching your changing code, please fix it properly on your side.

photo
1

Hello, Viktor,

thanks for updating this thread.

Issue was reported into our internal system.

Kind Regards

Keen Software House: QA Department

photo
1

Hello, Engineers!


We're happy to inform you that the upcoming "203" update contains a fix for the bug you have reported. Thank you for taking your time to inform us about this issue and making Space Engineers better.


If you are still experiencing the bug on the new version, please let us know by commenting here or opening a new thread.


We are closing this thread as "Solved".


Kind Regards,

Keen Software House: QA Department

Leave a Comment
 
Attach a file