You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2023/12/18 09:21:15 UTC
[PR] GH-39270: [C++] Avoid creating needless memory manager instance in MessageDecoder [arrow]
kou opened a new pull request, #39271:
URL: https://github.com/apache/arrow/pull/39271
### Rationale for this change
We can use `arrow::default_cpu_memory_manager()` for `default_memory_pool()`.
### What changes are included in this PR?
Check the given `pool` and use `arrow::default_cpu_memory_manager()` if it's `arrow::default_memory_pool()`.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No.
--
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-39270: [C++] Avoid creating memory manager instance for every buffer view/copy [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #39271:
URL: https://github.com/apache/arrow/pull/39271
--
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-39270: [C++] Avoid creating needless memory manager instance in MessageDecoder [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #39271:
URL: https://github.com/apache/arrow/pull/39271#discussion_r1446731997
##########
cpp/src/arrow/ipc/message.cc:
##########
@@ -607,6 +607,9 @@ class MessageDecoder::MessageDecoderImpl {
MemoryPool* pool, bool skip_body)
: listener_(std::move(listener)),
pool_(pool),
+ memory_manager_(pool_ == default_memory_pool()
+ ? default_cpu_memory_manager()
+ : CPUDevice::memory_manager(pool_)),
Review Comment:
Oh, I didn't notice it.
I'll do it.
--
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-39270: [C++] Avoid creating memory manager instance for every buffer view/copy [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39271:
URL: https://github.com/apache/arrow/pull/39271#issuecomment-1888339857
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 2132cb361c386ab25d72b4990772ca6e5c31312c.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/20410446249) has more details. It also includes information about 9 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-39270: [C++] Avoid creating needless memory manager instance in MessageDecoder [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39271:
URL: https://github.com/apache/arrow/pull/39271#issuecomment-1859874649
:warning: GitHub issue #39270 **has been automatically assigned in GitHub** to PR creator.
--
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-39270: [C++] Avoid creating needless memory manager instance in CPUDevice::memory_manager() [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #39271:
URL: https://github.com/apache/arrow/pull/39271#discussion_r1446745288
##########
cpp/src/arrow/ipc/message.cc:
##########
@@ -607,6 +607,9 @@ class MessageDecoder::MessageDecoderImpl {
MemoryPool* pool, bool skip_body)
: listener_(std::move(listener)),
pool_(pool),
+ memory_manager_(pool_ == default_memory_pool()
+ ? default_cpu_memory_manager()
+ : CPUDevice::memory_manager(pool_)),
Review Comment:
Done.
I've updated the PR title.
--
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-39270: [C++] Avoid creating needless memory manager instance in CPUDevice::memory_manager() [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39271:
URL: https://github.com/apache/arrow/pull/39271#issuecomment-1885258982
@kou Your latest push is ondoing the main optimization, which is to avoid calling `CPUDevice::memory_manager` for each buffer view.
--
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-39270: [C++] Avoid creating needless memory manager instance in CPUDevice::memory_manager() [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #39271:
URL: https://github.com/apache/arrow/pull/39271#issuecomment-1885776979
I thought that reusing the default memory manager for CPU is the main optimization. Caching it as an instance variable is just for simplifying codes.
Anyway, I've added the instance variable again.
--
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-39270: [C++] Avoid creating needless memory manager instance in MessageDecoder [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39271:
URL: https://github.com/apache/arrow/pull/39271#discussion_r1446444605
##########
cpp/src/arrow/ipc/message.cc:
##########
@@ -607,6 +607,9 @@ class MessageDecoder::MessageDecoderImpl {
MemoryPool* pool, bool skip_body)
: listener_(std::move(listener)),
pool_(pool),
+ memory_manager_(pool_ == default_memory_pool()
+ ? default_cpu_memory_manager()
+ : CPUDevice::memory_manager(pool_)),
Review Comment:
Why not add this optimization to `CPUDevice::memory_manager` directly?
--
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-39270: [C++] Avoid creating memory manager instance for every buffer view/copy [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #39271:
URL: https://github.com/apache/arrow/pull/39271#issuecomment-1886631821
Ah, I didn't care about the case!
--
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-39270: [C++] Avoid creating memory manager instance for every buffer view/copy [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39271:
URL: https://github.com/apache/arrow/pull/39271#issuecomment-1886624672
> I thought that reusing the default memory manager for CPU is the main optimization
Only if you're using the default 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