GH-39270: [C++] Avoid creating memory manager instance for every buffer view/copy#39271
Conversation
|
|
cpp/src/arrow/ipc/message.cc
Outdated
There was a problem hiding this comment.
Why not add this optimization to CPUDevice::memory_manager directly?
There was a problem hiding this comment.
Oh, I didn't notice it.
I'll do it.
There was a problem hiding this comment.
Done.
I've updated the PR title.
… in MessageDecoder We can use `arrow::default_cpu_memory_manager()` for `default_memory_pool()`.
704f556 to
ec38f61
Compare
|
@kou Your latest push is ondoing the main optimization, which is to avoid calling |
|
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. |
Only if you're using the default memory manager! |
|
Ah, I didn't care about the case! |
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 2132cb3. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…y buffer view/copy (apache#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()`. This also caches `arrow::CPUDevice::memory_manager()` result to avoid calling it multiple times. Note that we can avoid creating needless memory manager instance without this. This just avoid calling it multiple times. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#39270 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
We can use
arrow::default_cpu_memory_manager()fordefault_memory_pool().What changes are included in this PR?
Check the given
pooland usearrow::default_cpu_memory_manager()if it'sarrow::default_memory_pool().This also caches
arrow::CPUDevice::memory_manager()result to avoid calling it multiple times. Note that we can avoid creating needless memory manager instance without this. This just avoid calling it multiple times.Are these changes tested?
Yes.
Are there any user-facing changes?
No.