[Parallel_libs-dev] StreamExecutor design questions

[Jason Henline]

However, this makes it even more unclear (to me) what the benefit is of
having the Platform{Kernel,Stream}Handle wrapper classes at all. Why not
just have createKernel() and createStream() return a raw void* which is
then stored directly in the Kernel and Stream classes? The raw pointers
could then be passed to PlatformDevice as needed, including to the new
destroy*() methods, and the platform implementor no longer has to create a
couple of subclasses to hold those raw pointers with an extra level of
indirection.

You're exactly right. I forgot that the void* handles could be stored
directly in the Stream and Kernel objects. I had no other plans for the
Platform*Handle classes in the future, so I'll plan to get rid of them,
just as you suggested. I'll plan to do that early next week.

A ‘shutdown everything in the platform’ function that is called
automatically as Justin proposed feels like it would be a good solution.

I'm fine with the idea of a "platform kill switch" that can leave dangling
pointers to Device, etc. If we make one, I'll just try to make big, scary
warnings in the documentation that tell folks to make sure they don't use
any of the dangling platform handles after killing the platform.

On Fri, Sep 2, 2016 at 3:24 PM James Price <J.Price at bristol.ac.uk> wrote:

> Hi Jason,
>
> I agree this is inconsistent with the way device memory is currently
> handled using a void* handle, and I'm glad you pointed that out. The device
> memory object currently handles its own cleanup by making a call to
> PlatformDevice::freeDeviceMemory, so we could create consistency by
> following this model. PlatformKernelHandle and PlatformStreamHandle would
> each store a void* opaque handle to the platform-specific implementation
> and new functions would be added to the PlatformDevice interface to
> destroyKernel(PlatformKernelHandle*) and
> destroyStream(PlatformStreamHandle*). Then the destructors for PlatformKernelHandle
> and PlatformStreamHandle would call the corresponding Device destroy
> functions just like GlobalDeviceMemoryBase calls the freeDeviceMemory
> function in its destructor.
>
> Now that I've laid it out this way, I think you're right that that design
> would be better. It would be consistent and it would be easier to implement
> a platform. Does that sound like the right design to you, as well? If so,
> I'll write up some patches to make those changes.
>
>
> That’s halfway to what I was thinking. If PlatformDevice is the thing that
> creates the objects, it makes sense for it to also be in charge of
> destroying them with the destroyKernel() and destroyStream() methods you
> propose, and this is indeed more consistent with the device memory handling.
>
> However, this makes it even more unclear (to me) what the benefit is of
> having the Platform{Kernel,Stream}Handle wrapper classes at all. Why not
> just have createKernel() and createStream() return a raw void* which is
> then stored directly in the Kernel and Stream classes? The raw pointers
> could then be passed to PlatformDevice as needed, including to the new
> destroy*() methods, and the platform implementor no longer has to create a
> couple of subclasses to hold those raw pointers with an extra level of
> indirection.
>
> If, however, the Platform*Handle classes are intended to also serve
> another purpose in the future, then it probably makes sense to keep them.
> I’m no C++ design guru, so I’m also happy to accept if the wrapper class
> approach is simply better practice or safer in some way than raw pointers.
>
>
> It also seems that there is no way for a user to fully release the
> resources used by a SE device, since the end-user doesn’t own the Device
> pointer. Maybe I’m worrying about nothing here, but I wonder whether there
> may be some devices that become ‘unhappy' if you don’t release their
> contexts/default queues etc before the application exits.
>
> I think you are right to say there is no way for a user to free an SE
> device. I don't know if anyone will ever need to free those resources, but
> I think we will want to add a method to the platform class to free the
> resources for a device with a given ordinal. Of course that might leave
> dangling references to a device that has been freed, so we might want to
> have the platform pass out std::shared_ptr to Device instead of raw
> Device*. In any case, I think we have a few good options here that won't be
> hard to change later. What do you think?
>
>
> A ‘shutdown everything in the platform’ function that is called
> automatically as Justin proposed feels like it would be a good solution.
>
>
> James
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/parallel_libs-dev/attachments/20160903/0548cc2b/attachment.html>