You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alanstoate (via GitHub)" <gi...@apache.org> on 2024/04/30 21:32:36 UTC
[PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
alanstoate opened a new pull request, #41477:
URL: https://github.com/apache/arrow/pull/41477
### Rationale for this change
Currently ```MemoryManager``` objects define functionality to Copy or View entire buffers. Occasionally there is the need to only copy a single value or slice from a buffer (see https://github.com/apache/arrow/pull/39770#discussion_r1470135438). It's overkill to do a bunch of whole Buffer operations and manually slicing just to copy 4 or 8 bytes.
Instead we can add new methods to the ```MemoryManager``` interface such as ```CopyBufferSlice``` etc.
### What changes are included in this PR?
Add slice versions of the ```MemoryManager``` interface methods:
- ```CopyBufferSliceFrom```
- ```CopyBufferSliceTo```
- ```CopyNonOwnedSliceFrom```
- ```CopyNonOwnedSliceTo```
- ```ViewBufferSliceFrom```
- ```ViewBufferSliceTo```
As well as ```CPUMemoryManager``` overrides.
Add ```static Result<std::shared_ptr<Buffer>> ViewOrCopySlice```
Add ```static Result<std::shared_ptr<Buffer>> CopyBufferSlice``` & ```static Result<std::shared_ptr<Buffer>> ViewBufferSlice```
-### Are these changes tested?
todo
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "alanstoate (via GitHub)" <gi...@apache.org>.
alanstoate commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2094568477
@zeroshade @felipecrv thanks for the feedback and happy to refactor a bit
So if I'm understanding correctly we want a ``` CopyBufferSlice ``` that copies from a buffer using the ```MemoryManager``` into a raw pointer?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "alanstoate (via GitHub)" <gi...@apache.org>.
alanstoate commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2087419948
Hi @zeroshade I've had a go at adding these MemoryManager methods, just a few q's:
- I've added all the copy/view methods in MemoryManager as slice versions I'm not sure if this is more than requested?
- Should the existing copy/view methods in CPUMemoryManager call these slice versions with ```offset = 0``` & ```length = buf.size()``` to reduce code replication?
- I should probably add a test for all of these, just wondering where that should live?
Thanks!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41477:
URL: https://github.com/apache/arrow/pull/41477#discussion_r1604244038
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -1868,24 +1868,18 @@ struct ArrayImporter {
template <typename OffsetType>
Status ImportStringValuesBuffer(int32_t offsets_buffer_id, int32_t buffer_id,
int64_t byte_width = 1) {
- if (device_type_ == DeviceAllocationType::kCPU) {
- auto offsets = data_->GetValues<OffsetType>(offsets_buffer_id);
+ int64_t last_offset_value_offset =
+ (c_struct_->length + c_struct_->offset) * sizeof(OffsetType);
+ OffsetType last_offset_value;
+ RETURN_NOT_OK(MemoryManager::CopyBufferSlice(
+ data_->buffers[offsets_buffer_id], last_offset_value_offset, sizeof(OffsetType),
+ reinterpret_cast<uint8_t*>(&last_offset_value)));
+
+ int64_t buffer_size = 0;
+ if (c_struct_->length > 0) {
// Compute visible size of buffer
- int64_t buffer_size =
- (c_struct_->length > 0) ? byte_width * offsets[c_struct_->length] : 0;
- return ImportBuffer(buffer_id, buffer_size);
- }
-
- // we only need the value of the last offset so let's just copy that
- // one value from device to host.
- auto single_value_buf =
- SliceBuffer(data_->buffers[offsets_buffer_id],
- c_struct_->length * sizeof(OffsetType), sizeof(OffsetType));
- ARROW_ASSIGN_OR_RAISE(
- auto cpubuf, Buffer::ViewOrCopy(single_value_buf, default_cpu_memory_manager()));
- auto offsets = cpubuf->data_as<OffsetType>();
- // Compute visible size of buffer
- int64_t buffer_size = (c_struct_->length > 0) ? byte_width * offsets[0] : 0;
+ buffer_size = (c_struct_->length > 0) ? byte_width * last_offset_value : 0;
Review Comment:
```suggestion
buffer_size = byte_width * last_offset_value;
```
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -1868,24 +1868,18 @@ struct ArrayImporter {
template <typename OffsetType>
Status ImportStringValuesBuffer(int32_t offsets_buffer_id, int32_t buffer_id,
int64_t byte_width = 1) {
- if (device_type_ == DeviceAllocationType::kCPU) {
- auto offsets = data_->GetValues<OffsetType>(offsets_buffer_id);
+ int64_t last_offset_value_offset =
+ (c_struct_->length + c_struct_->offset) * sizeof(OffsetType);
+ OffsetType last_offset_value;
+ RETURN_NOT_OK(MemoryManager::CopyBufferSlice(
+ data_->buffers[offsets_buffer_id], last_offset_value_offset, sizeof(OffsetType),
+ reinterpret_cast<uint8_t*>(&last_offset_value)));
+
+ int64_t buffer_size = 0;
+ if (c_struct_->length > 0) {
Review Comment:
```suggestion
int64_t buffer_size = 0;
if (c_struct_->length > 0) {
int64_t last_offset_value_offset =
(c_struct_->length + c_struct_->offset) * sizeof(OffsetType);
OffsetType last_offset_value;
RETURN_NOT_OK(MemoryManager::CopyBufferSlice(
data_->buffers[offsets_buffer_id], last_offset_value_offset, sizeof(OffsetType),
reinterpret_cast<uint8_t*>(&last_offset_value)));
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2096211969
That's my understanding. @felipecrv that sound about right to you?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2093194720
@alanstoate what do you think of @felipecrv's feedback and refactoring a bit based on his suggestions?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "alanstoate (via GitHub)" <gi...@apache.org>.
alanstoate commented on code in PR #41477:
URL: https://github.com/apache/arrow/pull/41477#discussion_r1597340834
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -1868,24 +1868,15 @@ struct ArrayImporter {
template <typename OffsetType>
Status ImportStringValuesBuffer(int32_t offsets_buffer_id, int32_t buffer_id,
int64_t byte_width = 1) {
- if (device_type_ == DeviceAllocationType::kCPU) {
- auto offsets = data_->GetValues<OffsetType>(offsets_buffer_id);
- // Compute visible size of buffer
- int64_t buffer_size =
- (c_struct_->length > 0) ? byte_width * offsets[c_struct_->length] : 0;
- return ImportBuffer(buffer_id, buffer_size);
- }
-
- // we only need the value of the last offset so let's just copy that
- // one value from device to host.
- auto single_value_buf =
- SliceBuffer(data_->buffers[offsets_buffer_id],
- c_struct_->length * sizeof(OffsetType), sizeof(OffsetType));
- ARROW_ASSIGN_OR_RAISE(
- auto cpubuf, Buffer::ViewOrCopy(single_value_buf, default_cpu_memory_manager()));
- auto offsets = cpubuf->data_as<OffsetType>();
+ int64_t last_offset_value_offset =
+ c_struct_->length * sizeof(OffsetType) + c_struct_->offset;
Review Comment:
@felipecrv @zeroshade just want to make sure I'm meant to add the ```c_struct_->offset``` here?
I think the last offset value will be data() + buffer offset (c_struct_->offset()) + item offset but thats different from the ```SliceBuffer``` call that was previously here
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2096872504
Yes. You would be giving the length and CPU memory pointer to the `MemoryManager` and it would decide the best way to copy (always copy) the data.
It is a virtual function so it can have the code that @zeroshade wrote to copy those 4 or 8 bytes as the default implementation, but the CPU memory manager can have a much simpler one using `memcpy`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2115190785
I don't really understand what the purpose of this PR is. It claims that slicing a Buffer is overkill, but I'm a bit skeptical: are there really situations where the overhead would be unacceptable?
Especially, the `CopyBufferSlice` implementation is always first calling `ViewBufferTo`, then falls back to `CopyBufferTo` (both of which instantiate a `std::shared_ptr<Buffer>`), then does a `memcpy` always. If the data length is small, this is still considerable overhead. If the data length is large, then we're doing a spurious copy compared to `ViewOrCopy`...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41477:
URL: https://github.com/apache/arrow/pull/41477#discussion_r1596160781
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -1876,16 +1876,14 @@ struct ArrayImporter {
return ImportBuffer(buffer_id, buffer_size);
}
- // we only need the value of the last offset so let's just copy that
- // one value from device to host.
- auto single_value_buf =
- SliceBuffer(data_->buffers[offsets_buffer_id],
- c_struct_->length * sizeof(OffsetType), sizeof(OffsetType));
- ARROW_ASSIGN_OR_RAISE(
- auto cpubuf, Buffer::ViewOrCopy(single_value_buf, default_cpu_memory_manager()));
- auto offsets = cpubuf->data_as<OffsetType>();
+ OffsetType last_offset = 0;
+ RETURN_NOT_OK(
+ data_->buffers[offsets_buffer_id]->memory_manager()->CopyBufferSliceFrom(
+ data_->buffers[offsets_buffer_id], &last_offset,
+ c_struct_->length * sizeof(OffsetType), sizeof(OffsetType)));
Review Comment:
You shouldn't need the `if (device_type_ == DeviceAllocationType::kCPU) {` case in the beginning of this function if `CopyBufferSliceFrom` is as efficient as a `memcpy`.
##########
cpp/src/arrow/device.cc:
##########
@@ -100,6 +100,20 @@ Result<std::unique_ptr<Buffer>> MemoryManager::CopyNonOwned(
" to ", to->device()->ToString(), " not supported");
}
+Status MemoryManager::CopyBufferSliceFrom(const std::shared_ptr<Buffer>& buf, void* to,
+ const int64_t offset, const int64_t length) {
+ auto maybe_buffer = CopyBuffer(buf, default_cpu_memory_manager());
+
+ if (COPY_BUFFER_SUCCESS(maybe_buffer)) {
+ memcpy(to, (*maybe_buffer)->data() + offset, length);
+ return Status::OK();
+ }
+
+ return Status::NotImplemented("Copying buffer slice from ",
+ buf->memory_manager()->device()->ToString(),
+ " to memory not supported");
+}
+
Review Comment:
This class is complicated, so I had to try implementing this to understand the code.
One of the things I've learned: the `-From` suffix means your function takes `from` `MemoryManager` parameter that matches `buf->memory_manager()`. That doesn't apply here.
I also realized that the `static` function is enough, no need to have the `virtual` version — the fact that we are always copying to CPU memory means we don't have to virtual-dispatch another call and we only have to virtual-dispatch to `buf->memory_manager()`.
So here is what I drafted (I didn't test it):
```cpp
Status MemoryManager::CopyBufferSlice(const std::shared_ptr<Buffer>& buf, int64_t offset,
int64_t length, uint8_t* out_data) {
if (ARROW_PREDICT_TRUE(buf->is_cpu())) {
memcpy(out_data, buf->data() + offset, static_cast<size_t>(length));
return Status::OK();
}
auto& from = buf->memory_manager();
auto cpu_mm = default_cpu_memory_manager();
// Try a view first
auto maybe_buffer_result = from->ViewBufferTo(buf, cpu_mm);
if (!COPY_BUFFER_SUCCESS(maybe_buffer_result)) {
// View failed, try a copy instead
maybe_buffer_result = from->CopyBufferTo(buf, cpu_mm);
}
ARROW_ASSIGN_OR_RAISE(auto maybe_buffer, std::move(maybe_buffer_result));
if (maybe_buffer != nullptr) {
memcpy(out_data, maybe_buffer->data() + offset, static_cast<size_t>(length));
return Status::OK();
}
return Status::NotImplemented("Copying buffer slice from ", from->device()->ToString(),
" to CPU not supported");
}
```
Please define and declare it after `ViewBuffer` to not split that related group of functions.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #41477:
URL: https://github.com/apache/arrow/pull/41477#discussion_r1608360497
##########
cpp/src/arrow/device.h:
##########
@@ -249,6 +249,10 @@ class ARROW_EXPORT MemoryManager : public std::enable_shared_from_this<MemoryMan
static Result<std::shared_ptr<Buffer>> ViewBuffer(
const std::shared_ptr<Buffer>& source, const std::shared_ptr<MemoryManager>& to);
+ /// \brief Copy a slice of a buffer into a CPU pointer
+ static Status CopyBufferSlice(const std::shared_ptr<Buffer>& buf, int64_t offset,
Review Comment:
Can we name this `CopyBufferSliceToCPU` or similar? To make it clear that, unlike other methods here, this isn't taking a target memory manager.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to a CPU pointer [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2128255957
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit ecd769c3e9dade8dcb381fba7c2ac059d46a3a17.
There were 15 benchmark results indicating a performance regression:
- Commit Run on `ec2-m5-4xlarge-us-east-2` at [2024-05-23 18:29:38Z](https://conbench.ursa.dev/compare/runs/b64f2b4b247a4c73af9e1bfdaa890c62...ad2ff02ae45740edb735481b0ec9d3dc/)
- [`dataset-serialize` (Python) with dataset=nyctaxi_multi_ipc_s3, format=feather, selectivity=1pc](https://conbench.ursa.dev/compare/benchmarks/0664f6f48c137e8980008f07eccde191...0664f926d4fe7cd58000cee8b57a279b)
- [`file-write` (Python) with compression=uncompressed, dataset=fanniemae_2016Q4, file_type=feather, input_type=table](https://conbench.ursa.dev/compare/benchmarks/0664f74a73ad7a578000b1ee28d8f01f...0664f97d3acc74a580002e09028f9648)
- and 13 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/25356826032) has more details. It also includes information about 202 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2116480668
The goal is to have a definitive function that can copy bytes from any device buffer to CPU by delegating to `MemoryManager` who should know the best way to do it. Having to write that code that @zeroshade wrote in `bridge.cc` to copy 4 or 8 bytes from a `Buffer` was very annoying. Without getting into the detail of what is fast and what is slow, I think there should be a helper for doing this:
```cpp
if (device_type_ == DeviceAllocationType::kCPU) {
auto offsets = data_->GetValues<OffsetType>(offsets_buffer_id);
// Compute visible size of buffer
int64_t buffer_size =
(c_struct_->length > 0) ? byte_width * offsets[c_struct_->length] : 0;
return ImportBuffer(buffer_id, buffer_size);
}
// we only need the value of the last offset so let's just copy that
// one value from device to host.
auto single_value_buf =
SliceBuffer(data_->buffers[offsets_buffer_id],
c_struct_->length * sizeof(OffsetType), sizeof(OffsetType));
ARROW_ASSIGN_OR_RAISE(
auto cpubuf, Buffer::ViewOrCopy(single_value_buf, default_cpu_memory_manager()));
auto offsets = cpubuf->data_as<OffsetType>();
// Compute visible size of buffer
int64_t buffer_size = (c_struct_->length > 0) ? byte_width * offsets[0] : 0;
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to a CPU pointer [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2127325429
Revision: 8293e79ca6410720672e74ba0da81333e880f358
Submitted crossbow builds: [ursacomputing/crossbow @ actions-aae53c830d](https://github.com/ursacomputing/crossbow/branches/all?query=actions-aae53c830d)
|Task|Status|
|----|------|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/9210119938/job/25336346216)|
|test-cuda-python|[](https://github.com/ursacomputing/crossbow/actions/runs/9210119834/job/25336345973)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "alanstoate (via GitHub)" <gi...@apache.org>.
alanstoate commented on code in PR #41477:
URL: https://github.com/apache/arrow/pull/41477#discussion_r1602286157
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -1868,24 +1868,15 @@ struct ArrayImporter {
template <typename OffsetType>
Status ImportStringValuesBuffer(int32_t offsets_buffer_id, int32_t buffer_id,
int64_t byte_width = 1) {
- if (device_type_ == DeviceAllocationType::kCPU) {
- auto offsets = data_->GetValues<OffsetType>(offsets_buffer_id);
- // Compute visible size of buffer
- int64_t buffer_size =
- (c_struct_->length > 0) ? byte_width * offsets[c_struct_->length] : 0;
- return ImportBuffer(buffer_id, buffer_size);
- }
-
- // we only need the value of the last offset so let's just copy that
- // one value from device to host.
- auto single_value_buf =
- SliceBuffer(data_->buffers[offsets_buffer_id],
- c_struct_->length * sizeof(OffsetType), sizeof(OffsetType));
- ARROW_ASSIGN_OR_RAISE(
- auto cpubuf, Buffer::ViewOrCopy(single_value_buf, default_cpu_memory_manager()));
- auto offsets = cpubuf->data_as<OffsetType>();
+ int64_t last_offset_value_offset =
+ c_struct_->length * sizeof(OffsetType) + c_struct_->offset;
Review Comment:
Ah yep that makes sense
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "alanstoate (via GitHub)" <gi...@apache.org>.
alanstoate commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2102512067
@felipecrv I've had another go with just the ```CopyBufferSliceFrom``` function
Not really sure about the interface especially copying into a ```void*```, but I'm not really sure how else to copy an 1 or more ```OffsetType``` (or another buffer item type)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "alanstoate (via GitHub)" <gi...@apache.org>.
alanstoate commented on code in PR #41477:
URL: https://github.com/apache/arrow/pull/41477#discussion_r1604700787
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -1868,24 +1868,18 @@ struct ArrayImporter {
template <typename OffsetType>
Status ImportStringValuesBuffer(int32_t offsets_buffer_id, int32_t buffer_id,
int64_t byte_width = 1) {
- if (device_type_ == DeviceAllocationType::kCPU) {
- auto offsets = data_->GetValues<OffsetType>(offsets_buffer_id);
+ int64_t last_offset_value_offset =
+ (c_struct_->length + c_struct_->offset) * sizeof(OffsetType);
+ OffsetType last_offset_value;
+ RETURN_NOT_OK(MemoryManager::CopyBufferSlice(
+ data_->buffers[offsets_buffer_id], last_offset_value_offset, sizeof(OffsetType),
+ reinterpret_cast<uint8_t*>(&last_offset_value)));
+
+ int64_t buffer_size = 0;
+ if (c_struct_->length > 0) {
Review Comment:
🤦 whoops, thanks!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2118307761
@alanstoate don't forget to update the PR description to reflect the final state of the code.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to a CPU pointer [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #41477:
URL: https://github.com/apache/arrow/pull/41477
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2090808588
When I wrote
> Perhaps copying a slice of an address() should be a MemoryManager operation.
I was wishing for an API that *didn't always* have to allocate `unique_ptr` or `shared_ptrs` of anything to copy 4 or 8 bytes from a CPU/non-CPU Buffer to a regular CPU memory pointer. It would lower to a `memcpy` if both source and destination are CPU memory managers.
A combinatorial explosion of functions to support on every `MemoryManager` is not desirable.
```
CopyBufferSliceFrom
CopyBufferSliceTo
CopyNonOwnedSliceFrom
CopyNonOwnedSliceTo
ViewBufferSliceFrom
ViewBufferSliceTo
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to a CPU pointer [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #41477:
URL: https://github.com/apache/arrow/pull/41477#issuecomment-2127318270
@github-actions crossbow submit test-cuda*
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39858: [C++][Device] Add Copy/View slice functions to MemoryManager [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41477:
URL: https://github.com/apache/arrow/pull/41477#discussion_r1600264211
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -1868,24 +1868,15 @@ struct ArrayImporter {
template <typename OffsetType>
Status ImportStringValuesBuffer(int32_t offsets_buffer_id, int32_t buffer_id,
int64_t byte_width = 1) {
- if (device_type_ == DeviceAllocationType::kCPU) {
- auto offsets = data_->GetValues<OffsetType>(offsets_buffer_id);
- // Compute visible size of buffer
- int64_t buffer_size =
- (c_struct_->length > 0) ? byte_width * offsets[c_struct_->length] : 0;
- return ImportBuffer(buffer_id, buffer_size);
- }
-
- // we only need the value of the last offset so let's just copy that
- // one value from device to host.
- auto single_value_buf =
- SliceBuffer(data_->buffers[offsets_buffer_id],
- c_struct_->length * sizeof(OffsetType), sizeof(OffsetType));
- ARROW_ASSIGN_OR_RAISE(
- auto cpubuf, Buffer::ViewOrCopy(single_value_buf, default_cpu_memory_manager()));
- auto offsets = cpubuf->data_as<OffsetType>();
+ int64_t last_offset_value_offset =
+ c_struct_->length * sizeof(OffsetType) + c_struct_->offset;
+ OffsetType last_offset_value;
+ RETURN_NOT_OK(MemoryManager::CopyBufferSlice(
+ data_->buffers[offsets_buffer_id], last_offset_value_offset, sizeof(OffsetType),
+ reinterpret_cast<uint8_t*>(&last_offset_value)));
+
// Compute visible size of buffer
- int64_t buffer_size = (c_struct_->length > 0) ? byte_width * offsets[0] : 0;
+ int64_t buffer_size = (c_struct_->length > 0) ? byte_width * last_offset_value : 0;
Review Comment:
You can guard all the slicing in a block after the length check to avoid potentially slicing an empty buffer or slicing unnecessarily because the length of the array is `0`.
```cpp
int64_t buffer_size = 0;
if (c_struct_->length > 0) {
// ...
}
return ImportBuffer(buffer_id, buffer_size);
```
##########
cpp/src/arrow/device.cc:
##########
@@ -116,6 +116,34 @@ Result<std::shared_ptr<Buffer>> MemoryManager::ViewBuffer(
" on ", to->device()->ToString(), " not supported");
}
+Status MemoryManager::CopyBufferSlice(const std::shared_ptr<Buffer>& buf, int64_t offset,
+ int64_t length, uint8_t* out_data) {
+ if (ARROW_PREDICT_TRUE(buf->is_cpu())) {
+ int64_t test;
+ memcpy(&test, buf->data() + offset, static_cast<size_t>(length));
+
Review Comment:
✂️
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -1868,24 +1868,15 @@ struct ArrayImporter {
template <typename OffsetType>
Status ImportStringValuesBuffer(int32_t offsets_buffer_id, int32_t buffer_id,
int64_t byte_width = 1) {
- if (device_type_ == DeviceAllocationType::kCPU) {
- auto offsets = data_->GetValues<OffsetType>(offsets_buffer_id);
- // Compute visible size of buffer
- int64_t buffer_size =
- (c_struct_->length > 0) ? byte_width * offsets[c_struct_->length] : 0;
- return ImportBuffer(buffer_id, buffer_size);
- }
-
- // we only need the value of the last offset so let's just copy that
- // one value from device to host.
- auto single_value_buf =
- SliceBuffer(data_->buffers[offsets_buffer_id],
- c_struct_->length * sizeof(OffsetType), sizeof(OffsetType));
- ARROW_ASSIGN_OR_RAISE(
- auto cpubuf, Buffer::ViewOrCopy(single_value_buf, default_cpu_memory_manager()));
- auto offsets = cpubuf->data_as<OffsetType>();
+ int64_t last_offset_value_offset =
+ c_struct_->length * sizeof(OffsetType) + c_struct_->offset;
Review Comment:
Yes. The offset of the ArrowArray should be applied when reading the `Buffer` of the array.
But the formula is different because the array offset is in number of values, not in the number of bytes.
`(c_struct_->offset + c_struct_->length) * sizeof(OffsetType)`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org