[Parallel_libs-dev] StreamExecutor design questions

[Jason Henline]

I decided I couldn't wait until next week to fix this unnecessary wrapper
class stuff. A patch is now up at https://reviews.llvm.org/D24213.

On Fri, Sep 2, 2016 at 5:38 PM Jason Henline <jhen at google.com> wrote:

> 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/5b67d22a/attachment-0001.html>