Make matrix_cl thread safe #2905
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Another idea would be to just have a ring buffer with fixed capacity and before we push onto the queue we have a spin lock to wait till space is available |
SteveBronder
commented
May 17, 2023
SteveBronder
commented
May 17, 2023
Comment on lines
+619
to
+640
| cl::Event cstr_event; | ||
| std::vector<cl::Event>* dep_events = new std::vector<cl::Event>( | ||
| A.write_events().begin(), A.write_events().end()); | ||
| try { | ||
| cl::Event cstr_event; | ||
| opencl_context.queue().enqueueCopyBuffer(A.buffer(), this->buffer(), 0, 0, | ||
| A.size() * sizeof(T), | ||
| &A.write_events(), &cstr_event); | ||
| A.size() * sizeof(T), dep_events, | ||
| &cstr_event); | ||
| if (opencl_context.device()[0].getInfo<CL_DEVICE_HOST_UNIFIED_MEMORY>()) { | ||
| buffer_cl_.setDestructorCallback( | ||
| &delete_it_destructor<std::vector<cl::Event>>, dep_events); | ||
| } else { | ||
| cstr_event.setCallback( | ||
| CL_COMPLETE, &delete_it_event<std::vector<cl::Event>>, dep_events); | ||
| } | ||
| this->add_write_event(cstr_event); | ||
| A.add_read_event(cstr_event); | ||
| } catch (const cl::Error& e) { | ||
| delete dep_events; | ||
| check_opencl_error("copy (OpenCL)->(OpenCL)", e); | ||
| } catch (...) { | ||
| delete dep_events; | ||
| throw; |
There was a problem hiding this comment.
This is a place where things get weird with the tbb concurrent vector. We need a local copy to hang around till this process is finished executing which involves making a raw pointer which we then use a callback on the buffer or copy event to destroy
3 tasks
Merged
3 tasks
|
@rok-cesnovar sorry to bother but could you look at this? |
|
Looking into this over the weekend. |
rok-cesnovar
approved these changes
Nov 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This makes
matrix_cl's event vectors thread safe by usingtbb::concurrent_vectorinstead of astd::vector.In Stan a
matrix_clcan be in a model class and shared across multiple threads. When using multiple threads we can have a race condition where two operations attempt to push to the vector of read/write events.I think the simplest fix for this is the below where we replace
std::vectorwithtbb::concurrent_vector. This has some issues though, notably thattbb::concurrent_vectoris not a contiguous block of memory. Some functions likeclenqueueReadBufferexpect a pointer to memory and the size of the block of memory so I end up doing is making a hard copy of thetbb::concurrent_vectorinto anstd::vector. This is not optimal for performance.If we wanted to I could write something based on rigtorp's
ring_bufferbut for just a vector of data. The only odd part of it would be when we have to resize the vector we would have to make a hard copy of the events into a new slice of memory and keep the old memory around until either the buffer falls out of scope or the last event is done. I'm fine with doing that as well.Tests
I'm also not sure how to write a test for this? The current tests all pass (in fact I found a small bug in normal_lccdf where the results didn't match and fixed that).
Side Effects
The bad thing to note here is the copy every time we need a contiguous block of memory for queuing the events for read/write ops. This is not great, though it will be fine for now if we keep it this way and then later I can write a faster scheme.
Release notes
Replace the
std::vectors for read/write events inmatrix_clwithtbb::concurrent_vectorsChecklist
Math issue #(issue number)
Copyright holder: Steve Bronder
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested