Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option<*const T> parameters are not zero-cost. #2513

Closed
nathanvoglsam opened this issue May 20, 2023 · 6 comments
Closed

Option<*const T> parameters are not zero-cost. #2513

nathanvoglsam opened this issue May 20, 2023 · 6 comments
Labels
question Further information is requested

Comments

@nathanvoglsam
Copy link

I believe #1939 and this #2036 combine to create many definitions like ID3D12Resource::Map where the function is defined as:

pub unsafe fn Map(
    &self,
    subresource: u32,
    preadrange: ::core::option::Option<*const D3D12_RANGE>,
    ppdata: ::core::option::Option<*mut *mut ::core::ffi::c_void>
) -> ::windows::core::Result<()>

Where preadrange and ppdata are defined as Option<*const/*mut>. This seems wrong as the size of Option<*> is 16 bytes. Raw pointers don't get the niche value optimization for Option like references. The functions won't be inlined across crate boundaries without the #[inline] attribute so there will always be 16 bytes for each option pointer sent into the function in either registers or on the stack. This will always waste stack space or register space when calling these APIs compared to a raw, nullable pointer.

The Option wrappers are not zero-cost, and I question if they add any real value to the Win32/COM bindings. The APIs in this library can't be practically used without referencing the official documentation, so the Option types don't really add information a user wouldn't have already known when calling the API but do add either syntax or memory overhead.

I see two alternatives:

  • Go back to the old, raw pointers
  • Use Option<NonNull<T>> instead

Raw pointers leaves the 'optional-ness' of the parameter unmarked, Option<NonNull<T>> obscures the const or mut qualification as NonNull is always equivalent to *mut.

There isn't really an ideal solution here, but I think adding register or memory pressure needlessly should be avoided.

What do you think?

@kennykerr kennykerr added the question Further information is requested label May 22, 2023
@kennykerr
Copy link
Collaborator

Yes, I'd considered switching to Option<NonNull<T>> - I use that internally in some places where the size matters - but haven't had the time to consider how to deal with multiple pointers. What happens to Option<*mut *mut T> for example? There may be an obvious answer - I've just not thought about this enough and have to consider how that would affect the usability of the API calls. Having to use NonNull::<T>::new(&mut x as *mut _) everywhere seems less than ideal.

Also, thanks to LTCG I think the optimization concern goes away but I haven't tried that myself.

@Muon
Copy link

Muon commented May 29, 2023

It's not obvious that Option(null_mut()) and None mean the same thing, and it's confusing that there are two possible ways to express a null pointer parameter. It's not obvious to me that the convenience of being able to just type None is worth this ambiguity, when you can instead use std::ptr::{null, null_mut} for much the same effect.

@mominul
Copy link
Contributor

mominul commented May 29, 2023

None represents absence of value which is more ergonomic than using raw pointers (null, null_mut). I think we should not encourage the usage of raw pointers, instead phase out their usage more.

@Muon
Copy link

Muon commented May 29, 2023

A null pointer also does that without introducing extra syntactic and compilation overhead. Raw pointers can't be avoided in this API anyway, this is just for the null case. If you're passing in a non null pointer, you still need a raw pointer to do that with. As mentioned, the ergonomics are about the same.

@nathanvoglsam
Copy link
Author

I would definitely agree that Option<*mut T> is worse than just a bare pointer. Both for the reasons I stated initially and because it adds two ways to encode a nullptr, as either None and Some(str::ptr::null_mut()). I think it provides the worst of all options:

  • Extra syntax overhead by wrapping into an Option.
  • Extra overhead relying on LTO to be avoided, internally the binding has to do if let Some(v) = v { v } else { std::ptr::null() } and takes more stack/register space across the function call boundary.
  • Allows for passing in a nullptr even with a Some value.

And the only benefit is being able to type someFn(None) instead of someFn(std::ptr::null()) which can be reduced to someFn(null()) with a use std::ptr::{null, null_mut}. It does also make the function binding self-documenting for which parameters are optional, but how likely is someone to be calling any function in these crates without referencing the API docs anyway?

Option<NonNull<T>> isn't an obvious choice either, as what should happen to ppSomething style parameters? The syntax overhead would keep building with multiple nested Option<NonNull<Option<NonNull<T>>>>. If I had to go with Option<NonNull<T>> I would personally choose Option<NonNull<*mut T>> for the ppSomething case as I think it more correctly encodes the intent, an optional pointer parameter. But this still leaves a raw pointer there so seems like a half measure too, and there's still some syntax overhead for converting the pointer to Option<NonNull<T>>. This scheme does allow expressing non-optional parameters with NonNull<T> though, which would be nice as it's zero cost and easy to do assuming the API metadata can be relied on.

I would use the windows-sys bindings which do follow the simpler raw pointer scheme but I depend on D3D12, DXGI and some WinRT bindings for my project. I would have to depend on both libraries, increasing compile times, to get my ideal Win32 bindings alongside the generated COM and WinRT bindings. And this wouldn't solve my original example either, which is a D3D12 COM API, as it's only available in the windows crate.

@kennykerr
Copy link
Collaborator

This is certainly debatable and rather subjective. One of the reasons that I went with Option is because there are optional parameter types that are not pointers in the windows crate - such as COM interfaces - and I found it confusing to use None for such parameters while using std::ptr::null() or std::ptr::null_mut() for other parameters. There are also arrays and reference types that aren't neatly supported with pointers. The approach taken provides a degree of uniformity to the calling patterns for Windows APIs. Lacking a compelling reason to change it now, I'm inclined to leave this as is. Feel free to keep the conversation going though, always happy for the feedback and perspective.

@kennykerr kennykerr closed this as not planned Won't fix, can't repro, duplicate, stale May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants