Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[C++] Use default_cpu_memory_manager() for the default memory pool in MessageDecoder #39270

Closed
kou opened this issue Dec 18, 2023 · 3 comments · Fixed by #39271
Closed

[C++] Use default_cpu_memory_manager() for the default memory pool in MessageDecoder #39270

kou opened this issue Dec 18, 2023 · 3 comments · Fixed by #39271
Assignees
@kou
Labels
Component: C++ Type: enhancement
Milestone
16.0.0

Comments

@kou
Copy link
Member

kou commented Dec 18, 2023 •
edited
Loading

Describe the enhancement requested

arrow::ipc::MessageDecoder::MessageDecoderImpl always use arrow::CPUDevice::memory_manager() but we can use existing arrow::default_cpu_memory_manager() for arrow::default_memory_manager().

Component(s)

C++
@kou kou changed the title [C++] Use default_cpu_memory_manager() for MessageDecoder by default [C++] Use default_cpu_memory_manager() for MessageDecoder for the default memory pool Dec 18, 2023
@kou kou changed the title [C++] Use default_cpu_memory_manager() for MessageDecoder for the default memory pool [C++] Use default_cpu_memory_manager() for the default memory pool in MessageDecoder Dec 18, 2023
kou added a commit to kou/arrow that referenced this issue Dec 18, 2023
@kou
apacheGH-39270: [C++] Avoid creating needless memory manager instance…
704f556 
… in MessageDecoder

We can use `arrow::default_cpu_memory_manager()` for
`default_memory_pool()`.
@kou kou mentioned this issue Dec 18, 2023
Merged
@github-actions github-actions bot mentioned this issue Dec 18, 2023
Merged
@pitrou
Copy link
Member

pitrou commented Dec 21, 2023

It may cause needless memory copy (Buffer copy)

Did you actually verify this?

@kou
Copy link
Member Author

kou commented Jan 5, 2024

Sorry. I was wrong.

I just saw

arrow/cpp/src/arrow/device.cc

Line 101 in bec0385

if (buf->memory_manager() == to) {
but copy isn't used by

arrow/cpp/src/arrow/device.cc

Line 105 in bec0385

auto maybe_buffer = to->ViewBufferFrom(buf, from);
.

I close this but I propose #39271 change for simplification.

@kou kou closed this as not planned Won't fix, can't repro, duplicate, stale Jan 5, 2024
@kou
Copy link
Member Author

kou commented Jan 5, 2024

Ah, I reuse this. This issue already has a general title.

@kou kou reopened this Jan 5, 2024
kou added a commit to kou/arrow that referenced this issue Jan 9, 2024
@kou
apacheGH-39270: [C++] Avoid creating needless memory manager instance…
ef1a5aa 
… in MessageDecoder

We can use `arrow::default_cpu_memory_manager()` for
`default_memory_pool()`.
kou added a commit that referenced this issue Jan 11, 2024
@kou
GH-39270: [C++] Avoid creating memory manager instance for every buff…
2132cb3 
…er view/copy (#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: #39270

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 16.0.0 milestone Jan 11, 2024
clayburn pushed a commit to clayburn/arrow that referenced this issue Jan 23, 2024
@kou @clayburn
apacheGH-39270: [C++] Avoid creating memory manager instance for ever…
ed73f02 
…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>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@kou @dgreiss
apacheGH-39270: [C++] Avoid creating memory manager instance for ever…
d2c4948 
…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>
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
@kou @zanmato1984
apacheGH-39270: [C++] Avoid creating memory manager instance for ever…
406a3b3 
…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>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
@kou @thisisnic
apacheGH-39270: [C++] Avoid creating memory manager instance for ever…
0407b1b 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kou kou

Labels
Component: C++ Type: enhancement
Projects
None yet
Milestone
16.0.0
Development

Successfully merging a pull request may close this issue.

GH-39270: [C++] Avoid creating memory manager instance for every buffer view/copy kou/arrow
2 participants
@kou @pitrou