<div dir="ltr">I decided I couldn't wait until next week to fix this unnecessary wrapper class stuff. A patch is now up at <a href="https://reviews.llvm.org/D24213">https://reviews.llvm.org/D24213</a>.</div><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 2, 2016 at 5:38 PM Jason Henline <<a href="mailto:jhen@google.com">jhen@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif">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.</span><br><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif"><br></span></div></div><div dir="ltr"><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif">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.</span></div></div><div dir="ltr"><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif"><br></span></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif">A ‘shutdown everything in the platform’ function that is called automatically as Justin proposed feels like it would be a good solution.</span><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif"><br></span></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif"><br></span></div></div><div dir="ltr"><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif">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.</span></div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 2, 2016 at 3:24 PM James Price <<a href="mailto:J.Price@bristol.ac.uk" target="_blank">J.Price@bristol.ac.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Jason,<div><br></div><div><div></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div>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. <span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif;line-height:1.5">PlatformKernelHandle and </span><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif;line-height:1.5">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 </span><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif">PlatformKernelHandle and PlatformStreamHandle would call the corresponding Device destroy functions just like GlobalDeviceMemoryBase calls the freeDeviceMemory function in its destructor.</font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><br></font></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif">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.</font></div></div></div></blockquote><div><br></div></div></div></div><div style="word-wrap:break-word"><div><div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div></div></div></div><div style="word-wrap:break-word"><div><div><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif">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.</span><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif"><br></font></div><div><span style="color:rgb(33,33,33);font-family:"helvetica neue",helvetica,arial,sans-serif"><br></span></div><div><font color="#212121" face="helvetica neue, helvetica, arial, sans-serif">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?</font></div></div></div></blockquote><div><br></div></div></div></div><div style="word-wrap:break-word"><div><div><div>A ‘shutdown everything in the platform’ function that is called automatically as Justin proposed feels like it would be a good solution.</div></div></div></div><div style="word-wrap:break-word"><div><div><div><br></div><div><br></div><div>James</div><blockquote type="cite"><div>
</div></blockquote></div></div></div></blockquote></div></blockquote></div>