Refactor gc

[CppCXY]

No description provided.

[github-actions]

Reasoning Details

We need to review the provided code changes. The diff shows several files modified and new files added in a Rust project (luars). The changes involve introducing a paged pool allocator (PagedPool, Pooled) and a table allocator (TableAllocHandle), and modifying various types to use these new allocation strategies instead of Box. The GcObjectOwner enum now uses Pooled instead of Box for most variants except Thread (still Box). The object_allocator creates objects via pools. The string_interner now takes a PagedPool parameter. The native_table uses TableAllocHandle for allocating array and hash memory. Also, a clippy allow annotation added.

Potential issues to check:

1. Memory safety: The Pooled type uses Rc<RefCell<...>> for inner pool management, but Pooled is not Send/Sync. That may be okay if all GC objects remain on one thread (Lua state). However, the GC might be accessed from multiple threads? Usually Lua is single-threaded. But we need to ensure no concurrent access.
2. The Pooled drop implementation calls drop_in_place and then pushes slot back to free list. The free list is a Vec of NonNull, which is correct. But we need to ensure that the slot is correctly reclaimed and not double-freed. The debug_assert for occupied will catch double-free in debug mode.
3. In GcObjectOwner::raw_header_ptr, for Thread variant, it uses t.as_ref() as *const _ as *mut GcHeader. This assumes that the Box has a stable address and that as_ref gives the correct pointer. It's fine for Box.
4. In Pooled, the as_ptr and as_mut_ptr methods: they dereference the slot pointer to get the value pointer. The slot is guaranteed to be occupied when Pooled is alive. However, note that as_mut_ptr returns a mutable pointer even though Pooled is not &mut self. That's okay for obtaining a raw pointer, but careful with aliasing. The Pooled type deref's to T via &*self.as_ptr(), which is safe because Pooled owns the value (slot occupied). But mutable access via as_mut_ptr and deref_mut also works as long as the Pooled is the sole owner. Since Pooled is not Copy/Clone? It is not Clone (needs to be). But if someone clones the Pooled? It would be problematic because two Pooled would point to same slot. The Pooled probably should not be Clone. Check if Pooled is Clone? It's not in the code. That's good.
5. The Thread variant still uses Box. That might be intentional because threads are special. But consistency? Possibly fine.
6. In object_allocator.rs, the pools are created with fixed sizes (128, 64, etc.). That might be okay as starting sizes but could lead to many small allocations if pool grows. The grow method doubles page size each time. That's fine.
7. The TableAllocHandle uses an Rc<RefCell> for free lists. That's shared across tables. If multiple tables are created and dropped, the free lists could accumulate memory. Not a memory leak, but could cause high memory usage if many different sizes are used. However, it's a custom allocator for small objects (<=64 nodes for hash, <=64KB for array). That's acceptable for performance.
8. In native_table, the alloc/free functions now call into TableAllocHandle. The free functions check for null and size zero. But in the original code, layout::array with size 0 might cause issues? The new code returns early if size 0, which is safer.
9. In native_table Drop, it uses self.hash_allocator.free_array_bytes and free_hash_nodes. But note that the hash_allocator is not used for allocation of Node? Actually, it is used: alloc_hash_nodes and free_hash_nodes. That's fine. However, the drop should also free the array if it was allocated. It does.
10. The StringInterner methods now require a &mut PagedPool<GcString> parameter. That changes the API and all callers. In the diff, the callers in object_allocator pass &mut self.string_pool. But note that StringInterner::intern is called with a mutable reference to the pool, which is fine.
11. The ObjectAllocator holds multiple PagedPool fields. They are mutable. The create_string methods take &mut self so they can access the pools. That's okay.
12. In paged_pool.rs, the test stable_addresses_survive_growth creates a pool with page_len=2 and allocates 1 + 32 items, checking that first pointer remains stable. The grow method allocates a new page and adds slots to free list. The slots themselves are in the page's Box<[PageSlot]>. The page is stored in Vec<Box<...>>, so it won't move. That ensures stable addresses. Good.
13. In table_allocator.rs, the alloc_array_bytes and free_array_bytes use Layout::from_size_align. Note that the alignment is passed as argument. In native_table, they call self.hash_allocator.free_array_bytes(start_ptr, total_size, std::mem::align_of::<Value>()). That's consistent.
14. One potential issue: In TableAllocInner, the free lists are HashMap<usize, Vec<usize>>. The address stored as usize. That's okay on 64-bit, but if the system is 32-bit, usize is 32-bit, which could be problematic if pointers are larger? Actually, it's fine because usize is pointer-sized. But storing a pointer in a usize is safe but not portable if pointer representation differs. Usually it's fine for same platform. But it would be better to store NonNull or *mut. However, since it's internal and only used for tracking, it's acceptable. But using NonNull would be more idiomatic. However, note that NonNull does not implement Hash/Deref? It does implement Hash, and can be used as key. Could be improved.
15. The TableAllocHandle is Clone via #[derive(Clone)]. That clones the Rc, which increases reference count. That's intended so multiple tables can share the same allocator handle. However, the inner RefCell allows interior mutability. If multiple tables across threads? But again, single-threaded. It's fine.
16. In gc_object.rs, the GcObjectOwner::raw_header_ptr for Thread uses t.as_ref() as *const _ as *mut GcHeader. For Box, as_ref() gives a reference to the pointee, which is fine. But note that the other variants use s.as_ptr() from Pooled. This inconsistency is acceptable because Thread is still Box.
17. The removal of box_raw_ptr function: It was reading the internal pointer of Box by transmuting the reference to Box to a const pointer. That was a hack to avoid Miri stacked borrows issues. Now they use Pooled::as_ptr, which is clean. But for Thread, they still need to get the pointer from Box. They do it via t.as_ref() as *const _ which is safe and not the same hack. So that's good.
18. The as_thread_ptr method now uses t.as_ref() as *const _ instead of Self::box_raw_ptr(t). That's correct.
19. In gc_object.rs, the raw_header_ptr and as_gc_ptr now use Pooled::as_ptr for all except Thread. For Thread, they use t.as_ref() as *const _ as *mut GcHeader. Ensure that this yields the correct pointer to the GcHeader (first field). Since GcThread starts with a GcHeader field? Likely yes, but we need to check the struct definition. Assuming it does, it's fine.
20. The from_value method in lua_api/mod.rs added a #[allow(clippy::wrong_self_convention)]. That's a minor style issue, not a problem. The function takes &mut self, which might be named from_value, but clippy expects from_value to be a constructor that takes owned self? Actually, clippy's wrong_self_convention might warn if a method named from_value takes &mut self instead of self or &self. Allowing it is fine.

Overall, the changes introduce custom pooling for GC objects and table memory. This is likely for performance (reducing allocation overhead, better cache locality). The code seems well-implemented with attention to safety. However, there are a few things to double-check:

• In paged_pool.rs, the PagedPoolInner::grow method pushes slots in reverse order to free list. That's fine.
• In Pooled::drop, after drop_in_place, it sets occupied = false and pushes the slot. But what if drop_in_place panics? Then the slot might be left in an inconsistent state. That's a general Rust issue; panic during drop is undefined behavior, so it's not a concern.
• The Pooled type does not implement Send or Sync. Since it uses Rc and RefCell, it is !Send and !Sync. That's fine for single-threaded usage. However, if the GC is used with threads (e.g., multiple Lua states), each state should have its own pools. So no issue.
• In table_allocator.rs, the alloc_array_bytes function uses Layout::from_size_align(total_size, align). It assumes that align is a power of two and that the layout is valid. It should be safe.
• In native_table.rs, the resize_array method calls self.hash_allocator.free_array_bytes with old_start which is computed as self.array.sub(old_values_size). But self.array points to the beginning of the values array, not the start of the allocation (which includes lenhint and tags). The comment says "array pointer points to lenhint, need to go back to start". Actually, in the alloc function, the start_ptr is returned, and then self.array = unsafe { (start_ptr as *mut Value).add(values_size) }. So indeed array points to the first Value after lenhint. So sub(values_size) goes back to the start of the whole allocation. That matches the free call. Good.
• The resize_array function creates a new allocation and then frees the old one. It uses self.hash_allocator.free_array_bytes with the old_start. That's correct.
• The resize_hash function similarly uses self.hash_allocator.free_hash_nodes for old node.
• The Drop for NativeTable also correctly frees array and hash.

Potential improvement: In NativeTable::new, if array_cap > 0, it calls self.resize_array(array_cap). But resize_array expects the array to be allocated? Actually, at that point self.array is null, and self.asize is 0. The resize_array function will allocate new memory. That's fine.

One subtle issue: In resize_array, when the new size is 0 (deallocation case), the code sets self.array = ptr::null_mut(); self.asize = 0; and frees old. That's fine. But when new size > 0, it allocates, then moves old values, then frees old. However, if the old array was also non-null, the code captures old_size from self.asize before changing. That's correct.

Another thing: In resize_hash, if old_size == 0, it uses dummynode() instead of allocating. That's fine.

Now, regarding the GC: The GC likely walks through GcObjectOwner variants and needs to mark/release objects. With Box, it used to drop the Box when the object is collected. Now with Pooled, the Pooled will be dropped, which returns the slot to the pool. That's good for reuse. But the drop order: When GC collects an object, it will drop the GcObjectOwner, which drops the Pooled, which returns the slot. But the pool itself is owned by ObjectAllocator, which lives longer (the Lua state). That's fine.

Potential memory leak: If the GC never collects, the Pooled objects remain, but that's fine. The pool pages are never freed until the pool is dropped. That's intended for reuse.

One potential issue: The PagedPoolInner::grow creates a new page and pushes all its slots to free_slots. But the page is stored in pages. However, when the PagedPool is dropped (when ObjectAllocator is dropped), the pages will be dropped, which deallocates the memory. But the Pooled objects may still exist? Actually, Pooled holds a Rc to the inner, so the inner will not be dropped until all Pooled are dropped. That's correct. But when the PagedPool is dropped (the outer struct), the inner Rc may still have references. That's fine, the inner lives as long as any Pooled exists. However, the pages vector is inside PagedPoolInner, which is reference counted. So pages will not be freed until all Pooled are dropped. That's good.

Now, check for any obvious unsoundness: In Pooled::as_mut_ptr, it does (&mut *self.slot.as_ptr()).value_mut_ptr(). This creates a mutable reference to the slot, which is allowed because we are in a unique owner (Pooled is unique? But Pooled is not Clone, but it could be moved, but the slot is not shared). The RefCell in the inner is only used for free list management, not for slot access. So the slot itself is not protected. The Pooled owns the slot exclusively. So this is safe.

However, in the drop method, we borrow the inner (RefCell) to manipulate the free list. At the same time, we drop the value in the slot. That's fine because we have a mutable reference to self (slot) and we borrow the inner mutably. No conflict.

One more thing: The PageSlot has occupied flag, but there's no check when accessing the value that it's occupied. In Pooled::as_ptr, it assumes the slot is occupied, which is true as long as Pooled exists. But if someone incorrectly calls as_ptr on a moved-from Pooled? That's not possible because Pooled does not have a moved-from state (no Default). So it's safe.

Overall, the changes appear sound and well-structured. However, I should note any potential issues in the review answer.

Review checklist items:

• Memory safety: OK (uses raw pointers carefully, pools manage memory).
• Thread safety: Not an issue (single-threaded).
• Performance: Good.
• Code style: Minor clippy allow, but okay.
• Potential bugs: None obvious.
• Completeness: The TableAllocHandle free lists might grow unbounded if many different sizes are allocated, but that's manageable.

