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 2020/06/18 16:28:30 UTC
[GitHub] [arrow] maxburke opened a new pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
maxburke opened a new pull request #7480:
URL: https://github.com/apache/arrow/pull/7480
If realloc is passed a block of size zero, free the block and return the
address of the bypass block.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7480:
URL: https://github.com/apache/arrow/pull/7480#discussion_r442475621
##########
File path: rust/arrow/src/memory.rs
##########
@@ -150,25 +150,32 @@ pub fn allocate_aligned(size: usize) -> *mut u8 {
}
pub unsafe fn free_aligned(ptr: *mut u8, size: usize) {
- if size != 0x00 && ptr != BYPASS_PTR.as_ptr() {
+ if ptr != BYPASS_PTR.as_ptr() {
std::alloc::dealloc(ptr, Layout::from_size_align_unchecked(size, ALIGNMENT));
}
}
pub unsafe fn reallocate(ptr: *mut u8, old_size: usize, new_size: usize) -> *mut u8 {
if ptr == BYPASS_PTR.as_ptr() {
- allocate_aligned(new_size)
- } else {
- let new_ptr = std::alloc::realloc(
- ptr,
- Layout::from_size_align_unchecked(old_size, ALIGNMENT),
- new_size,
- );
- if !new_ptr.is_null() && new_size > old_size {
- new_ptr.add(old_size).write_bytes(0, new_size - old_size);
- }
- new_ptr
+ return allocate_aligned(new_size);
+ }
+
+ if new_size == 0 {
+ free_aligned(ptr, old_size);
+ return BYPASS_PTR.as_ptr();
Review comment:
good catch, would be good to add a test for this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] maxburke commented on a change in pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
Posted by GitBox <gi...@apache.org>.
maxburke commented on a change in pull request #7480:
URL: https://github.com/apache/arrow/pull/7480#discussion_r443139838
##########
File path: rust/arrow/src/memory.rs
##########
@@ -150,25 +150,32 @@ pub fn allocate_aligned(size: usize) -> *mut u8 {
}
pub unsafe fn free_aligned(ptr: *mut u8, size: usize) {
- if size != 0x00 && ptr != BYPASS_PTR.as_ptr() {
+ if ptr != BYPASS_PTR.as_ptr() {
std::alloc::dealloc(ptr, Layout::from_size_align_unchecked(size, ALIGNMENT));
}
}
pub unsafe fn reallocate(ptr: *mut u8, old_size: usize, new_size: usize) -> *mut u8 {
if ptr == BYPASS_PTR.as_ptr() {
- allocate_aligned(new_size)
- } else {
- let new_ptr = std::alloc::realloc(
- ptr,
- Layout::from_size_align_unchecked(old_size, ALIGNMENT),
- new_size,
- );
- if !new_ptr.is_null() && new_size > old_size {
- new_ptr.add(old_size).write_bytes(0, new_size - old_size);
- }
- new_ptr
+ return allocate_aligned(new_size);
+ }
+
+ if new_size == 0 {
+ free_aligned(ptr, old_size);
+ return BYPASS_PTR.as_ptr();
Review comment:
What kind of test are you looking for? Running valgrind on the tests to validate there are no leaked blocks?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7480:
URL: https://github.com/apache/arrow/pull/7480#discussion_r443140804
##########
File path: rust/arrow/src/memory.rs
##########
@@ -150,25 +150,32 @@ pub fn allocate_aligned(size: usize) -> *mut u8 {
}
pub unsafe fn free_aligned(ptr: *mut u8, size: usize) {
- if size != 0x00 && ptr != BYPASS_PTR.as_ptr() {
+ if ptr != BYPASS_PTR.as_ptr() {
std::alloc::dealloc(ptr, Layout::from_size_align_unchecked(size, ALIGNMENT));
}
}
pub unsafe fn reallocate(ptr: *mut u8, old_size: usize, new_size: usize) -> *mut u8 {
if ptr == BYPASS_PTR.as_ptr() {
- allocate_aligned(new_size)
- } else {
- let new_ptr = std::alloc::realloc(
- ptr,
- Layout::from_size_align_unchecked(old_size, ALIGNMENT),
- new_size,
- );
- if !new_ptr.is_null() && new_size > old_size {
- new_ptr.add(old_size).write_bytes(0, new_size - old_size);
- }
- new_ptr
+ return allocate_aligned(new_size);
+ }
+
+ if new_size == 0 {
+ free_aligned(ptr, old_size);
+ return BYPASS_PTR.as_ptr();
Review comment:
At first i thought there is straightforward api from the global allocator we can use to access memory allocation metadata. This turned out not to be the case, which means we will either have to introduce valgrind or change rust global allocator to a custom one for tests. Both approaches will expand the scope of this PR too much, so scratch what I said, I think it's better to be done as a separate PRs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paddyhoran commented on a change in pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
Posted by GitBox <gi...@apache.org>.
paddyhoran commented on a change in pull request #7480:
URL: https://github.com/apache/arrow/pull/7480#discussion_r443242626
##########
File path: rust/arrow/src/memory.rs
##########
@@ -150,25 +150,32 @@ pub fn allocate_aligned(size: usize) -> *mut u8 {
}
pub unsafe fn free_aligned(ptr: *mut u8, size: usize) {
- if size != 0x00 && ptr != BYPASS_PTR.as_ptr() {
+ if ptr != BYPASS_PTR.as_ptr() {
std::alloc::dealloc(ptr, Layout::from_size_align_unchecked(size, ALIGNMENT));
}
}
pub unsafe fn reallocate(ptr: *mut u8, old_size: usize, new_size: usize) -> *mut u8 {
if ptr == BYPASS_PTR.as_ptr() {
- allocate_aligned(new_size)
- } else {
- let new_ptr = std::alloc::realloc(
- ptr,
- Layout::from_size_align_unchecked(old_size, ALIGNMENT),
- new_size,
- );
- if !new_ptr.is_null() && new_size > old_size {
- new_ptr.add(old_size).write_bytes(0, new_size - old_size);
- }
- new_ptr
+ return allocate_aligned(new_size);
+ }
+
+ if new_size == 0 {
+ free_aligned(ptr, old_size);
+ return BYPASS_PTR.as_ptr();
Review comment:
I guess we could start running valgrind in CI for Rust? I don't use Valgrind but I created an [issue](https://issues.apache.org/jira/browse/ARROW-9200) if anyone wants to pick it up.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] maxburke commented on pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
Posted by GitBox <gi...@apache.org>.
maxburke commented on pull request #7480:
URL: https://github.com/apache/arrow/pull/7480#issuecomment-646432538
Seeing some weird issues in long running applications; there are no leaks as reported by valgrind anymore, but memory use is up. I'm going to close this PR for now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] maxburke closed pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
Posted by GitBox <gi...@apache.org>.
maxburke closed pull request #7480:
URL: https://github.com/apache/arrow/pull/7480
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7480:
URL: https://github.com/apache/arrow/pull/7480#issuecomment-646148492
https://issues.apache.org/jira/browse/ARROW-9176
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7480:
URL: https://github.com/apache/arrow/pull/7480#discussion_r443140804
##########
File path: rust/arrow/src/memory.rs
##########
@@ -150,25 +150,32 @@ pub fn allocate_aligned(size: usize) -> *mut u8 {
}
pub unsafe fn free_aligned(ptr: *mut u8, size: usize) {
- if size != 0x00 && ptr != BYPASS_PTR.as_ptr() {
+ if ptr != BYPASS_PTR.as_ptr() {
std::alloc::dealloc(ptr, Layout::from_size_align_unchecked(size, ALIGNMENT));
}
}
pub unsafe fn reallocate(ptr: *mut u8, old_size: usize, new_size: usize) -> *mut u8 {
if ptr == BYPASS_PTR.as_ptr() {
- allocate_aligned(new_size)
- } else {
- let new_ptr = std::alloc::realloc(
- ptr,
- Layout::from_size_align_unchecked(old_size, ALIGNMENT),
- new_size,
- );
- if !new_ptr.is_null() && new_size > old_size {
- new_ptr.add(old_size).write_bytes(0, new_size - old_size);
- }
- new_ptr
+ return allocate_aligned(new_size);
+ }
+
+ if new_size == 0 {
+ free_aligned(ptr, old_size);
+ return BYPASS_PTR.as_ptr();
Review comment:
At first i thought there are straightforward apis from the global allocator we can use to access memory allocation metadata. This turned out not to be the case, which means we will either have to introduce valgrind or change rust global allocator to a custom one for tests. Both approaches will expand the scope of this PR too much, so scratch what I said, I think it's better to be done as a separate PRs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paddyhoran closed pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
Posted by GitBox <gi...@apache.org>.
paddyhoran closed pull request #7480:
URL: https://github.com/apache/arrow/pull/7480
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] maxburke commented on pull request #7480: ARROW-9176: [Rust] Fix for memory leaks in Arrow allocator
Posted by GitBox <gi...@apache.org>.
maxburke commented on pull request #7480:
URL: https://github.com/apache/arrow/pull/7480#issuecomment-646766294
Re-opening this PR because the memory use changes are related to our use of the underlying system allocator, not this change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org