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.

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

Support shrink to empty #6817

Merged
merged 3 commits into from
Dec 1, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion arrow-buffer/src/buffer/immutable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,7 @@ mod tests {
let mut shrunk_empty = empty_slice;
shrunk_empty.shrink_to_fit();
assert_eq!(shrunk_empty.as_slice(), &[]);
assert_eq!(shrunk_empty.capacity(), 1); // NOTE: `Buffer` and `Bytes` doesn't support 0-capacity
assert_eq!(shrunk_empty.capacity(), 0);
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion arrow-buffer/src/buffer/mutable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ impl MutableBuffer {
}

#[inline]
fn dangling_ptr() -> NonNull<u8> {
pub(crate) fn dangling_ptr() -> NonNull<u8> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding some some comments about this function given its name.

Specifically, that it returns a valid, u8 pointer that is valid for the entire life of the process. Suitable for allocations of 0 length.

I realize it wasn't introduced in this PR, but the name is confusing to me -- Maybe calling it arbtrary_ptr() or something would be less confusing 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah dangling is an odd name, but is based on what the standard library calls it. I have added some docs to this effect

// SAFETY: ALIGNMENT is a non-zero usize which is then casted
// to a *mut T. Therefore, `ptr` is not null and the conditions for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not introduced in this PR, but this function isn't generic, so I am not sure what *mut T is talking about

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a copy-pasta from NonNull::dangling

// calling new_unchecked() are respected.
Expand Down
26 changes: 18 additions & 8 deletions arrow-buffer/src/bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use std::ptr::NonNull;
use std::{fmt::Debug, fmt::Formatter};

use crate::alloc::Deallocation;
use crate::buffer::dangling_ptr;

/// A continuous, fixed-size, immutable memory region that knows how to de-allocate itself.
///
Expand Down Expand Up @@ -104,20 +105,29 @@ impl Bytes {
/// In case of `Err`, the [`Bytes`] will remain as it was (i.e. have the old size).
pub fn try_realloc(&mut self, new_len: usize) -> Result<(), ()> {
if let Deallocation::Standard(old_layout) = self.deallocation {
let new_len = new_len.max(1); // realloc requires a non-zero size
if old_layout.size() == new_len {
return Ok(()); // Nothing to do
}

if let Ok(new_layout) = std::alloc::Layout::from_size_align(new_len, old_layout.align())
{
let old_ptr = self.ptr.as_ptr();
// SAFETY: the call to `realloc` is safe if all of the following holds (from https://doc.rust-lang.org/stable/std/alloc/trait.GlobalAlloc.html#method.realloc):
// * `old_ptr` must be currently allocated via this allocator (guaranteed by the invariant/contract of `Bytes`)
// * `old_layout` must be the same layout that was used to allocate that block of memory (same)
// * `new_len` must be greater than zero (ensured by the `max` call earlier)
// * `new_len`, when rounded up to the nearest multiple of `layout.align()`, must not overflow `isize` (guaranteed by the success of `Layout::from_size_align`)
let new_ptr = unsafe { std::alloc::realloc(old_ptr, old_layout, new_len) };
if let Some(ptr) = NonNull::new(new_ptr) {

let new_ptr = match new_layout.size() {
0 => {
// SAFETY: Verified that old_layout.size != new_len (0)
unsafe { std::alloc::dealloc(self.ptr.as_ptr(), old_layout) };
Some(dangling_ptr())
}
// SAFETY: the call to `realloc` is safe if all the following hold (from https://doc.rust-lang.org/stable/std/alloc/trait.GlobalAlloc.html#method.realloc):
// * `old_ptr` must be currently allocated via this allocator (guaranteed by the invariant/contract of `Bytes`)
// * `old_layout` must be the same layout that was used to allocate that block of memory (same)
// * `new_len` must be greater than zero
// * `new_len`, when rounded up to the nearest multiple of `layout.align()`, must not overflow `isize` (guaranteed by the success of `Layout::from_size_align`)
_ => NonNull::new(unsafe { std::alloc::realloc(old_ptr, old_layout, new_len) }),
};

if let Some(ptr) = new_ptr {
self.ptr = ptr;
self.len = new_len;
self.deallocation = Deallocation::Standard(new_layout);
Expand Down
Loading