cogciprocate / ocl

OpenCL for Rust
Other
723 stars 75 forks source link

Use Rc<> instead of opencl::retain_command_queue #143

Closed kpp closed 5 years ago

kpp commented 5 years ago

https://github.com/cogciprocate/ocl/blob/d9c2e1e494ac3059e316f60d0d5ccb40bc49ad28/ocl-core/src/types/abs.rs#L629

Why not wrap cl_command_queue in Rc to use local reference counter instead of sending ffi calls each CommandQueue::clone?

dmarcuse commented 5 years ago

One minor point - Queue is currently Send and Sync, while Rc is neither. Arc can be used instead, but then I'd argue that since the developer using the API can still use Rc<Queue> or Arc<Queue> if they desire, avoiding cloning the Queue and transitively, FFI calls.

kpp commented 5 years ago

One minor point - Queue is currently Send and Sync

But is it actually Send and Sync in C code?

dmarcuse commented 5 years ago

According to the OpenCL specification, command queues may be manipulated by multiple threads, but the developer is responsible for ensuring these occur in the correct order. I take this to mean it's both Send and Sync since events can be used to ensure the correct order is used.

c0gent commented 5 years ago

The reason it's done this way is simply because these pointers to OpenCL abstract types are already reference counted. Wrapping it in a Rust Arc would be redundant as we would have an additional pointer/indirection involved. (Rc would not be suitable as @dmarcuse pointed out -- OpenCL abstract type pointers are supposed to be atomic even though the types they reference may or may not be safe to access from different threads... but that's another story.)

Good question though! Let me know if that clears it up.

kpp commented 5 years ago

Now I see. Thanks.