make small_version fit in a ptr

[Eh2406]

Some notes for safety reviewers. We never dereference a modified pointer. If a pointer is Full then it was constructed from Arc::into_raw and never modified. When we can construct a Small we arrange the appropriate bits in a usize and pretend it's a *const, but we never dereference that value. I believe this makes it fall under the strict provenance model, or at least the fuzzy_provenance_casts and lossy_provenance_casts lints now pass.

to quote https://doc.rust-lang.org/std/ptr/index.html

But it is still sound to:

• Create a pointer without provenance from just an address (see ptr::dangling). ... This can still be useful for sentinel values like null or to represent a tagged pointer that will never be dereferenceable. In general, it is always sound for an integer to pretend to be a pointer “for fun” as long as you don’t use operations on it which require it to be valid (non-zero-sized offset, read, write, etc).

[epage]

Does this happen enough to be worth it? Or is this for translating version reqs into version ranges?

[Eh2406]

Versions with 0 as a pre release are not organically common. But they are the smallest version. So at least with the current version of the code, they do get generated a lot when translating version requirements into PubGrub ranges. A req like ^2 ends up being desugared as the range between 2.0.0-0 (inclusive) and 3.0.0-0 (exclusive).

In general I've done no actual statistics to figure out the best use of each available bit. The fact that I have effectively 15 bits unused (on a 64 bit cpu) is pretty clear evidence that we are not at a maximum.

After this has gotten appropriate safety review. I am totally open to altering the code so that this pattern is generated less. And/Or finding a more structured way to decide what patterns to support.

[Eh2406]

There are 46163 unique versions on crates.io. So if we generated the perfect lookup table it should take 16 bits to be able to uniquely address all of them. Clearly, there's a lot of room for improvement in what we consider small.

[mpizenberg]

40k unique versions seems like an extremely small number for the whole of crates.

[mpizenberg]

https://lib.rs/stats

Maybe not too far off. The crates with the most number of versions have 50k versions it seems.

[jswrenn]

A technique we've adopted in zerocopy in lieu of unsafe fields is defining invariant-carrying structs in a def module that limits access to its implicit constructor; e.g.:

Suggested change

	/// A one pointer wide representation of common `semver::Version`s or a `Arc<semver::Version>`
	///
	/// A `semver::Version` is quite large (5 ptr) to support all kinds of uncommon use cases.
	/// A `Arc<semver::Version>` is 1 aligned ptr, but always allocates and has a cash miss when read.
	/// In practice most versions could be accurately represented by `[u8; 3]`, which is smaller than 1 ptr.
	/// So this type represents common versions as a usize and uses `Arc` for full generality.
	/// The discriminant is hidden in the unused alignment bits of the `Arc`.
	///
	/// The exact set of versions that are common enough to get a small representation depends on the size of a pointer
	/// and is subject to change between releases.
	#[derive(Debug, Eq)]
	#[repr(packed)]
	pub struct SmallVersion(*const semver::Version);
	mod def {
	pub struct SmallVersion {
	/// The version, either packed into a pointer, or allocated on the heap.
	///
	/// # Safety
	///
	/// If and only if the least significant is `0`, `raw` is derived from
	/// [`Arc::into_raw`].
	///
	/// # Invariants
	///
	/// If and only if the least significant bit is `1`, the value of `raw`
	/// should be interpreted as having the layout of [`PackedVersion`].
	raw: *const semver::Version
	}
	impl SmallVersion {
	/// The version, either packed into a pointer, or allocated on the heap.
	///
	/// # Safety
	///
	/// See [`SmallVersion::raw`].
	pub(super) unsafe fn from_raw(raw: *const semver::Version) -> Self {
	// SAFETY: Invariants guaranteed by caller.
	unsafe { Self { raw } }
	}
	pub(super) unsafe fn into_raw(self) -> *const semver::Version {
	self.raw
	}
	}
	pub(super) PackedVersion {
	/// # Safety
	///
	/// The least significant bit is `0`.
	///
	/// # Invariants
	///
	/// The most significant [`Elem`] encodes a SemVer major version,
	/// the next encodes a minor version, and the next encodes a patch
	/// version. The most significant bit of the least significant
	/// `Elem` is `1` if the version is a pre-release; otherwise `0`.
	raw: usize,
	}
	impl PackedVersion {
	/// Constructs a packed version.
	///
	/// # Panics
	///
	/// Panics if the least significant bit of `raw` is `0`.
	pub(super) fn from_raw(raw: usize) -> Self {
	// Enforce
	assert_eq!(raw & 1, 1)
	// SAFETY: Invariants guaranteed by above assertion.
	unsafe { Self { raw } }
	}
	/// Produces the raw packed representation.
	pub(super) fn into_raw(self) -> usize {
	self.raw
	}
	}
	}
	pub use def::*;

