Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TaskDriver - Improve Safety of JobData Query Operations #289

Open
mbaker3 opened this issue Oct 24, 2023 · 1 comment
Open

TaskDriver - Improve Safety of JobData Query Operations #289

mbaker3 opened this issue Oct 24, 2023 · 1 comment
Labels
effort-medium Standard - 2 to 4 hours priority-medium Standard task, plan as you see fit. type-chore Clean-up and optimization

Comments

@mbaker3
Copy link
Member

mbaker3 commented Oct 24, 2023

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:

  • 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.
  • On each Get*FromQuery<> request check the destination field's attributes and ensure they align with the access type requested in the query.
@mbaker3 mbaker3 added effort-medium Standard - 2 to 4 hours priority-medium Standard task, plan as you see fit. type-chore Clean-up and optimization labels Oct 24, 2023
@jkeon
Copy link
Member

jkeon commented Oct 24, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort-medium Standard - 2 to 4 hours priority-medium Standard task, plan as you see fit. type-chore Clean-up and optimization
Projects
None yet
Development

No branches or pull requests

2 participants