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

Many foreign imports make sense to be unsafe imports #4

Closed
jmcarthur opened this issue Feb 29, 2016 · 10 comments
Closed

Many foreign imports make sense to be unsafe imports #4

jmcarthur opened this issue Feb 29, 2016 · 10 comments

Comments

@jmcarthur
Copy link

A lot of Vulkan functions deserve unsafe bindings, for efficiency. I don't know if the spec provides enough information that the generator could figure out which ones to make unsafe automatically. Automatic or not, there are certainly some gotchas, and these are just the ones I can think of right now:

  • Some functions accept callbacks. These should have safe bindings. For those with optional callbacks, perhaps there could also be unsafe bindings, exposed to the user through wrapper functions that don't accept the callbacks.
  • I don't know the spec very well yet, but unless the spec forbids it, I can imagine that Vulkan might store a callback to use later. This possibility could make it difficult to tell from a function's signature whether it might call back into Haskell.
  • Some functions can block. These would need to be identified.
@expipiplus1
Copy link
Owner

Unfortunately the very first call one makes to Vulkan (vkCreateInstance) takes a bunch of callbacks and is entitled to remember them and call them at any time.

Any Create call is able to remember (and use) the allocation callbacks until the corresponding Destroy call.

See here: https://www.khronos.org/registry/vulkan/specs/1.0/xhtml/vkspec.html#memory-allocation

If an implementation intends to make calls through an VkAllocationCallbacks structure between the time a vkCreate* command returns and the time a corresponding vkDestroy* begins, that implementation must save a copy of the allocator before the vkCreate* command returns.

Because of this by default I think it's best to keep these as safe calls to avoid surprises.

Although: I imagine that lots of users are going to just stick with the default malloc/free pair and don't need to be bothered with the safe call overhead so the unsafe alternatives should probably be provided too.

How would be best to expose these unsafe variants?

  • Duplicating every Vulkan command and adding a suffix along the lines of UnsafeNoCallbacks or NoCallbacks
  • Duplicating every Vulkan command and adding a low profile suffix such as '
  • Duplicating every module so one could import Graphics.Vulkan.Device or Graphics.Vulkan.Device.NoCallbacks
  • Duplicating every module so one could import Graphics.Vulkan.NoCallbacks.Device instead.
  • Something else altogether

@expipiplus1
Copy link
Owner

It might even make more sense to invert those alternatives and provide the unsafe functions by default, and require using alternative command or module names if one requires using Haskell callbacks.

@jmcarthur
Copy link
Author

I like the duplicated modules. It gives users more control over scope and module qualification. I have no opinion on whether to put the NoCallbacks portion of the name near the beginning or the end, nor on which alternative should be the default.

@expipiplus1
Copy link
Owner

Sounds good to me.

I'll get down to closing this issue after the other issues with this package are fixed (Show instances etc...). I can't imagine that anyone's going to be requiring losing this safe call overhead for a little while yet.

@expipiplus1
Copy link
Owner

Hmm, I wonder if this could be done with a configuration flag. Along the same lines as vector's "don't do any bounds checking" flag.

@achirkin
Copy link

I would suggest having a special flag for this. Moreover, maybe it makes sense to generalize the flag idea for something like dev with safe calls and automatic error checking after each command?
I've seen nice a approach in some yesod's projects:

vulkan.cabal:

flag dev
    description: Turn on development settings.
    default: True

library
    if (flag(dev))
        cpp-options: -DDEVELOPMENT
        ghc-options: -Wall -fwarn-tabs -O0
...

*.hs:

{-# Language CPP #-}
...
#if DEVELOPMENT
foreign import ccall "vkFunc" vkFunc :: Param -> IO VkResult
#else
foreign import ccall unsafe "vkFunc" vkFunc :: Param -> IO VkResult
#endif

@expipiplus1
Copy link
Owner

Well, it's not exactly a development setting. It's an optimization which is only safe to do when one hasn't given Vulkan any callbacks which could come back into Haskell-land.

@expipiplus1
Copy link
Owner

After consulting with the #vulkan channel it seems as though idiomatic usage is for development to track allocations. Seems like custom allocators are not a widely used feature anyway.

@achirkin
Copy link

I just made two copies of each function to let user decide if they want to play risky or not :)
Also, I think it may be ok to call some vulkan functions unsafe even with callbacks attached, because the functions seem return immediately (callbacks are executed later, when the haskell runtime is resumed and there is no danger of blocking during FFI call). Though, I have not tested this yet.

expipiplus1 added a commit that referenced this issue Apr 22, 2018
@expipiplus1
Copy link
Owner

Unsafe calls are now enabled by default, they can be made safe with the safe-foreign-calls flag.

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

No branches or pull requests

3 participants