decline-cookies / anvil-unity-dots

Unity DOTS and ECS specific additions and extensions to Anvil
MIT License
4 stars 1 forks source link

TaskDriver - Improve Safety of JobData Query Operations #289

Open mbaker3 opened 10 months ago

mbaker3 commented 10 months ago

In a job configured and run through IJobConfig/AbstractJobData (usually a job in a TaskDriver) it's currently on the developer to remember to add [ReadOnly] attributes to any NativeArray's they are populating from a query when they don't plan to write to the elements. For example via AbstractJobData.GetIComponentDataNativeArrayFromQuery and AbstractJobData.GetEntityNativeArrayFromQuery.

Unity uses the prsence of a [ReadOnly] attribute on the field to determine if the component is read only even if the query only specified read only access (Ex: ComponentType.ReadOnly<MyComponent>)

If a developer forgets the attribute NativeArray<T>.FailOutOfRangeErrror will eventually trigger when two jobs go to get read access to the component at the same time.

Ideally, the developer shouldn't have to remember to add the atribute. If they do have to remember then our safety system should let them know if the discrepency as soon as possible. Waiting until a race condition actually comes up at runtime is too late.

Some possible approaches:

jkeon commented 10 months ago

Wrap array access in a type reader. We're not sure whether putting the [ReadOnly] attribute on the field inside the wrapper will correctly inform Unity's safety system but this is ideal.

I think this does work correctly. We're using it on our CDFEReader and other "Reader" wrappers today. I've definitely gotten errors before where I didn't specify the access on an inner field in the wrapper and then tried to use it in a context where it needed to be marked as ReadOnly.