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

Error-handling could be nicer #1

Open
ehird opened this issue Nov 20, 2011 · 12 comments
Open

Error-handling could be nicer #1

ehird opened this issue Nov 20, 2011 · 12 comments

Comments

@ehird
Copy link
Contributor

ehird commented Nov 20, 2011

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
Copy link
Owner

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
Copy link
Contributor Author

ehird commented Nov 20, 2011

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
Copy link
Owner

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
Copy link
Contributor Author

ehird commented Nov 21, 2011

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
Copy link
Owner

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
Copy link
Contributor Author

ehird commented Nov 21, 2011

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
Copy link
Owner

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
Copy link
Contributor Author

ehird commented Nov 21, 2011

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
Copy link
Contributor Author

ehird commented Nov 22, 2011

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
Copy link
Owner

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
Copy link
Contributor Author

ehird commented Nov 22, 2011

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
Copy link
Owner

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?

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

2 participants