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

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?

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

@emilio done

emilio

Thanks, mostly nits, looks good with this!

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

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.

technically already checked by NonZero::new_unchecked but it would probably help with backtrace and stronger code documentation

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.

no sure what you mean by that

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() {

…place inside the drop impl

emilio · 2026-09-16T14:45:13Z

FWIW I don't think that the MAX_CAP is a breaking chance in practice, IIUC the slice array size is capped at isize::MAX:

The isize type is a signed two’s complement integer type with the same number of bits as the platform’s pointer type. The theoretical upper bound on object and array size is the maximum isize value. This ensures that isize can be used to calculate differences between pointers into an object or array and can address every byte within an object along with one byte past the end.

https://doc.rust-lang.org/stable/reference/types/numeric.html#r-type.numeric.int.size.isize

emilio · 2026-09-16T14:45:30Z

Thanks for the fix and for bearing with me btw! :)

Baptistemontan · 2026-09-16T14:46:03Z

    /// **NOTE**: unlike `Vec`, `ThinVec` **MUST** allocate once to keep track of non-zero
    /// lengths. As such, we cannot provide the same guarantees about ThinVecs
    /// of ZSTs not allocating. However the allocation never needs to be resized
    /// to add more ZSTs, since the underlying array is still length 0.


@emilio not sure what to do with this paragraph, now that ZSTs are simply not allowed with gecko-ffi, and don't allocate without the feature the whole thing is wrong.
what about

/// **NOTE**: like `Vec`, `ThinVec` does'nt allocate for ZSTs and store the length inline, /// but creating a `ThinVec` of ZSTs is not allowed if the "gecko-ffi" feature is enabled.

That looks about right!

Baptistemontan · 2026-09-16T14:47:16Z

Thanks for the fix and for bearing with me btw! :)

No problem !
Thanks for your time

Baptistemontan · 2026-09-16T14:53:19Z

ahem, there's still some docs I was updating lol

emilio · 2026-09-16T14:54:41Z

Err, sorry! Can you send another PR for those?

Baptistemontan · 2026-09-16T14:57:36Z

Err, sorry! Can you send another PR for those?

sure, give me a moment

emilio reviewed Sep 1, 2026

View reviewed changes

emilio requested changes Sep 15, 2026

View reviewed changes

Comment thread src/lib.rs

Comment thread src/lib.rs

Comment thread src/lib.rs

Comment thread src/lib.rs Outdated

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

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

emilio requested changes Sep 15, 2026

View reviewed changes

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs

Comment thread src/lib.rs

Baptistemontan commented Sep 15, 2026

View reviewed changes

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs

emilio approved these changes Sep 16, 2026

View reviewed changes

Baptistemontan added 10 commits September 16, 2026 16:21

don't allocate for ZSTs

2cbd648

always inline is_zst()

011927e

fix typo

3841784

fix importing NonZeroUsize from std and import from core instead

1059bc6

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

16221cc

…place inside the drop impl

made len_to_ptr_unchecked const for the const_new feature

34680ec

removed polyfill for ptr::dangling_mut

18f14e1

fix typo

c18315e

remove unecessary zst checks

66db9d6

minor changes

127a785

Baptistemontan force-pushed the no_alloc_zst branch from 527496d to 127a785 Compare September 16, 2026 14:21

emilio approved these changes Sep 16, 2026

View reviewed changes

emilio merged commit 696791e into mozilla:main Sep 16, 2026
5 checks passed

Baptistemontan commented Sep 16, 2026

View reviewed changes

Baptistemontan deleted the no_alloc_zst branch September 16, 2026 14:59

Baptistemontan mentioned this pull request Sep 16, 2026

chore(doc): update zst doc #96

Merged

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!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Baptistemontan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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 commented Sep 16, 2026

Uh oh!

emilio commented Sep 16, 2026

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Baptistemontan commented Sep 16, 2026

Uh oh!

Baptistemontan commented Sep 16, 2026

Uh oh!

emilio commented Sep 16, 2026

Uh oh!

Baptistemontan commented Sep 16, 2026

Uh oh!

Reviewers

Baptistemontan commented Sep 1, 2026 •

edited

Loading

Baptistemontan Sep 16, 2026 •

edited

Loading