Khronos Public Bugzilla
Bug 427 - C++ bindings may leak events
C++ bindings may leak events
Status: RESOLVED FIXED
Product: OpenCL
Classification: Unclassified
Component: Header Files
1.1
All All
: P3 normal
: ---
Assigned To: Benedict Gaster
OpenCL Working Group
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-04 18:49 PST by David Garcia
Modified: 2011-02-07 14:26 PST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Garcia 2011-02-04 18:49:41 PST
User fedechicco from the forums has found a bug in the C++ bindings and posted it in http://www.khronos.org/message_boards/viewtopic.php?f=28&t=3419&start=0

Copied&pasted from his message:

<<I see 2 problems in [the way CommandQueue::enqueueNDRangeKernel() is implemented]:
1. When debugging I know for sure that this doesn't release the previous cl_event stored in the Event wrapper. If you put this code in a loop it'll fill up your RAM. On the other hand there is no easy way to release the previous Event easily, since the Wrapper::release method is protected, the fastest way is to assign a null Event.
2. Why are we casting an Event pointer to a cl_event pointer? I don't see any operator& overloading in the wrapper that permits us to do such a thing. I'm no C++ guru, if somebody can explain me that i'd be glad to understand. Is it because the cl_event is the first and only field in the class (therefore it starts exactly where the Event object starts)?>>

Item #1 looks pretty serious IMO.
Comment 1 Brian Cole 2011-02-04 19:25:56 PST
(In reply to comment #0)
> User fedechicco from the forums has found a bug in the C++ bindings and posted
> it in http://www.khronos.org/message_boards/viewtopic.php?f=28&t=3419&start=0
> 
> Copied&pasted from his message:
> 
> <<I see 2 problems in [the way CommandQueue::enqueueNDRangeKernel() is
> implemented]:
> 1. When debugging I know for sure that this doesn't release the previous
> cl_event stored in the Event wrapper. If you put this code in a loop it'll fill
> up your RAM. On the other hand there is no easy way to release the previous
> Event easily, since the Wrapper::release method is protected, the fastest way
> is to assign a null Event.
> 2. Why are we casting an Event pointer to a cl_event pointer? I don't see any
> operator& overloading in the wrapper that permits us to do such a thing. I'm no
> C++ guru, if somebody can explain me that i'd be glad to understand. Is it
> because the cl_event is the first and only field in the class (therefore it
> starts exactly where the Event object starts)?>>
> 
> Item #1 looks pretty serious IMO.

Responded on the forum. Need more information to chase the memory leak as I can't reproduce it.
Comment 2 David Garcia 2011-02-04 19:54:32 PST
> Responded on the forum. Need more information to chase the memory leak as I
> can't reproduce it.

I just looked at cl.hpp's source code and I don't see how could this possibly _not_ leak. The event is overwritten blindly without decreasing its refcount first as far as I can tell.

Let's look at this method:


    cl_int enqueueNDRangeKernel(
        const Kernel& kernel,
        const NDRange& offset,
        const NDRange& global,
        const NDRange& local,
        const VECTOR_CLASS<Event>* events = NULL,
        Event* event = NULL) const
    {
        return detail::errHandler(
            ::clEnqueueNDRangeKernel(
                object_, kernel(), (cl_uint) global.dimensions(),
                offset.dimensions() != 0 ? (const ::size_t*) offset : NULL,
                (const ::size_t*) global,
                local.dimensions() != 0 ? (const ::size_t*) local : NULL,
                (events != NULL) ? (cl_uint) events->size() : 0,
                (events != NULL && events->size() > 0) ? (cl_event*) &events->front() : NULL,
                (cl_event*) event),
            __ENQUEUE_NDRANGE_KERNEL_ERR);
    }


What happens if "event" already contained an event object returned by a previous enqueue call? It's never released.

Do you follow the argument? I could be missing something.
Comment 3 Brian Cole 2011-02-05 10:57:46 PST
> What happens if "event" already contained an event object returned by a
> previous enqueue call? It's never released.

Ah, that's the key part I didn't get before, I see it now. I've responded to the original poster on the forum with a work around as well. 

The fix for the bindings is going to be a little trickier. Adding the following to any method that takes a cl::Event is sufficient enough to prevent memory leaks: 

        if (event && (*event)() != NULL)
            ReferenceHandler<cl_event>::release(object_);

However, it will have unintended consequences whenever an error is thrown. The following will likely crash with the addition of the above fix:

      cl::Event event;
      try
      {
        for (unsigned int i = 0; i < 10000000; i++)
        {
          queue.enqueueNDRangeKernel(kernel, 
                                     cl::NullRange, 
                                     cl::NDRange(1000), 
                                     cl::NullRange,
                                     NULL,
                                     &event);
          event.wait();
        }
      }
      catch (cl::Error err) 
      {
        if (event() != NULL)
          event.getInfo<CL_COMMAND_EXECUTION_STATUS>();
      }

Getting around this problem will take a bit more thinking.
Comment 4 Brian Cole 2011-02-05 11:04:29 PST
(In reply to comment #3)
> Getting around this problem will take a bit more thinking.

Not long after I hit submit I figured out the appropriate fix, just use the workaround I gave to the OP in the forum. The following should fix the problem and be exception/error safe. 

    cl_int enqueueNDRangeKernel(
        const Kernel& kernel,
        const NDRange& offset,
        const NDRange& global,
        const NDRange& local,
        const VECTOR_CLASS<Event>* events = NULL,
        Event* event = NULL) const
    {
        cl::Event tmp;
        cl_int err = detail::errHandler(
            ::clEnqueueNDRangeKernel(
                object_, kernel(), (cl_uint) global.dimensions(),
                offset.dimensions() != 0 ? (const ::size_t*) offset : NULL,
                (const ::size_t*) global,
                local.dimensions() != 0 ? (const ::size_t*) local : NULL,
                (events != NULL) ? (cl_uint) events->size() : 0,
                (events != NULL && events->size() > 0) ? (cl_event*) &events->front() : NULL,
                (cl_event*) event),
            __ENQUEUE_NDRANGE_KERNEL_ERR);
        if (event != NULL && err == CL_SUCCESS)
            *event = tmp;

        return err;
    }
Comment 5 Brian Cole 2011-02-07 14:26:01 PST
Fixed revision 13822.