GH-36103: [C++] Initial device sync API #37040

zeroshade · 2023-08-07T17:14:41Z

Rationale for this change

Building on the ArrowDeviceArray we need to expand the abstractions for handling events and stream synchronization for devices.

What changes are included in this PR?

Initial Abstract implementations for the new DeviceSync API and a CPU implementation. This will be followed up by a CUDA implementation in a subsequent PR.

Are these changes tested?

Yes, tests are added for Import/Export DeviceArrays using the DeviceSync handling.

Closes: [C++] Add device-specific synchronization API to Buffer #36103

zeroshade · 2023-08-07T17:14:49Z

CC @kkraus14

github-actions · 2023-08-07T17:15:11Z

⚠️ GitHub issue #36103 has been automatically assigned in GitHub to PR creator.

cpp/src/arrow/buffer.h

cpp/src/arrow/c/bridge.cc

kkraus14 · 2023-08-08T00:33:37Z

cpp/src/arrow/c/bridge_test.cc

+  }
+
+  static ArrayFactory JSONArrayFactory(const std::shared_ptr<MemoryManager>& mm,
+                                       std::shared_ptr<DataType> type, const char* json) {


Instead of a const char* here can we take a string_view? Not sure how we know what length the json is currently (unless it's assumed to be null terminated?)

Yea it's assumed to be null terminated for these tests

kkraus14 · 2023-08-08T00:36:43Z

cpp/src/arrow/gpu/cuda_context.cc

+  return nullptr;
+  // auto ev = reinterpret_cast<CUstream*>(sync_event);
+  // return std::make_shared<CudaDeviceSync>(ev);


Is this TODO?

Yea, in the next PR I'll implement the Cuda implementations ie: a CudaDeviceSync object etc.

bkietz · 2023-08-08T15:18:30Z

cpp/src/arrow/device.h

+class ARROW_EXPORT DeviceSync {
+ public:
+  explicit DeviceSync(void* sync_event) : sync_event_{sync_event}, owns_event_{false} {}
+  virtual ~DeviceSync() = default;


The compiler-generated default constructor won't call release_event; I think this destructor should probably do that

Suggested change

virtual ~DeviceSync() = default;

virtual ~DeviceSync() {

if (owns_event_) {

release_event(sync_event_);

}

}

The problem with this is that release_event is a virtual function which is a bad idea to call from a destructor, so the current semantics require explicitly calling release_event.

bkietz · 2023-08-08T15:22:19Z

cpp/src/arrow/device.h

+    return sync_event_;
+  }
+
+  void clear_event() {


Would this be used (instead of just destroying the DeviceSync)? I think it'd be best to keep the interface minimal until we can document the usage which each function anticipates

Yes, this has to be used instead of just destroying the DeviceSync for the same reason as above, it's not safe to call a polymorphic virtual function from a destructor.

bkietz · 2023-08-08T15:23:18Z

cpp/src/arrow/device.h

+  /// 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
+  virtual Status stream_wait(void* stream) = 0;


Similarly: it's not clear when this would be used instead of set_stream() then wait()

wait specifies that it blocks until the internal event is completed. stream_wait places a wait into the stream queue and will not block the CPU.

cpp/src/arrow/device.h

cpp/src/arrow/buffer.h

cpp/src/arrow/c/bridge.cc

Co-authored-by: Benjamin Kietzman <[email protected]>

cpp/src/arrow/device.h

cpp/src/arrow/c/bridge.cc

cpp/src/arrow/device.h

Co-authored-by: Antoine Pitrou <[email protected]>

pitrou · 2023-08-22T16:02:04Z

I posted a couple comments but we can probably go with this regardless. We'll probably end up changing those APIs a bit once comes the time to use them concretely.

conbench-apache-arrow · 2023-08-23T10:07:45Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit d062c89.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change Building on the `ArrowDeviceArray` we need to expand the abstractions for handling events and stream synchronization for devices. ### What changes are included in this PR? Initial Abstract implementations for the new DeviceSync API and a CPU implementation. This will be followed up by a CUDA implementation in a subsequent PR. ### Are these changes tested? Yes, tests are added for Import/Export DeviceArrays using the DeviceSync handling. * Closes: apache#36103 Lead-authored-by: Matt Topol <[email protected]> Co-authored-by: Benjamin Kietzman <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Matt Topol <[email protected]>

zeroshade requested review from bkietz and pitrou August 7, 2023 17:14

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Aug 7, 2023

zeroshade added 7 commits August 7, 2023 15:19

apacheGH-36103: [C++] Initial device-specific synchronization API

22c0c98

more tests

20d8781

some cleanup

e6ec912

linting stuff

c114b91

fix comments and cleanup

3ca47c8

lint

3978429

fix redundant 'virtual'

8a6148b

zeroshade force-pushed the event-stream-abstractions branch from 8cd91ac to 8a6148b Compare August 7, 2023 19:19

fix windows compile error

fd2edab

zeroshade requested a review from westonpace as a code owner August 7, 2023 20:09

kkraus14 reviewed Aug 8, 2023

View reviewed changes

bkietz requested changes Aug 8, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 8, 2023

Update cpp/src/arrow/device.h

23ba278

Co-authored-by: Benjamin Kietzman <[email protected]>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 9, 2023

updates from feedback

c12dedb

zeroshade requested a review from bkietz August 9, 2023 21:39

zeroshade added 2 commits August 10, 2023 12:27

updates from feedback

2405c85

fix test

6374cea

kkraus14 reviewed Aug 11, 2023

View reviewed changes

cpp/src/arrow/device.h Outdated Show resolved Hide resolved

cpp/src/arrow/device.h Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 11, 2023

github-actions bot added the awaiting changes Awaiting changes label Aug 21, 2023

shift stream wait to Stream object

de7c319

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 22, 2023

linting

9544495

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 22, 2023

pitrou reviewed Aug 22, 2023

View reviewed changes

cpp/src/arrow/c/bridge.cc Outdated Show resolved Hide resolved

pitrou reviewed Aug 22, 2023

View reviewed changes

cpp/src/arrow/device.h Outdated Show resolved Hide resolved

pitrou reviewed Aug 22, 2023

View reviewed changes

cpp/src/arrow/device.h Outdated Show resolved Hide resolved

pitrou reviewed Aug 22, 2023

View reviewed changes

cpp/src/arrow/device.h Show resolved Hide resolved

Update cpp/src/arrow/c/bridge.cc

0b74c00

Co-authored-by: Antoine Pitrou <[email protected]>

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 22, 2023

pitrou approved these changes Aug 22, 2023

View reviewed changes

rename to WrapDeviceSyncEvent

1fdcd0a

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 22, 2023

kkraus14 approved these changes Aug 22, 2023

View reviewed changes

zeroshade merged commit d062c89 into apache:main Aug 22, 2023
37 of 38 checks passed

zeroshade removed the awaiting changes Awaiting changes label Aug 22, 2023

zeroshade deleted the event-stream-abstractions branch August 22, 2023 19:08

zeroshade mentioned this pull request Aug 24, 2023

[C++][GPU] Add CUDA Implementation of Device Event/Stream #37364

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-36103: [C++] Initial device sync API #37040

GH-36103: [C++] Initial device sync API #37040

zeroshade commented Aug 7, 2023 •

edited by github-actions bot

Loading

zeroshade commented Aug 7, 2023

github-actions bot commented Aug 7, 2023

kkraus14 Aug 8, 2023

zeroshade Aug 9, 2023

kkraus14 Aug 8, 2023

zeroshade Aug 9, 2023

bkietz Aug 8, 2023

zeroshade Aug 8, 2023

bkietz Aug 8, 2023

zeroshade Aug 9, 2023

bkietz Aug 8, 2023

zeroshade Aug 8, 2023

pitrou commented Aug 22, 2023 •

edited

Loading

conbench-apache-arrow bot commented Aug 23, 2023

-  virtual ~DeviceSync() = default;
+  virtual ~DeviceSync() {
+    if (owns_event_) {
+      release_event(sync_event_);
+    }
+  }

GH-36103: [C++] Initial device sync API #37040

GH-36103: [C++] Initial device sync API #37040

Conversation

zeroshade commented Aug 7, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

zeroshade commented Aug 7, 2023

github-actions bot commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Aug 22, 2023 • edited Loading

conbench-apache-arrow bot commented Aug 23, 2023

zeroshade commented Aug 7, 2023 •

edited by github-actions bot

Loading

pitrou commented Aug 22, 2023 •

edited

Loading