Setting component enabled thread safety | Official Unity Discord | Page 1

stark ferry Jun 24, 2023, 11:48 AM

#

@echo quarry sorry for ping but I'm pretty sure you're the one to confirm this with.

Is setting IEnableableComponent state thread safe?

Looking at the source it appears to be specifically written to be but I just want to confirm if this is supported behaviour before I depend on it.

Specifically I'm looking at this method where I noticed the old bit state is validated to not have changed.

        internal void SetComponentEnabled(Chunk* chunk, int indexInChunk, int typeIndexInArchetype, bool value)
        {
#if ENABLE_UNITY_COLLECTIONS_CHECKS || UNITY_DOTS_DEBUG
            // the bit array size is padded up to 64 bits, so we validate we're not indexing outside the valid data.
            if (Hint.Unlikely(indexInChunk < 0 || indexInChunk >= chunk->Capacity))
                throw new ArgumentException($"indexInChunk {indexInChunk} is outside the valid range [0..{chunk->Capacity-1}]");
#endif

            var bits = ChunkDataUtility.GetEnabledRefRW(chunk, typeIndexInArchetype, out var ptrChunkDisabledCount);
            var numStridesIntoBits = (indexInChunk / 64);
            var pBits = bits.Ptr + numStridesIntoBits;
            var indexInPBits = indexInChunk - (numStridesIntoBits * 64);
            var mask = 1L << indexInPBits;

            var oldBits = (long)*pBits;
            var newBits = 0L;
            var expectedOldBits = 0L;

            do
            {
                newBits = math.select(oldBits & ~mask, oldBits | mask, value);
                expectedOldBits = oldBits;
                oldBits = Interlocked.CompareExchange(ref UnsafeUtility.AsRef<long>(pBits), newBits, expectedOldBits);
            } while (expectedOldBits != oldBits);

            if (oldBits == newBits)
                return;

            // do we need increment or decrement?
            var adjustment = math.select(1, -1, value);
            Interlocked.Add(ref UnsafeUtility.AsRef<int>(ptrChunkDisabledCount), adjustment);
        }```

#

Yet I still noticed a write handle protecting the method

        public void SetComponentEnabled(Entity entity, bool value)
        {
#if ENABLE_UNITY_COLLECTIONS_CHECKS
            AtomicSafetyHandle.CheckWriteAndThrow(m_Safety);
#endif```
So you'd need to turn off safety to do this but it seems safe?

#

(I only want to enable so there's not going to be an unexpected state)

echo quarry Jun 26, 2023, 1:34 PM

#

Is setting IEnableableComponent state thread safe?
This is 100% the intent, yes. If we can't safely toggle enableable components from job code, that's a bug. (Subject to the usual caveats: e.g. if multiple threads try to toggle the same component from the same job, that's a race condition)

#

That said, the only way to toggle enableable components through a job is via a read/write ComponentLookup<T>, and a read/write ComponentLookup<T> requires you to disable safety checks for that access (specifically because we can't stop you from introducing race conditions with random-access writes from within a single job). So, the confusion about whether it's safe or not is justifiable! But it's as safe as any random-access write. And if it's not, please let us know!

#

The oldBits validation you see is probably an overly-cautious relic of IJobEntityBatch, which could potentially distribute entity ranges within a single chunk to multiple worker threads. In that case, even if each entity was "staying in its lane", you could potentially have two threads safely-in-theory modifying nearby bits independently, and we don't want the second writer to stomp the first writer's changes. That specific case is no longer possible in IJobChunk, so the extra atomicity of this operation is probably not needed any more. But if I were going to start optimizing SetComponentEnabled, that's not where I'd start; instead I'd get rid of the "chunk disabled count" stuff at the bottom of the function, which is only used for internal validation & has no business being in a release build at all.

stark ferry Jul 1, 2023, 3:55 AM

#

So I'm still experimenting a lot with enable components atm to see what's possible and m_PtrChunkDisabledCount makes things quite limiting on the potential I can reach
so if that was on the chopping block would be amazing

#

i already wrote an extensions to copy a mask from 1 component to another

public void Execute(in ArchetypeChunk chunk, int unfilteredChunkIndex, bool useEnabledMask, in v128 chunkEnabledMask)
{
    chunk.CopyEnableMaskFrom(ref this.EffectActivePreviousHandle, ref this.EffectActiveHandle);
}```
```cs
// ...
var dst = ChunkDataUtility.GetEnabledRefRW(
    archetypeChunk.m_Chunk, destination.m_LookupCache.IndexInArchetype, destination.GlobalSystemVersion, out var dstPtrChunkDisabledCount).Ptr;

var src = ChunkDataUtility.GetEnabledRefRO(archetypeChunk.m_Chunk, source.m_LookupCache.IndexInArchetype).Ptr;

// ..
// UnsafeUtility.MemCpy(dst, src, sizeof(ulong) * 2);
dst[0] = src[0];
dst[1] = src[1];
*dstPtrChunkDisabledCount = *srcPtrChunkDisabledCount;```
which is pretty clean and works

#

but I'd also love to be able to do this conditionally

v128 effectOnDuration = chunk.GetEnableableBits(ref this.EffectOnDurationHandle);
v128 conditionAllActive = chunk.GetEnableableBits(ref this.ConditionAllActiveHandle);
v128 effectOnCooldown = chunk.GetEnableableBits(ref this.EffectOnCooldownHandle);

ref v128 effectActive = ref chunk.GetEnableableBitsRW(ref this.EffectActiveHandle); // this is what I want to invent
effectActive.ULong0 = effectOnDuration.ULong0 | (conditionAllActive.ULong0 & !effectOnCooldown.ULong0);
effectActive.ULong1 = effectOnDuration.ULong1 | (conditionAllActive.ULong1 & !effectOnCooldown.ULong1);```
it'd be so efficient
but this isn't going to work because it obviously doesn't respect m_PtrChunkDisabledCount

#

previously these were all bool Value components

var effects = (EffectActive*)chunk.GetRequiredComponentDataPtrRW(ref this.EffectActiveHandle);
for (var index = 0; index < length; index++)
{
    effects[index] = new EffectActive
    {
        Value = effectOnDurations[index].Value | (conditionAllActives[index].Value & !effectOnCooldowns[index].Value),
    };
}```
that I'm experimentally replacing with enablable

#

Now I'm not asking for official support for any of this, I'm quite happy to write my own 'unsafe' extensions to entities. Just pointing out if you did

get rid of the "chunk disabled count" stuff at the bottom of the function, which is only used for internal validation & has no business being in a release build at all.
it would open up a world of possibilities!

#

I'm really seeing a lot of potential for ienableable the more I experiment and think about it

stark ferry Jul 2, 2023, 4:15 AM

#

i guess for the above example i can just do a countbits() and update it myself 🤔

stark ferry Jul 2, 2023, 8:08 AM

#

well I seem to have it working

#Setting component enabled thread safety