We have concurrent code running that is adding gameplay tags to different FGameplayTagContainer instances in a theadsafe way. These are adding tags via calls to FGameplayTagContainer::AddTagFast, which in turn calls UGameplayTagsManager::ExtractParentTags for the tag getting added. Although all of these calls are read only (to find the parent tags for the added tag), and do not occur at a time in the frame when UGameplayTagsManager would be written to, the critical section is causing unnecessary lock contention.
We’re planning on a local engine change to UGameplayTagsManager, changing from using a FTransactionallySafeCriticalSection to a UE::FRWSpinLock for synchronization. Do you anticipate any issues with this change, or perhaps have an alternative solution for allowing fast concurrent usage of different FGameplayTagContainer instances, adding and removing tags from them?
Thanks.
[Attachment Removed]
Hi there,
The scope lock was added to prevent serialised gameplay tag containers being de-serialised before the tags are registered, resulting in incorrect parent tags, and corrupting them.
FTransactionallySafeCriticalSection allows for the same thread to aquire the lock multiple times, and only when the entire the thread is finished with the lock entirely that it can then be used by other threads, that could be where you are running into issues cocurrently adding tags.
I cannot find a case in the engine where FRWSpinLock is being used, to use as a comparision.
As the critial section lock is shared for different parts of the gameplay tag manager, changing this could re-introduce serialisation bug, specially from background loading threads.
Are adding tags concurrently for performance reasons, or is it just comming from a various threads at the same time?
A note on performance, FGameplayTagContainer is backed by a TArray, calling FGameplayTagContainer::Reset with a reserve slack size, can improve performance when adding a lot of tags as it will pre allocate the memory instead of potentially re-allocating a lot of memory.
Kind Regards
Keegan Gibson
[Attachment Removed]
Thanks for the reply. I’m locking around the same critical sections with the FRWSpinLock, but explicitly locking for read or write access -- I think this should keep the required thread safety for addressing the serialization issue, but will keep an eye out.
We’re adding from multiple threads concurrently for performance reasons; we’re using the gameplay tag containers to control some animation logic from gameplay, and want to update the tags that are set on a large number of entities, which should be able to run concurrently, as they are generally operating only on the state associated with each entity. The only issue we were running into with running this step in parallel was the contention on the lock for looking up the parent tags.
[Attachment Removed]
I think locking for reads vs writes is a really good idea. That should hopefully allow the parent tags to be popluated without worrying about the tag map being modified.
Another option, but is less recomended then what you are doing already
You could create a new version of add tag fast that calls a new version of extract parent tags without the critical section, as long as you could assure that nothing would be modifying GameplayTagNodeMap during the find.
If you do i suggest adding some debug checks around it so you could ■■■■■ the likelyhood of it.
[Attachment Removed]