Skip to content

Conversation

ensh63
Copy link
Contributor

@ensh63 ensh63 commented Aug 15, 2025

Proposed changes

If resizing is requested for the last allocation in the pool, it may be possible to adjust pool data and avoid any real allocations.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have written my commit messages in the Conventional Commits format.
  • I have read the CONTRIBUTING doc
  • I have added tests (when possible) that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements optimized grow() and shrink() functions for the Pool allocator that can avoid real allocations when resizing the last allocation in the pool by simply adjusting the pool's last pointer.

  • Adds grow() method that can expand the last allocation in-place if there's sufficient space
  • Adds shrink() method that can contract the last allocation in-place by moving the pool's last pointer backward
  • Falls back to allocate-copy-deallocate pattern when in-place resizing isn't possible

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

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

Some minor nitpicks, for the commit log
Making the commit log shorter and adding a new line for the body.

`feat: Add optimized grow()/shrink() functions for Pool

If resizing is requested for the last allocation in the pool, it may be
possible to adjust pool data and avoid any real allocations.`

?

Otherwise looks good.

@bavshin-f5
Copy link
Member

  1. What about Allocator::grow_zeroed?
  2. new_layout.size() >= old_layout.size() is a safety requirement of the Allocator::grow interface. It is acceptable to assume that the opposite never happens and limit handling of this case to debug_assert.
  3. old_layout.size() == new_layout.size() does not guarantee that old_layout == new_layout.
  4. It seems to me that all 3 methods could be generalized to
    #[inline(always)]
    fn resize_in_place(&self, ptr: NonNull<u8>, old_layout: Layout, new_layout: Layout) -> Option<NonNull<[u8]>> { ... }
    
    fn grow(...) {
         ...
         if let Some(new_ptr) = self.resize_in_place(...) {
             Ok(new_ptr)
          } else {
              ...
          }
    }

@ensh63 ensh63 force-pushed the shirykalov/alloc branch 3 times, most recently from 22992a9 to 5bf391f Compare August 16, 2025 00:03
@xeioex
Copy link
Contributor

xeioex commented Aug 18, 2025

@ensh63

BTW, were you able to try it somehow (manual test rust code)? Are you sure it will work in practice?

@ensh63
Copy link
Contributor Author

ensh63 commented Aug 20, 2025

@ensh63

BTW, were you able to try it somehow (manual test rust code)? Are you sure it will work in practice?

It took some time, but here is the text of simple tests:

#[cfg(test)]
mod tests {

    use nginx_sys::{ngx_create_pool, ngx_destroy_pool};

    use super::*;

    unsafe extern "C" {
        pub unsafe fn libngx_init(prefix: *mut nginx_sys::u_char) -> *mut nginx_sys::ngx_cycle_t;
    }

    #[test]
    fn test_pool_resize() {
        let _ = unsafe {
            libngx_init("prefix".as_ptr() as *mut nginx_sys::u_char);
        };

        let mut log: nginx_sys::ngx_log_t = unsafe { core::mem::zeroed() };
        let p: *mut nginx_sys::ngx_pool_t = unsafe { ngx_create_pool(1024, &mut log) };
        let pool = unsafe { Pool::from_ngx_pool(p) };

        let layout = Layout::from_size_align(16, 8).unwrap();
        let slice_ptr = pool.allocate(layout).unwrap();
        let ptr = slice_ptr.as_ptr().cast::<u8>();

        let new_layout = Layout::from_size_align(32, 8).unwrap();
        let nonnull_ptr = unsafe { NonNull::new_unchecked(ptr) };
        let newptr = unsafe { pool.resize(nonnull_ptr, layout, new_layout) };

        assert!(newptr.is_ok());
        assert!(std::ptr::addr_eq(newptr.unwrap().as_ptr(), ptr));

        unsafe { ngx_destroy_pool(p) };
    }

    #[test]
    fn test_vec() {
        let _ = unsafe {
            libngx_init("prefix".as_ptr() as *mut nginx_sys::u_char);
        };

        let mut log: nginx_sys::ngx_log_t = unsafe { core::mem::zeroed() };
        let p: *mut nginx_sys::ngx_pool_t = unsafe { ngx_create_pool(1024, &mut log) };
        let pool = unsafe { Pool::from_ngx_pool(p) };

        let mut v1: allocator_api2::vec::Vec<u8,Pool> = allocator_api2::vec::Vec::new_in(pool);

        v1.reserve(4);
        assert!(v1.capacity() >= 4);
        let v1_ptr1 = v1.as_ptr();

        v1.reserve(4);
        assert!(v1.capacity() >= 8);
        let v1_ptr2 = v1.as_ptr();

        assert!(v1_ptr1 == v1_ptr2);

        v1.resize(4, 1);

        v1.shrink_to_fit();
        let v1_ptr3 = v1.as_ptr();

        assert!(v1_ptr1 == v1_ptr3);

        unsafe { ngx_destroy_pool(p) };
    }

    #[test]
    fn test_two_vecs() {
        let _ = unsafe {
            libngx_init("prefix".as_ptr() as *mut nginx_sys::u_char);
        };

        let mut log: nginx_sys::ngx_log_t = unsafe { core::mem::zeroed() };
        let p: *mut nginx_sys::ngx_pool_t = unsafe { ngx_create_pool(2048, &mut log) };
        let pool = unsafe { Pool::from_ngx_pool(p) };

        let mut v1: allocator_api2::vec::Vec<u8,Pool> = allocator_api2::vec::Vec::new_in(pool.clone());

        v1.reserve(128);
        assert!(v1.capacity() >= 128);
        let v1_ptr1 = v1.as_ptr();

        v1.resize(128, 1);

        let mut v2: allocator_api2::vec::Vec<u8,Pool> = allocator_api2::vec::Vec::new_in(pool);

        v2.reserve(128);
        assert!(v2.capacity() >= 128);

        v1.reserve(128);
        assert!(v1.capacity() >= 256, "actual capacity: {}", v1.capacity());
        let v1_ptr2 = v1.as_ptr();

        assert!(v1_ptr1 != v1_ptr2);
 
        unsafe { ngx_destroy_pool(p) };
   }

}

They work as expected. I don't add these tests to PR because some tricks are needed to link test executable with nginx. These tricks deserve special PR, but they are not generalized yet to be published.

@ensh63 ensh63 force-pushed the shirykalov/alloc branch 2 times, most recently from 8929b39 to 8a0f2ad Compare August 20, 2025 22:10
@ensh63
Copy link
Contributor Author

ensh63 commented Aug 20, 2025

The last changes were done after offline discussions:

  • optimization and some streamlining in resize();
  • switch from inspect() to map() in grow_zeroed() to avoid potentially incorrect use of inspect() for changing data.

If resizing is requested for the last allocation in the pool, it may be
possible to adjust pool data and avoid any real allocations.
@bavshin-f5 bavshin-f5 merged commit 07ce1f7 into nginx:main Aug 20, 2025
15 checks passed
@ensh63 ensh63 deleted the shirykalov/alloc branch August 25, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimize Allocator::grow and Allocator::shrink for the last allocation in ngx_pool_t.

4 participants