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

Make SimSYCL thread-safe #14

Merged
merged 3 commits into from
Dec 9, 2024
Merged

Make SimSYCL thread-safe #14

merged 3 commits into from
Dec 9, 2024

Conversation

fknorr
Copy link
Collaborator

@fknorr fknorr commented Nov 30, 2024

Overall, SimSYCL has very little shared mutable state:

Fiber scheduling always happens inside a parallel_for, so it will never cross thread boundaries. The corresponding cooperative_schedule and "return fiber" can be made thread-local.

For where there is globally visible shared state (system config, USM allocations, buffer access validation), I've introduced a global lock (named system_lock, which is backed by a singleton mutex). All shared mutable state is now explicitly wrapped in a mutable shared_state<>, which only grants access to its interior when a system_lock is in scope.

To guarantee command-group ordering across threads without tracking dependencies explicitly, submit (and all similar functions) hold the system_lock while they're running.

With this patch, Celerity works fine on my machine ™️ with workarounds removed.

Fiber scheduling is now thread local, and truly thread-shared data structures are protected by a global lock.
@fknorr fknorr requested a review from PeterTh November 30, 2024 13:21
@fknorr fknorr self-assigned this Nov 30, 2024
Copy link
Collaborator

@PeterTh PeterTh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fknorr fknorr merged commit b71bad5 into master Dec 9, 2024
16 checks passed
@fknorr fknorr deleted the thread-safety branch December 9, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants