Remove allocation for ZSTs by Baptistemontan · Pull Request #92 · mozilla/thin-vec

Baptistemontan · 2026-09-01T14:43:17Z

Store the length as a NonZero<usize> inside the NonNull<Header>, the pointer is actually never read for ZSTs, when one is needed just create a dangling one.
The only problem is that now the length is off by one (as it can't be 0), this makes the maximum capacity usize::MAX - 1 instead, this might be a breaking change ?
Did'nt enabled those optimizations for feature = "gecko-ffi", as it can't be turned into nsTArray, tho from my understanding ZSTs are undefined in C++ so it would be UB to turn a ThinVec of ZSTs into a nsTArray anyway, but I prefer to get confirmation first

emilio

Curious, can you elaborate on what's the rationale for this? It seems somewhat complicated and having an array of ZSTs is already pretty edgecasey.

In general, I'd rather keep the differences between the gecko-ffi path and the other as small as possible.

Baptistemontan · 2026-09-01T15:16:03Z

Deliberately constructing a Vec of ZSTs is very niche, but it's actually pretty common when generics comes into play, deeply nested in data structures. This lib might be a bridge for Gecko but it's used a lot outside of that space by other people, and removing allocations for ZSTs is basically free.
Also, the crate doc says

ThinVec currently doesn’t bother to not-allocate for Zero Sized Types (e.g. ThinVec<()>), but it could be done if someone cared enough to implement it.

Well I'm that someone lol

And for keeping the same paths, it could be enabled on both since ZSTs should'nt be passed to C++ anyway

emilio · 2026-09-15T16:37:26Z

    unsafe fn reallocate(&mut self, new_cap: usize) {
        debug_assert!(new_cap > 0);
+        debug_assert!(!Self::is_zst());
        if self.has_allocation() {


has_allocation still returns true for ZSTs, which seems rather footgunny.

totally footgunny, I'm gonna update has_allocation.
But in the case of reallocate even if has_allocation return false it still take a bad path in case T is ZST.

emilio · 2026-09-15T16:38:13Z

+#[cold]
+#[inline(never)]
+fn drop_zsts<T>(this: &mut ThinVec<T>) {


Why is this cold? is_singleton is false for zst so aren't you forcing a function call for no reason?

Honestly I was probably just following drop_non_singleton, but yeah you're correct, unlike singletons ZSTs don't have a branch, I should just put the drop in place inside the drop function

Baptistemontan · 2026-09-15T17:15:15Z

Humm, tests are going to fail for "const_new", I can't make len_to_ptr_unchecked const with msrv 1.53, but const new already requires 1.83, I would like to have the const_new version of len_to_ptr_unchecked to use NonNull::without_provenance instead of transmute shenanegans but it's const stable only for 1.89, so for now I'll just duplicate the function and slap const under a feature cfg.

emilio · 2026-09-15T17:37:52Z

I think we should just bump the MSRV, see #93. Can you rebase atop?

…place inside the drop impl

Baptistemontan · 2026-09-15T17:49:43Z

@emilio done

emilio

Thanks, mostly nits, looks good with this!

emilio · 2026-09-15T18:20:03Z

+            }
+        } else {
+            // See the comment in with_capacity().
+            let _ = padding::<T>();


I was planning to disallow ZSTs in gecko-ffi mode altogether, see #94. I think you need to keep calling padding unconditionally.

emilio · 2026-09-15T18:26:11Z

-        if old_len == self.capacity() {
+        if Self::is_zst() {
+            assert!(old_len < MAX_CAP);
+        } else if old_len == self.capacity() {


I think this would be cleaner like this:

if old_len == self.capacity() { if Self::is_zst() { capacity_overflow(); } self.reserve(1); }

That gives consistent panic messages regardless of ZST.

emilio · 2026-09-15T18:29:53Z

            self.set_len_non_singleton(old_len - 1);
-            Some(ptr::read(self.data_raw().add(old_len - 1)))
+            if Self::is_zst() {
+                Some(mem::zeroed())


Do you know if this matches what Vec or so does? It seems ok-ish to me but worth double-checking.

just checked, and Vec actually don't branch on ZSTs and just read from the pointer, I'll do that then, and my guess is that it's going to compile to the same thing as a pointer read of ZSTs are noop anyway

emilio · 2026-09-15T18:30:53Z


        assert!(idx <= old_len, "Index out of bounds");
+        if Self::is_zst() {
+            assert!(old_len < MAX_CAP);


Same remark about reserve() and the panic message. In fact maybe reserve() should just have a if Self::is_zst() { capacity_overflow(); }? Then the specialness can be reduced.

make sense, and with the ocmment about pop I checked Vec for that too and they don't have special handling of ZSTs either for insert, just write the value to the offseted ptr, so I'll reduce the special handling there too

emilio · 2026-09-15T18:32:01Z

            return;
        }
+        // only way to get here is if min_cap == usize::MAX, which we can't handle.
+        if Self::is_zst() {


Ah, there we go, so I think some of the special cases elsewhere aren't really needed.

Baptistemontan

Alright I removed a good chunk of unnecessary zst checks (either unneeded or checked afterward anyway).
I think this should wait for #94 to merge, there might be some conflicts as those two PRs both adresses ZSTs

Baptistemontan · 2026-09-15T18:50:51Z

+            }
+        } else {
+            // See the comment in with_capacity().
+            let _ = padding::<T>();


Baptistemontan · 2026-09-15T18:53:06Z

-        if old_len == self.capacity() {
+        if Self::is_zst() {
+            assert!(old_len < MAX_CAP);
+        } else if old_len == self.capacity() {


Baptistemontan · 2026-09-15T18:54:33Z

            self.set_len_non_singleton(old_len - 1);
-            Some(ptr::read(self.data_raw().add(old_len - 1)))
+            if Self::is_zst() {
+                Some(mem::zeroed())


just checked, and Vec actually don't branch on ZSTs and just read from the pointer, I'll do that then, and my guess is that it's going to compile to the same thing as a pointer read of ZSTs are noop anyway

Baptistemontan · 2026-09-15T19:00:45Z


        assert!(idx <= old_len, "Index out of bounds");
+        if Self::is_zst() {
+            assert!(old_len < MAX_CAP);


make sense, and with the ocmment about pop I checked Vec for that too and they don't have special handling of ZSTs either for insert, just write the value to the offseted ptr, so I'll reduce the special handling there too

emilio

Last pass, I promise. Fairly minor things :)

emilio · 2026-09-16T11:18:32Z

+/// len must be != 0, this uses the `NonNull` to store a length, so the length must be stored offset by one.
+/// This function expect the len to be already shifted
+#[inline(always)]
+const unsafe fn len_to_ptr_unchecked<T: Sized>(len: usize) -> NonNull<T> {


Worth a debug_assert!(len != 0); at least.

emilio · 2026-09-16T11:19:13Z

        // `Drop` impl, trippng an assertion along that code path causes a
        // double panic. We duplicate the assertion here so that it is
        // testable,
+


Random newline?

emilio · 2026-09-16T11:19:24Z

    ///
    /// // Only true **without** the gecko-ffi feature!
-    /// // assert_eq!(vec_units.capacity(), usize::MAX);
+    /// // assert_eq!(vec_units.capacity(), usize::MAX - 1);


Ok, I enabled this in the other PR so will need a trivial-ish rebase.

emilio · 2026-09-16T11:20:04Z

+        debug_assert!(
+            len <= MAX_CAP,
+            "invalid set_len(usize::MAX) on ZST ThinVec (max cap is usize::MAX - 1)"
+        );


I guess this assert isn't super useful, because len + 1 would debug-assert anyway. But fine to keep.

emilio · 2026-09-16T11:21:06Z

    fn is_singleton(&self) -> bool {
-        unsafe { self.ptr.as_ptr() as *const Header == &EMPTY_HEADER }
+        // could technicaly remove this branch
+        // but there is a 1/2^64 chance of the number of ZST being equal to &EMPTY_HEADER


I'd remove this comment, as given that chance you can't remove the branch, right? :)

emilio · 2026-09-16T11:21:58Z

+/// # Safety
+///
+/// This function drop and deallocates the inner values of the `ThinVec`,
+/// invariants are therefore brokens and the value must be considered dropped and should not be accessed again.


typo: broken.

emilio · 2026-09-16T11:23:19Z

 impl<T> MallocShallowSizeOf for ThinVec<T> {
    fn shallow_size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
-        if self.capacity() == 0 || self.uses_stack_allocated_buffer() {
+        if self.capacity() == 0 || self.uses_stack_allocated_buffer() || Self::is_zst() {


This could be simplified to if !self.has_allocation() {

emilio reviewed Sep 1, 2026

View reviewed changes

emilio requested changes Sep 15, 2026

View reviewed changes

Baptistemontan requested a review from emilio September 15, 2026 17:18

Baptistemontan added 6 commits September 15, 2026 19:43

don't allocate for ZSTs

2c7a637

always inline is_zst()

b0e5302

fix typo

cfb12d3

fix importing NonZeroUsize from std and import from core instead

2590c0e

has_allocation now return false on ZSTs, and inlined the ZST drop_in_…

9c4413d

…place inside the drop impl

made len_to_ptr_unchecked const for the const_new feature

4edd4dd

Baptistemontan force-pushed the no_alloc_zst branch from 56c9eb0 to 4edd4dd Compare September 15, 2026 17:47

removed polyfill for ptr::dangling_mut

d7472fa

emilio requested changes Sep 15, 2026

View reviewed changes

fix typo

6746ae6

Baptistemontan commented Sep 15, 2026

View reviewed changes

remove unecessary zst checks

527496d

emilio approved these changes Sep 16, 2026

View reviewed changes

Uh oh!

Conversation

Baptistemontan commented Sep 1, 2026

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Baptistemontan commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Baptistemontan commented Sep 15, 2026

Uh oh!

emilio commented Sep 15, 2026

Uh oh!

Baptistemontan commented Sep 15, 2026

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Baptistemontan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Baptistemontan commented Sep 1, 2026 •

edited

Loading