Thus, the review will note that the changes are generally good, but might point out for the reviewer to consider using NonNull in free lists instead of usize, and ensure that Box consistency is intentional. Also, check that the Pooled type is not accidentally Clone or Copy (it is not, but ensure). Also, note that the TableAllocHandle free lists might cause high memory usage if not capped; but it's a trade-off.

Given the instructions: "Identify potential issues such as code style violations, logical errors, security vulnerabilities, and provide improvement suggestions. Clearly list the problems and recommendations in a concise manner."

I'll produce a list of observations and recommendations.

Code Review Findings

Overview

This PR introduces custom pool-based allocators (PagedPool, TableAllocHandle) replacing Box for most GC-managed objects and table memory. The goal is to reduce allocation overhead and improve cache locality. Overall implementation looks well thought out, but a few areas warrant attention.

---

Potential Issues & Recommendations

1. Pooled not Clone or Copy – Confirmed safe

Pooled does not implement Clone or Copy, which prevents sharing a slot. Good.

2. Pooled is !Send and !Sync – Intentional, but document

Pooled uses Rc<RefCell<…>>, making it non‑thread‑safe. Since Lua states are single‑threaded, this is fine. However, adding a comment or !Send/!Sync impl markers would make the safety contract explicit.

3. GcObjectOwner::Thread still uses Box – Inconsistency

All other variants now use Pooled<T>, but Thread remains Box<GcThread>. This may be intentional (threads are special), but it breaks the uniform memory management pattern. Consider either migrating Thread to Pooled or adding a clarifying comment.

4. TableAllocHandle stores raw addresses as usize – Minor safety concern

In TableAllocInner, free lists store usize instead of *mut u8 or NonNull<u8>. This works on all common platforms, but is less type‑safe and makes the code less portable.
Recommendation: Replace Vec<usize> with Vec<NonNull<u8>> and adjust the Hash/Eq implementations accordingly.

5. TableAllocHandle free lists unbounded growth – Potential memory usage

The free lists (HashMap<usize, Vec<…>>) accumulate freed memory for different sizes. Over time, this could hold a large amount of memory, especially if many distinct array/hash sizes are used across many tables.
Recommendation: Consider adding a size cap (e.g., maximum number of cached blocks per size) or a time‑based eviction policy.

6. unsafe in Pooled::as_mut_ptr – Aliasing assumption

as_mut_ptr obtains a mutable pointer even with a &self receiver. The caller must ensure exclusive access. This is acceptable because Pooled owns the slot, but adding a // Safety comment would help maintainers.

7. paged_pool.rs test: freed_slots_are_reused – Relies on pool growth order

The test expects that after dropping the first Pooled, the slot is reused immediately. This is correct given the LIFO free‑list, but it’s an implementation detail. The test is fine as a regression check.

8. StringInterner now requires explicit PagedPool parameter – API breaking

All interning methods now take &mut PagedPool<GcString>. This is a necessary change, but ensure external code (if any) is updated. The diff shows internal callers are updated correctly.

9. #[allow(clippy::wrong_self_convention)] – Stylistic, acceptable

The warning is suppressed because from_value takes &mut self. If the method is meant to be a constructor, the name could be changed (e.g., to_value?), but the suppression is harmless.

10. Double‑free safeguard in Pooled::drop – Good

The debug_assert!(!slot_ref.occupied) catches double‑drops in debug builds. The occupied flag ensures a freed slot is not reused while a Pooled still exists.

Summary

The changes are logically sound and well‑crafted. The main recommendations are to address the usize‑vs‑NonNull issue in TableAllocHandle, document the thread‑safety assumptions, and consider adding a cap on free‑list sizes. The Box<GcThread> inconsistency is minor but worth clarifying. No critical security or correctness bugs are apparent.