bkietz commented on code in PR #37040:
URL: https://github.com/apache/arrow/pull/37040#discussion_r1298567614
##########
cpp/src/arrow/device.h:
##########
@@ -98,6 +101,71 @@ class ARROW_EXPORT Device : public
std::enable_shared_from_this<Device>,
/// \brief Return the DeviceAllocationType of this device
virtual DeviceAllocationType device_type() const = 0;
+ /// \brief EXPERIMENTAL: An object that provides event/stream sync primitives
+ class ARROW_EXPORT SyncEvent {
+ public:
+ virtual ~SyncEvent() = default;
+
+ /// @brief Block until sync event is completed.
+ virtual Status wait() = 0;
+
+ /// @brief Make the provided stream wait on the sync event.
+ ///
+ /// Tells the provided stream that it should wait until the
+ /// synchronization event is completed without blocking the CPU.
+ /// @param stream Should be appropriate for the underlying device
Review Comment:
I was thinking that `Device::Stream` would be an opaque and empty base
class, mostly just a wrapper around `void*` to make it more clear that it's a
Device-specific object. No ownership is necessary here since as @pitrou noted
these won't be owned by arrow and users of this API will not want to maintain a
`map<CUstream, std::shared_ptr<Device::Stream>>` in addition to their own
stream pooling. Instead, subclasses of `Device::Stream` should be trivially
constructible from their native counterparts.
```c++
class Device {
// ...
/// A synchronization stream in which SyncEvents may be sequenced for non
/// host/CPU devices.
class Stream {
public:
virtual const void* get_raw() const { return nullptr; }
protected:
Stream() = default;
};
class SyncEvent {
// ...
virtual Status StreamWait(const Stream&) = 0;
virtual Status Record(const Stream&) = 0;
};
};
// CPU device can have its own placeholder stream, if necessary
const Stream& CpuDevice::stream() { ... }
```
A subclass would be pretty trivial:
```c++
class CudaDevice {
// ...
class Stream : public Device::Stream {
public:
const void* get_raw() const override { return &stream; }
const CUstream stream;
explicit Stream(CUstream stream) : stream{stream} {}
// if devices have special stream values those can be specified here
static Stream default_() { return Stream{0}; }
};
};
```
... usage would just involve constructing the appropriate Stream whenever a
sync event function is called:
```c++
CUstream stream = GetStreamFromPool();
// ...
event->StreamWait(CudaDevice::Stream{stream});
```
It's pretty trivial, but I think it wraps the intent into more OO-obvious
terms
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
