ethorsoe / OpenCLWrappers

haskell wrappers for OpenCL
Other
6 stars 4 forks source link

Error-handling could be nicer #1

Open ehird opened 12 years ago

ehird commented 12 years ago

This is just a duplicate of my issue for the OpenCL package, since the situation and suggested fix is the same, so rather than writing anything here I'll just link there. :-)

ethorsoe commented 12 years ago

I agree that this aspect is painful. I used Error in examples to simulate this behaviour, but it's boilerplatey.

Mostly I'm saying that your bug report has been read, not sure what to do yet =o)

ehird commented 12 years ago

Thanks for taking a look :-)

I don't know of a nicer solution than exceptions, but since the bindings are low-level there's not really any pure code to catch errors in, so they fit quite well.

ErrorT works (and indeed the types match up perfectly, since it's actually EitherT with an evocative name), but monad transformers over IO can be quite painful, and it requires a specific layer of wrapping just for OpenCL errors.

I'll wait to see what solution you come up with — thanks!

ethorsoe commented 12 years ago

Something we should also consider here is that is there some way to automatically deconstruct allocated resources in a monad block on exception (though this problem is also present with eg. ErrorT, it's also something we should consider at this point).

Is there some sort of exception that allows for insertion of deconstructors? Should we use foreign pointers that allow for automatic deallocation?

ehird commented 12 years ago

Yes, ForeignPtr is the right thing to use here; you give it a pointer to a C deallocation function and it's called when the Haskell object is GC'd. It should be fairly easy to do.

It's not deterministic destruction, but neither is the destruction of ordinary Haskell objects.

ethorsoe commented 12 years ago

It seems to me that eg. context is not really used much after CommandQueue has been created, this would require user not to discard Context ahead of time in a memory leaky fashion (like the examples do).

ehird commented 12 years ago

Not sure I understand — a ForeignPtr getting freed can't break things unless a C struct has a pointer to it; the solution to that is to extend the Haskell data type for the C struct to include a reference to the ForeignPtr.

Manually freeing is sometimes important, though; for instance, control of GPU memory (including OpenCL objects that can take up GPU memory — I wonder if there's a canonical list somewhere? Maybe not, since OpenCL tries to be pretty device-agnostic...) should probably stay pretty manual. Guarding against exceptions is pretty easy there:

do foo <- allocateGPUMemory
   useMemory foo `finally` freeGPUMemory foo

An easy solution would be to offer two forms of allocation for each type:

newFoo :: ... -> IO Foo  -- ForeignPtr-based
withFoo :: ... -> (Foo -> IO r) -> IO r  -- Ptr-based; does { callback foo `finally` freeMemory foo }

Unfortunately, this is pretty ugly, since it forces a relatively simple allocation/deallocation style for deterministic allocation (though some would consider this a good thing...), and requires each Foo function to be able to handle both the Ptr and ForeignPtr case (not a big deal, since it'd be a trivial helper function, but still suboptimal).

I'm inclined to say that the best solution is to use ForeignPtrs everywhere and not expose any freeing functions. The Haskell garbage structure of a well-written program will mirror the actual garbage structure, and garbage collections are fairly frequent, so GPU RAM shouldn't stay garbage for all that long. You could always just use System.OpenCL.Wrappers.Raw if you really need completely deterministic deallocation.

ethorsoe commented 12 years ago

Destroying the Context destroys all objects within the Context regardless of any member pointing to it. Adding complex data structures for Mem and CommandQueue will expose users to implementation details if they use any extensions or pass them as kernel parameters (though it already does to some extent).

I guess we could reintroduce KernelParameter for this and let users sort things out for extensions, since extension require FFI hacking anyways. data KernelParameter = KPMem Mem | forall b. Storable b => KPStorable b

Wonder if there is anything else.

ehird commented 12 years ago

I just meant to change e.g. type Foo = Ptr Fooc into data Foo = Foo Context (ForeignPtr Fooc). The declarations will have to change incompatibly to use ForeignPtrs anyway.

Wouldn't it be simplest just to add an instance Storable Mem that does the right thing?

ehird commented 12 years ago

Aha, ForeignPtrs work just fine here, even for deterministic scenarios: finalizeForeignPtr runs the finalisers once and then removes them, so it can be used to deterministically free a ForeignPtr.

ethorsoe commented 12 years ago

Even, if we make memory objects regular pointers so they won't get garbage collected, the context they refer to would, if user loses them.

data Mem = Mem Context (Ptr Memc)

So we would have to protect the Context here, which will get garbage collected, even if Ptr Memc remains. the CommandQueue would still retain the context, but can we rely on it being enough.

OpenGL interoperability might also require low level access to pointer that was one of the other things.

Sorry about delaying, but this is a pretty big design decision.

ehird commented 12 years ago

Well, if ForeignPtr Memc is used, then all that's needed is a withMemPtr :: Mem -> (Ptr Memc -> IO r) -> IO r, analogous to withForeignPtr but that retains the reference to the Context.

No problem with delaying, design decisions like this should be thought through :)

ethorsoe commented 12 years ago

This is still IO, we would expect the Ptr to be passed forward and used even after that withMemPtr (eg. in OpenGL context).

Since destroying the Context cleans everything, I think we could leave it up to the users. The only thing that typically needs more granularity in control is probably allocation of buffers, which user should probably manage anyways. Leaving things as is (using regular Ptr instead of ForeignPtr) would retain most of the required low level control in the library.

Sounds ok?