alamb commented on a change in pull request #9495: URL: https://github.com/apache/arrow/pull/9495#discussion_r576397354
########## File path: rust/arrow/src/alloc/alignment.rs ########## @@ -0,0 +1,119 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// NOTE: Below code is written for spatial/temporal prefetcher optimizations. Memory allocation Review comment: FWIW I know you didn't introduce this comment in this PR (it just moved) but I find this comment more confusing than enlightening. ########## File path: rust/arrow/src/alloc/mod.rs ########## @@ -0,0 +1,136 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Defines memory-related functions, such as allocate/deallocate/reallocate memory +//! regions, cache and allocation alignments. + +use std::mem::size_of; +use std::ptr::NonNull; +use std::{ + alloc::{handle_alloc_error, Layout}, + sync::atomic::AtomicIsize, +}; + +mod alignment; +mod types; + +pub use alignment::ALIGNMENT; +pub use types::NativeType; + +// If this number is not zero after all objects have been `drop`, there is a memory leak +pub static mut ALLOCATIONS: AtomicIsize = AtomicIsize::new(0); + +#[inline] +unsafe fn null_pointer<T: NativeType>() -> NonNull<T> { + NonNull::new_unchecked(ALIGNMENT as *mut T) +} + +/// Allocates a cache-aligned memory region of `size` bytes with uninitialized values. +/// This is more performant than using [allocate_aligned_zeroed] when all bytes will have +/// an unknown or non-zero value and is semantically similar to `malloc`. +pub fn allocate_aligned<T: NativeType>(size: usize) -> NonNull<T> { + unsafe { + if size == 0 { + null_pointer() + } else { + let size = size * size_of::<T>(); + ALLOCATIONS.fetch_add(size as isize, std::sync::atomic::Ordering::SeqCst); + + let layout = Layout::from_size_align_unchecked(size, ALIGNMENT); + let raw_ptr = std::alloc::alloc(layout) as *mut T; + NonNull::new(raw_ptr).unwrap_or_else(|| handle_alloc_error(layout)) + } + } +} + +/// Allocates a cache-aligned memory region of `size` bytes with `0` on all of them. +/// This is more performant than using [allocate_aligned] and setting all bytes to zero +/// and is semantically similar to `calloc`. +pub fn allocate_aligned_zeroed<T: NativeType>(size: usize) -> NonNull<T> { + unsafe { + if size == 0 { + null_pointer() + } else { + let size = size * size_of::<T>(); + ALLOCATIONS.fetch_add(size as isize, std::sync::atomic::Ordering::SeqCst); + + let layout = Layout::from_size_align_unchecked(size, ALIGNMENT); + let raw_ptr = std::alloc::alloc_zeroed(layout) as *mut T; + NonNull::new(raw_ptr).unwrap_or_else(|| handle_alloc_error(layout)) + } + } +} + +/// # Safety +/// +/// This function is unsafe because undefined behavior can result if the caller does not ensure all +/// of the following: +/// +/// * ptr must denote a block of memory currently allocated via this allocator, +/// +/// * size must be the same size that was used to allocate that block of memory, +pub unsafe fn free_aligned<T: NativeType>(ptr: NonNull<T>, size: usize) { + if ptr != null_pointer() { + let size = size * size_of::<T>(); + ALLOCATIONS.fetch_sub(size as isize, std::sync::atomic::Ordering::SeqCst); + std::alloc::dealloc( + ptr.as_ptr() as *mut u8, + Layout::from_size_align_unchecked(size, ALIGNMENT), + ); + } +} + +/// # Safety +/// +/// This function is unsafe because undefined behavior can result if the caller does not ensure all +/// of the following: +/// +/// * ptr must be currently allocated via this allocator, Review comment: ```suggestion /// ptr must denote a block of memory previously returned from `allocate_aligned` or /// `allocate_aligned_zeroed` ``` ########## File path: rust/arrow/src/alloc/types.rs ########## @@ -0,0 +1,166 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::datatypes::DataType; + +/// A type that Rust's custom allocator knows how to allocate and deallocate. +/// This is implemented for all Arrow's physical types whose in-memory representation +/// matches Rust's physical types. Consider this trait sealed. +/// # Safety +/// Do not implement this trait. +pub unsafe trait NativeType: + Sized + Copy + std::fmt::Debug + std::fmt::Display + PartialEq + Default + Sized + 'static +{ + type Bytes: AsRef<[u8]>; + + /// Whether a DataType is a valid type for this physical representation. Review comment: ```suggestion /// Whether an array element of `data_type` can be stored using this `NativeType` as /// its physical representation. For example, `i32` stores `DataType::Int32` as well as /// `DataType::Date32` ``` ########## File path: rust/arrow/src/alloc/mod.rs ########## @@ -0,0 +1,136 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Defines memory-related functions, such as allocate/deallocate/reallocate memory +//! regions, cache and allocation alignments. + +use std::mem::size_of; +use std::ptr::NonNull; +use std::{ + alloc::{handle_alloc_error, Layout}, + sync::atomic::AtomicIsize, +}; + +mod alignment; +mod types; + +pub use alignment::ALIGNMENT; +pub use types::NativeType; + +// If this number is not zero after all objects have been `drop`, there is a memory leak +pub static mut ALLOCATIONS: AtomicIsize = AtomicIsize::new(0); + +#[inline] +unsafe fn null_pointer<T: NativeType>() -> NonNull<T> { + NonNull::new_unchecked(ALIGNMENT as *mut T) +} + +/// Allocates a cache-aligned memory region of `size` bytes with uninitialized values. +/// This is more performant than using [allocate_aligned_zeroed] when all bytes will have +/// an unknown or non-zero value and is semantically similar to `malloc`. +pub fn allocate_aligned<T: NativeType>(size: usize) -> NonNull<T> { + unsafe { + if size == 0 { + null_pointer() + } else { + let size = size * size_of::<T>(); + ALLOCATIONS.fetch_add(size as isize, std::sync::atomic::Ordering::SeqCst); + + let layout = Layout::from_size_align_unchecked(size, ALIGNMENT); + let raw_ptr = std::alloc::alloc(layout) as *mut T; + NonNull::new(raw_ptr).unwrap_or_else(|| handle_alloc_error(layout)) + } + } +} + +/// Allocates a cache-aligned memory region of `size` bytes with `0` on all of them. +/// This is more performant than using [allocate_aligned] and setting all bytes to zero +/// and is semantically similar to `calloc`. +pub fn allocate_aligned_zeroed<T: NativeType>(size: usize) -> NonNull<T> { + unsafe { + if size == 0 { + null_pointer() + } else { + let size = size * size_of::<T>(); + ALLOCATIONS.fetch_add(size as isize, std::sync::atomic::Ordering::SeqCst); + + let layout = Layout::from_size_align_unchecked(size, ALIGNMENT); + let raw_ptr = std::alloc::alloc_zeroed(layout) as *mut T; + NonNull::new(raw_ptr).unwrap_or_else(|| handle_alloc_error(layout)) + } + } +} + +/// # Safety +/// +/// This function is unsafe because undefined behavior can result if the caller does not ensure all +/// of the following: +/// +/// * ptr must denote a block of memory currently allocated via this allocator, +/// +/// * size must be the same size that was used to allocate that block of memory, +pub unsafe fn free_aligned<T: NativeType>(ptr: NonNull<T>, size: usize) { + if ptr != null_pointer() { + let size = size * size_of::<T>(); + ALLOCATIONS.fetch_sub(size as isize, std::sync::atomic::Ordering::SeqCst); + std::alloc::dealloc( + ptr.as_ptr() as *mut u8, + Layout::from_size_align_unchecked(size, ALIGNMENT), + ); + } +} + +/// # Safety +/// +/// This function is unsafe because undefined behavior can result if the caller does not ensure all +/// of the following: +/// +/// * ptr must be currently allocated via this allocator, +/// +/// * new_size must be greater than zero. +/// +/// * new_size, when rounded up to the nearest multiple of [ALIGNMENT], must not overflow (i.e., Review comment: If your allocation size is even anywhere near overflowing a 64-bit `usize` you are likely going to have problems 😆 ########## File path: rust/arrow/src/alloc/types.rs ########## @@ -0,0 +1,166 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::datatypes::DataType; + +/// A type that Rust's custom allocator knows how to allocate and deallocate. +/// This is implemented for all Arrow's physical types whose in-memory representation +/// matches Rust's physical types. Consider this trait sealed. +/// # Safety +/// Do not implement this trait. Review comment: 👍 ########## File path: rust/arrow/src/alloc/mod.rs ########## @@ -0,0 +1,136 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Defines memory-related functions, such as allocate/deallocate/reallocate memory +//! regions, cache and allocation alignments. + +use std::mem::size_of; +use std::ptr::NonNull; +use std::{ + alloc::{handle_alloc_error, Layout}, + sync::atomic::AtomicIsize, +}; + +mod alignment; +mod types; + +pub use alignment::ALIGNMENT; +pub use types::NativeType; + +// If this number is not zero after all objects have been `drop`, there is a memory leak +pub static mut ALLOCATIONS: AtomicIsize = AtomicIsize::new(0); + +#[inline] +unsafe fn null_pointer<T: NativeType>() -> NonNull<T> { + NonNull::new_unchecked(ALIGNMENT as *mut T) +} + +/// Allocates a cache-aligned memory region of `size` bytes with uninitialized values. +/// This is more performant than using [allocate_aligned_zeroed] when all bytes will have +/// an unknown or non-zero value and is semantically similar to `malloc`. +pub fn allocate_aligned<T: NativeType>(size: usize) -> NonNull<T> { + unsafe { + if size == 0 { + null_pointer() + } else { + let size = size * size_of::<T>(); + ALLOCATIONS.fetch_add(size as isize, std::sync::atomic::Ordering::SeqCst); + + let layout = Layout::from_size_align_unchecked(size, ALIGNMENT); + let raw_ptr = std::alloc::alloc(layout) as *mut T; + NonNull::new(raw_ptr).unwrap_or_else(|| handle_alloc_error(layout)) + } + } +} + +/// Allocates a cache-aligned memory region of `size` bytes with `0` on all of them. +/// This is more performant than using [allocate_aligned] and setting all bytes to zero +/// and is semantically similar to `calloc`. +pub fn allocate_aligned_zeroed<T: NativeType>(size: usize) -> NonNull<T> { + unsafe { + if size == 0 { + null_pointer() + } else { + let size = size * size_of::<T>(); + ALLOCATIONS.fetch_add(size as isize, std::sync::atomic::Ordering::SeqCst); + + let layout = Layout::from_size_align_unchecked(size, ALIGNMENT); + let raw_ptr = std::alloc::alloc_zeroed(layout) as *mut T; + NonNull::new(raw_ptr).unwrap_or_else(|| handle_alloc_error(layout)) + } + } +} + +/// # Safety +/// +/// This function is unsafe because undefined behavior can result if the caller does not ensure all +/// of the following: +/// +/// * ptr must denote a block of memory currently allocated via this allocator, Review comment: ```suggestion /// * ptr must denote a block of memory previously returned from `allocate_aligned` or /// `allocate_aligned_zeroed` ``` ---------------------------------------------------------------- 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: [email protected]
