You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/02/04 20:41:31 UTC
[GitHub] [arrow] lidavidm opened a new pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
lidavidm opened a new pull request #12341:
URL: https://github.com/apache/arrow/pull/12341
This is useful to implement, for instance, a copy from a void* (from an external API) to a buffer on a different device (the current API would require allocating a shared_ptr, and provides no way to get back a unique_ptr). Note that while we have the BufferWriter API, that also fails (because it needs ownership of the buffer to write to, and there isn't a good way to express "a temporary writer that does not have ownership of the target buffer").
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#issuecomment-1043134044
Benchmark runs are scheduled for baseline = 17fcbdbd8fa9ca186c6f1c1f371acd8f571402d5 and contender = d94365f745ac51937f010fa32efbb2ce13f90116. d94365f745ac51937f010fa32efbb2ce13f90116 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/26e06260a39b498c9161fc4790794de6...c7fdf0476c314895a2ee2c79dd99af78/)
[Finished :arrow_down:0.89% :arrow_up:0.04%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/65cff6789ac2450fb46f11b472783a73...13c1f08072454a12867a2deb95aeb1d2/)
[Failed :arrow_down:0.36% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/a602e51eb23f452f982404514edf84d8...209f58ad5cde4d9ca139c58ba1ee78f7/)
[Finished :arrow_down:0.09% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/09e918dd2de14e108f4359f9ad617a91...621540efd5fc4dbea3a988addcc93cf4/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] pitrou commented on pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#issuecomment-1043122870
CI failures look unrelated, will merge.
--
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
[GitHub] [arrow] lidavidm commented on a change in pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#discussion_r803656701
##########
File path: cpp/src/arrow/device.h
##########
@@ -119,13 +119,16 @@ class ARROW_EXPORT MemoryManager : public std::enable_shared_from_this<MemoryMan
/// The buffer will be allocated in the device's memory.
virtual Result<std::unique_ptr<Buffer>> AllocateBuffer(int64_t size) = 0;
- // XXX Should this take a `const Buffer&` instead
/// \brief Copy a Buffer to a destination MemoryManager
///
/// See also the Buffer::Copy shorthand.
static Result<std::shared_ptr<Buffer>> CopyBuffer(
const std::shared_ptr<Buffer>& source, const std::shared_ptr<MemoryManager>& to);
+ /// \brief Copy a Buffer to a destination MemoryManager
+ static Result<std::unique_ptr<Buffer>> CopyBuffer(
+ const Buffer& source, const std::shared_ptr<MemoryManager>& to);
Review comment:
Hmm, CopyMemory is apparently a [macro defined on Windows](https://docs.microsoft.com/en-us/previous-versions/windows/desktop/legacy/aa366535(v=vs.85)). Renamed to CopyBufferRef.
--
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
[GitHub] [arrow] pitrou closed pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #12341:
URL: https://github.com/apache/arrow/pull/12341
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#issuecomment-1043134044
Benchmark runs are scheduled for baseline = 17fcbdbd8fa9ca186c6f1c1f371acd8f571402d5 and contender = d94365f745ac51937f010fa32efbb2ce13f90116. d94365f745ac51937f010fa32efbb2ce13f90116 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/26e06260a39b498c9161fc4790794de6...c7fdf0476c314895a2ee2c79dd99af78/)
[Finished :arrow_down:0.89% :arrow_up:0.04%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/65cff6789ac2450fb46f11b472783a73...13c1f08072454a12867a2deb95aeb1d2/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/a602e51eb23f452f982404514edf84d8...209f58ad5cde4d9ca139c58ba1ee78f7/)
[Finished :arrow_down:0.09% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/09e918dd2de14e108f4359f9ad617a91...621540efd5fc4dbea3a988addcc93cf4/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#issuecomment-1030334197
https://issues.apache.org/jira/browse/ARROW-15579
--
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
[GitHub] [arrow] pitrou commented on a change in pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#discussion_r801605527
##########
File path: cpp/src/arrow/device.h
##########
@@ -119,13 +119,16 @@ class ARROW_EXPORT MemoryManager : public std::enable_shared_from_this<MemoryMan
/// The buffer will be allocated in the device's memory.
virtual Result<std::unique_ptr<Buffer>> AllocateBuffer(int64_t size) = 0;
- // XXX Should this take a `const Buffer&` instead
/// \brief Copy a Buffer to a destination MemoryManager
///
/// See also the Buffer::Copy shorthand.
static Result<std::shared_ptr<Buffer>> CopyBuffer(
const std::shared_ptr<Buffer>& source, const std::shared_ptr<MemoryManager>& to);
+ /// \brief Copy a Buffer to a destination MemoryManager
+ static Result<std::unique_ptr<Buffer>> CopyBuffer(
+ const Buffer& source, const std::shared_ptr<MemoryManager>& to);
Review comment:
I would recommend choosing a different signature and perhaps a different name to avoid programming errors, e.g.:
```c++
// \brief Copy a non-owned memory area to a destination MemoryManager
static Result<std::unique_ptr<Buffer>> CopyMemory(
uintptr_t source_address, int64_t size, const std::shared_ptr<MemoryManager>& to);
```
--
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
[GitHub] [arrow] ursabot commented on pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#issuecomment-1043134044
Benchmark runs are scheduled for baseline = 17fcbdbd8fa9ca186c6f1c1f371acd8f571402d5 and contender = d94365f745ac51937f010fa32efbb2ce13f90116. d94365f745ac51937f010fa32efbb2ce13f90116 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/26e06260a39b498c9161fc4790794de6...c7fdf0476c314895a2ee2c79dd99af78/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/65cff6789ac2450fb46f11b472783a73...13c1f08072454a12867a2deb95aeb1d2/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/a602e51eb23f452f982404514edf84d8...209f58ad5cde4d9ca139c58ba1ee78f7/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/09e918dd2de14e108f4359f9ad617a91...621540efd5fc4dbea3a988addcc93cf4/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] pitrou commented on pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#issuecomment-1034983401
I renamed to `CopyNonOwned`. It's not terrific, but definitely less unhelpful than `CopyBufferRef`. Feel free to find something better :-)
(as an aside, I would find it less confusing if it didn't take a Buffer at all, but as you point out you then have to pass the source MemoryManager separately)
--
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
[GitHub] [arrow] lidavidm commented on pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#issuecomment-1040712369
CopyNonOwned sounds fine to me as well.
--
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
[GitHub] [arrow] lidavidm commented on a change in pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#discussion_r802800399
##########
File path: cpp/src/arrow/device.h
##########
@@ -119,13 +119,16 @@ class ARROW_EXPORT MemoryManager : public std::enable_shared_from_this<MemoryMan
/// The buffer will be allocated in the device's memory.
virtual Result<std::unique_ptr<Buffer>> AllocateBuffer(int64_t size) = 0;
- // XXX Should this take a `const Buffer&` instead
/// \brief Copy a Buffer to a destination MemoryManager
///
/// See also the Buffer::Copy shorthand.
static Result<std::shared_ptr<Buffer>> CopyBuffer(
const std::shared_ptr<Buffer>& source, const std::shared_ptr<MemoryManager>& to);
+ /// \brief Copy a Buffer to a destination MemoryManager
+ static Result<std::unique_ptr<Buffer>> CopyBuffer(
+ const Buffer& source, const std::shared_ptr<MemoryManager>& to);
Review comment:
Renamed, though I kept the signature since we do want to be able to know the device of the source buffer.
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#issuecomment-1043134044
Benchmark runs are scheduled for baseline = 17fcbdbd8fa9ca186c6f1c1f371acd8f571402d5 and contender = d94365f745ac51937f010fa32efbb2ce13f90116. d94365f745ac51937f010fa32efbb2ce13f90116 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/26e06260a39b498c9161fc4790794de6...c7fdf0476c314895a2ee2c79dd99af78/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/65cff6789ac2450fb46f11b472783a73...13c1f08072454a12867a2deb95aeb1d2/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/a602e51eb23f452f982404514edf84d8...209f58ad5cde4d9ca139c58ba1ee78f7/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/09e918dd2de14e108f4359f9ad617a91...621540efd5fc4dbea3a988addcc93cf4/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #12341: ARROW-15579: [C++] Add MemoryManager::CopyBuffer(const Buffer&)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12341:
URL: https://github.com/apache/arrow/pull/12341#issuecomment-1043134044
Benchmark runs are scheduled for baseline = 17fcbdbd8fa9ca186c6f1c1f371acd8f571402d5 and contender = d94365f745ac51937f010fa32efbb2ce13f90116. d94365f745ac51937f010fa32efbb2ce13f90116 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/26e06260a39b498c9161fc4790794de6...c7fdf0476c314895a2ee2c79dd99af78/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/65cff6789ac2450fb46f11b472783a73...13c1f08072454a12867a2deb95aeb1d2/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/a602e51eb23f452f982404514edf84d8...209f58ad5cde4d9ca139c58ba1ee78f7/)
[Finished :arrow_down:0.09% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/09e918dd2de14e108f4359f9ad617a91...621540efd5fc4dbea3a988addcc93cf4/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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