Skip to content

Enable null pointer discriminant optimisation for XDRFILE#12

Open
Yoshanuikabundi wants to merge 1 commit into
dnlbauer:masterfrom
Yoshanuikabundi:owned-xdrfile
Open

Enable null pointer discriminant optimisation for XDRFILE#12
Yoshanuikabundi wants to merge 1 commit into
dnlbauer:masterfrom
Yoshanuikabundi:owned-xdrfile

Conversation

@Yoshanuikabundi
Copy link
Copy Markdown
Contributor

The XDRFile.xdrfile field is now a NonNull<XDRFILE> rather than a *mut XDRFILE, and a PhantomData<XDRFILE> field was added. NonNull allows a null value to be used as the discriminant in Option<XDRFile>, and the PhantomData makes sure that drop checks, send + sync derivation, anything Rust adds in the future etc treat XDRFile as owning an XDRFILE.

For non-generic types like both XDRFILE and XDRFile, NonNull<T> is just a *mut T with the extra guarantee that it will never be null. This is constructed safely by the NonNull::new() method, which performs the check and returns an Option<NonNull<T>>. Generic types with lifetimes have some extra implications about covariance but they're irrelevant here.

The Rustonomicon (and Vec and Box in the standard library) uses a type called Unique<T>, which is a hidden type with the following definition:

pub struct Unique<T: ?Sized> {
    pointer: *const T,
    // NOTE: this marker has no consequences for variance, but is necessary
    // for dropck to understand that we logically own a `T`.
    //
    // For details, see:
    // https://github.com/rust-lang/rfcs/blob/master/text/0769-sound-generic-drop.md#phantom-data
    _marker: PhantomData<T>,
}

So the effect of that type is replicated here with the PhantomData. A struct with a PhantomData<T> acts as though it has a field of type <T> for the purposes of drop checking, auto-deriving the Send and Sync traits, and so on. Raw pointers and NonNull do not have this property. I don't think we technically need the PhantomData because XDRFILE doesn't have a drop implementation, but it doesn't do any harm and it future proofs the implementation against changes in type checking and so on. Since the pointer is only constructed by the open method and is private, its guaranteed to be unique which satisfies the requirements for the Unique pointer type anyway.

What's the benefit? With *mut T, Option<T> and other enums of XDRFile require extra memory to store which variant an instance is. With NonNull, the null value is used as the discriminant and Option should be a byte smaller. Although it may not be if the compiler can find somewhere else to put the discriminant.

@Yoshanuikabundi
Copy link
Copy Markdown
Contributor Author

I did a lot of reading for this one :P

@dnlbauer
Copy link
Copy Markdown
Owner

Hi,
sorry for my ignorance :p
Is XDRFile being one or a few bytes smaller the only benefit from this? Looks like introducing a lot of additional code complexity for only saving a very small amount of RAM. + it now requires to get a raw pointer to the xdrfile every time its used.

@Yoshanuikabundi
Copy link
Copy Markdown
Contributor Author

Getting the raw pointer is a no-op, NonNull is just a raw pointer that makes some promises to the compiler. I'd argue that it's not very much complexity in the actual code, and it also helps keep the code self-documenting to have the promise there that the pointer is non-null. But I also understand that there isn't really a problem there. Up to you :) Was worth implementing for me just for the knowledges :)

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.

2 participants