Fix thread-safety for the Serial backend #7080
Conversation
Co-authored-by: Nevin ":-)" Liber <nliber+github@gmail.com>
| if (it == all_instances.end()) | ||
| Kokkos::abort( | ||
| "Execution space instance to be removed couldn't be found!"); | ||
| std::swap(*it, all_instances.back()); |
There was a problem hiding this comment.
Giving it more thought: you don't even need swap, as assignment will do:
| std::swap(*it, all_instances.back()); | |
| *it = all_instances.back(); |
Sorry about that. The usual reason to prefer swap is when it is non-throwing, but pointer assignment doesn't throw either.
| @@ -147,7 +170,9 @@ Serial::Serial(NewInstance) | |||
| : m_space_instance(new Impl::SerialInternal, [](Impl::SerialInternal* ptr) { | |||
| ptr->finalize(); | |||
| delete ptr; | |||
| }) {} | |||
| }) { | |||
| m_space_instance->initialize(); | |||
There was a problem hiding this comment.
Is it necessary to initialize this explicitly here? Is this space not initialized along with other spaces during Kokkos::initialize() (by Kokkos::initialize_backends())?
There was a problem hiding this comment.
Kokkos::initialize only initializes default instances.
| @@ -147,7 +170,9 @@ Serial::Serial(NewInstance) | |||
| : m_space_instance(new Impl::SerialInternal, [](Impl::SerialInternal* ptr) { | |||
| ptr->finalize(); | |||
| delete ptr; | |||
| }) {} | |||
| }) { | |||
| m_space_instance->initialize(); | |||
There was a problem hiding this comment.
So this was an oversight right? It means that the m_is_initialized member was never set to true but that somehow didn't cause an error :/
There was a problem hiding this comment.
Right, before this pull request that only affected SerialInternal::is_initialized().
|
Only the which we also see in other pull requests. |
Splits the changes for the Serial backend from #6151.
This pull request ensures that kernels submitted to the same execution space instance from multiple threads don't run concurrently.