[GitHub] [arrow] vertexclique commented on a change in pull request #7061: ARROW-8629: [Rust] Eliminate indirection of zero sized allocations
vertexclique commented on a change in pull request #7061:
URL: https://github.com/apache/arrow/pull/7061#discussion_r419000857
##
File path: rust/arrow/src/memory.rs
##
@@ -20,34 +20,56 @@
use std::alloc::Layout;
use std::mem::align_of;
+use std::ptr::NonNull;
pub const ALIGNMENT: usize = 64;
+///
+/// As you can see this is global and lives as long as the program lives.
+/// Be careful to not write anything to this pointer in any scenario.
+/// If you use allocation methods shown here you won't have any problems.
+const BYPASS_PTR: NonNull = unsafe { NonNull::new_unchecked(0xDEADBEEF as
*mut u8) };
+
pub fn allocate_aligned(size: usize) -> *mut u8 {
unsafe {
-let layout = Layout::from_size_align_unchecked(size, ALIGNMENT);
-std::alloc::alloc_zeroed(layout)
+if size == 0 {
+// In a perfect world, there is no need to request zero size
allocation.
+// Currently, passing zero sized layout to alloc is UB.
+// This will dodge allocator api for any type.
+BYPASS_PTR.as_mut()
+} else {
+let layout = Layout::from_size_align_unchecked(size, ALIGNMENT);
+std::alloc::alloc_zeroed(layout)
+}
}
}
-pub unsafe fn free_aligned(p: *mut u8, size: usize) {
-std::alloc::dealloc(p, Layout::from_size_align_unchecked(size, ALIGNMENT));
+pub unsafe fn free_aligned(ptr: *mut u8, size: usize) {
+if size != 0x00 && ptr != BYPASS_PTR.as_mut() {
+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 {
-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);
+if old_size == 0x00 && ptr == BYPASS_PTR.as_mut() {
Review comment:
Addressed. True, that can be a 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [arrow] vertexclique commented on a change in pull request #7061: ARROW-8629: [Rust] Eliminate indirection of zero sized allocations
vertexclique commented on a change in pull request #7061:
URL: https://github.com/apache/arrow/pull/7061#discussion_r419000857
##
File path: rust/arrow/src/memory.rs
##
@@ -20,34 +20,56 @@
use std::alloc::Layout;
use std::mem::align_of;
+use std::ptr::NonNull;
pub const ALIGNMENT: usize = 64;
+///
+/// As you can see this is global and lives as long as the program lives.
+/// Be careful to not write anything to this pointer in any scenario.
+/// If you use allocation methods shown here you won't have any problems.
+const BYPASS_PTR: NonNull = unsafe { NonNull::new_unchecked(0xDEADBEEF as
*mut u8) };
+
pub fn allocate_aligned(size: usize) -> *mut u8 {
unsafe {
-let layout = Layout::from_size_align_unchecked(size, ALIGNMENT);
-std::alloc::alloc_zeroed(layout)
+if size == 0 {
+// In a perfect world, there is no need to request zero size
allocation.
+// Currently, passing zero sized layout to alloc is UB.
+// This will dodge allocator api for any type.
+BYPASS_PTR.as_mut()
+} else {
+let layout = Layout::from_size_align_unchecked(size, ALIGNMENT);
+std::alloc::alloc_zeroed(layout)
+}
}
}
-pub unsafe fn free_aligned(p: *mut u8, size: usize) {
-std::alloc::dealloc(p, Layout::from_size_align_unchecked(size, ALIGNMENT));
+pub unsafe fn free_aligned(ptr: *mut u8, size: usize) {
+if size != 0x00 && ptr != BYPASS_PTR.as_mut() {
+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 {
-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);
+if old_size == 0x00 && ptr == BYPASS_PTR.as_mut() {
Review comment:
Addressed.
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:
us...@infra.apache.org
[GitHub] [arrow] vertexclique commented on a change in pull request #7061: ARROW-8629: [Rust] Eliminate indirection of zero sized allocations
vertexclique commented on a change in pull request #7061:
URL: https://github.com/apache/arrow/pull/7061#discussion_r418999354
##
File path: rust/arrow/src/util/bit_util.rs
##
@@ -148,11 +148,17 @@ pub fn count_set_bits_offset(data: &[u8], offset: usize,
length: usize) -> usize
/// Returns the ceil of `value`/`divisor`
#[inline]
pub fn ceil(value: usize, divisor: usize) -> usize {
-let mut result = value / divisor;
-if value % divisor != 0 {
-result += 1
-};
-result
+if value == 0_usize {
Review comment:
Oh it is, meanwhile looking for zero sized allocations I came across
with this, from this chunk of code:
```
impl BufferBuilderTrait for BufferBuilder {
fn new(capacity: usize) -> Self {
let byte_capacity = bit_util::ceil(capacity, 8);
let actual_capacity =
bit_util::round_upto_multiple_of_64(byte_capacity);
let mut buffer = MutableBuffer::new(actual_capacity);
buffer.set_null_bits(0, actual_capacity);
Self {
buffer,
len: 0,
_marker: PhantomData,
}
}
```
BufferBuilderTrait is using this code for every reallocation. Ceil is not
euclidean C- division. According to this paper:
https://www.microsoft.com/en-us/research/wp-content/uploads/2016/02/divmodnote-letter.pdf
So I thought better to use established C-division in this case, where things
got improved from that side too.
Separate PR should be open to fixing this in Parquet too.
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:
us...@infra.apache.org
[GitHub] [arrow] vertexclique commented on a change in pull request #7061: ARROW-8629: [Rust] Eliminate indirection of zero sized allocations
vertexclique commented on a change in pull request #7061:
URL: https://github.com/apache/arrow/pull/7061#discussion_r417823393
##
File path: rust/arrow/src/memory.rs
##
@@ -20,34 +20,56 @@
use std::alloc::Layout;
use std::mem::align_of;
+use std::ptr::NonNull;
pub const ALIGNMENT: usize = 64;
+///
+/// As you can see this is global and lives as long as the program lives.
+/// Be careful to not write anything to this pointer in any scenario.
+/// If you use allocation methods shown here you won't have any problems.
+const BYPASS_PTR: NonNull = unsafe { NonNull::new_unchecked(0xDEADBEEF as
*mut u8) };
+
pub fn allocate_aligned(size: usize) -> *mut u8 {
unsafe {
Review comment:
It is allowed in the language itself, though my personal opinion that
this should be allowed neither in this codebase nor in general at the language
level. By definition, it still goes to the OS and does the necessary kernel
procedure.
I didn't want to change this code piece at first, wanted to fix it at the
caller site. But I've discovered that the forgetting this at the caller site or
checking it creates yet another responsibility to a newcomer to code. So I've
explicitly put it in the place of memory operations.
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:
us...@infra.apache.org
