GH-39782: [C++] Use correct (non-CPU) address of buffer in ExportDeviceArray#39783
GH-39782: [C++] Use correct (non-CPU) address of buffer in ExportDeviceArray#39783pitrou merged 8 commits intoapache:mainfrom
Conversation
|
|
|
I don't think we currently have non-CPU tests for the device interface. |
Good point. We should set up actual CUDA tests, IMHO. |
cpp/src/arrow/c/bridge.cc
Outdated
There was a problem hiding this comment.
Do we want to distinguish between C Data Interface and C Device Data Interface here? We could use buffer->data() for the former and buffer->address() for the latter - ensuring that non-CPU buffers cannot get exported over the C Data Interface.
There was a problem hiding this comment.
At the moment, this ArrayExporter is not aware of whether the ArrowArray came from a plain C Data array or a C Device array.
I could add that as a boolean flag to the class so we can distinguish based on that here?
|
@github-actions crossbow submit test-cuda-* |
|
Revision: a3910171759ec4940cf7f1cebf0783060f46c4d7 Submitted crossbow builds: ursacomputing/crossbow @ actions-904d985d92
|
|
I added a basic roundtrip test to |
cpp/src/arrow/c/bridge_test.cc
Outdated
There was a problem hiding this comment.
To avoid CUDA-specific code in non-CUDA directories, perhaps move this test somewhere in src/arrow/gpu?
There was a problem hiding this comment.
Yes, will move to cuda_test.cc
|
@github-actions crossbow submit test-cuda-* |
This comment was marked as outdated.
This comment was marked as outdated.
|
@pitrou updated to move the tests to |
pitrou
left a comment
There was a problem hiding this comment.
Pushed a small improvement. LGTM, thank you!
|
@github-actions crossbow submit test-cuda-* |
|
Revision: 48eac29 Submitted crossbow builds: ursacomputing/crossbow @ actions-6418ae939e
|
|
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 19e874f. There was 1 benchmark result indicating a performance regression:
The full Conbench report has more details. It also includes information about 10 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
The Array exporter currently uses
buffer->data(), which is documented as "The buffer has to be a CPU buffer (is_cpu()is true). Otherwise, an assertion may be thrown or a null pointer may be returned.".In practice, we indeed return null for non-CPU data. There is only a debug check that asserts the device.
This means that for non-CPU data the C device interface is currently returning null pointers for the buffers.
What changes are included in this PR?
Use the buffer's
address()instead ofdata()Are these changes tested?
Not yet