This ensures that consumers of SmallVersion must write unsafe before doing something unsafe (like constructing an invalid SmallVersion.

[Eh2406]

I had consider having a separate module, a separate file seemed too much overhead given that it's going to end up having ~1/3 code of this already not very big module. But an in-line module might be a good compromise here. I will give it a try.

[jswrenn]

Note that most of your existing impls should still be placed outside the def module. The only things that go in the def module are constructors, getters, and setters (i.e., things that touch or construct conceptually unsafe fields).

[Eh2406]

Trying to get a sense of what should be inside and outside this module. It feels natural to have SmallVersion::from_arc(arc: Arc<semver::Version>) -> Self and a SmallVersion::from_packed(pack: PackedVersion) -> Self inside the module, instead of having one unsafe from_raw inside and from_arc and from_packed outside. Does that seem reasonable to you?

[jswrenn]

Since the whole thing is a workaround for not having unsafe fields, my preference is to keep the things inside the def module extremely lightweight — just basic constructors/getters/setters. The more complicated, safe items then call into these unsafe items, and Rust's safety tooling makes sure all these use sites are properly unsafe { ... }'d and documented with safety comments. See zerocopy's Ptr definition for an example of the pattern. IMO, this pattern has helped us evolve our highly unsafe codebase in a more sustainable way over a long period of time.

Once unsafe fields are stabilized, the def hack can be removed.

What you suggest isn't unreasonable though! I think from_arc and from_packed are basically one-liners (especially with the zerocopy struct trick), so they're probably lightweight enough to put in the def module.

[Eh2406]

Trying to wrap my head around what should be in the def and what should be outside. On one hand almost nothing, this is a hack for unsafe fields. On the other hand anything that accesses the raw data, which ends up being almost everything.

I did the best I could, and pushed it. Let me know what you think.

[jswrenn]

This is missing a // SAFETY comment — why does without_provenance(s) conform to the invariants of SmallVersion? (This is the sort of missed safety comment that the aforementioned def trick helps avoid.)

[Eh2406]

Odd that std::without_provenance is safe, given that it should have safety comments. Misunderstood what you were saying, I get it now.

[jswrenn]

It's the SmallVersion(...) invocation that's conceptually unsafe — its field has a safety invariant that must be upheld. The without_provenance(s) invocation is fine.

[jswrenn]

perhaps split it over two lines:

Suggested change

	let out = SmallVersion(without_provenance(s));
	let raw = without_provenance(s);
	// SAFETY: the least significant bit of `raw` is `1`. (but say more about why this is true)
	let out = SmallVersion(raw);

[jswrenn]

I think Small's field carries a safety invariant that needs documentation. Perhaps PackedVersion might be a more apt name? I kept finding myself mixed up between SmallVersion and Small.

[jswrenn]

Also, rather than manipulating bits yourself, you could do a target_endian cfg to switch between two struct definitions with the correct layout, then just use normal field accesses:

#[cfg(target_endian = "little")]
#[repr(C)]
struct PackedVersion {
    tag_and_pre: Elem,
    patch: Elem,
    minor: Elem,
    major: Elem,
}

#[cfg(target_endian = "big")]
#[repr(C)]
struct PackedVersion {
    major: Elem,
    minor: Elem,
    patch: Elem,
    tag_and_pre: Elem,
}

[jswrenn]

^This probably gets messy because Elem can be two bytes, and so endian matters there, too. But maybe less messy than bitops?

[Eh2406]

I've tried several representations:

• A plain struct, reading fields is very simple but converting from and to a usize is ugly.
• A [Elme; 4], reading fields is not to bad but converting from and to a usize is not grate.
• A [u8; size_of::<ptr>()], reading fields is ugly but converting from and to a usize is just a from_bl_bits.
• A usize, you can see the consistent mess that is. It has the advantage of not having endian specific code I need to test. but is otherwise pretty painful.

But not yet tried #[repr(C)]. I imagine that reading fields would be as easy as the plane struct. The question is how hard would it be to convert into a pointer/usize?

[jswrenn]

The question is how hard would it be to convert into a pointer/usize?

With zerocopy, you'd roughly achieve it like so:

#[derive(zerocopy::FromBytes, zerocopy::IntoBytes)]
#[cfg(target_endian = "little")]
#[repr(C)]
struct PackedVersion {
    tag_and_pre: Elem,
    patch: Elem,
    minor: Elem,
    major: Elem,
}

impl PackedVersion {
    fn into_usize(self) -> usize {
        zerocopy::transmute!(self)
    }

    fn from_usize(raw: usize) -> Self {
        assert_eq!(raw & 1, 1)
        zerocopy::transmute!(raw)
    }
}

The safety boundary here is weaker (since FromBytes is an unchecked constructor), but we can shore that up with TryFromBytes and a few more lines of code.

[jswrenn]

Concretely, you'd do so like this:

#[derive(TryFromBytes, IntoBytes)]
#[cfg_attr(target_pointer_width = "32", repr(u8))]
#[cfg_attr(target_pointer_width = "64", repr(u16))]
enum Pre {
    No = 1,
    Yes = 3,
}

#[derive(IntoBytes, TryFromBytes)]
#[repr(C)]
struct PackedVersion {
    pre: Pre,
    patch: Elem,
    minor: Elem,
    major: Elem,
}

impl PackedVersion {
    fn into_raw(self) -> usize {
        zerocopy::transmute!(self)
    }

    fn from_raw(raw: usize) -> Option<Self> {
        zerocopy::try_transmute!(raw).ok()
    }
}

This has the advantage, I think, that the LSB constraint is now impossible to violate. (But note that the pre bit is now the second-least significant.)

[Eh2406]

and, without a unsafe field it doesn't need to live in def! I pushed more code, let me know what you think. I needed to custom impl Ord, to get it to match Version. and while I was at it I did a bunch of others as well. Although whether it's more efficient to convert and then do a compare or the other way round is not clear to me. If ever shows up in a benchmark I can revisit it.

What you think?

[jswrenn]

A few nits, but this looks good to me! I'm extremely pleased we managed to encode PackedVersion as a Rust type with no unsafe code!