Prevent OpenCL timing memory leak

Fixes #2470

Change-Id: I4917de697bee7df98da0037e9165e52e660f83a0

diff --git a/docs/release-notes/2018/2018.2.rst b/docs/release-notes/2018/2018.2.rst
index b3d96ff574f5c7c9202217a5626e4962cfa8fd4e..933d6d35ab4462d7913a0d0433e058a5ba137bfd 100644 (file)
--- a/docs/release-notes/2018/2018.2.rst
+++ b/docs/release-notes/2018/2018.2.rst
@@ -9,6 +9,13 @@ earlier, which you can find described in the :ref:`release-notes`.
 Fixes where mdrun could behave incorrectly
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+Prevent OpenCL timing memory leak
+"""""""""""""""""""""""""""""""""
+
+When using OpenCL builds and timing, a memory leak would lead to all system memory being used up.
+
+:issue:`2470`
+
 Fixes for ``gmx`` tools
 ^^^^^^^^^^^^^^^^^^^^^^^
 

diff --git a/src/gromacs/gpu_utils/gpuregiontimer_ocl.h b/src/gromacs/gpu_utils/gpuregiontimer_ocl.h
index fc64d04e96e18752607130dcad23cee6df3656ce..81444cafdcee5a10b73927ba191bf2d516b52e67 100644 (file)
--- a/src/gromacs/gpu_utils/gpuregiontimer_ocl.h
+++ b/src/gromacs/gpu_utils/gpuregiontimer_ocl.h
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2016,2017, by the GROMACS development team, led by
+ * Copyright (c) 2016,2017,2018, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -60,6 +60,16 @@ template <> struct GpuTraits<GpuFramework::OpenCL>
 using GpuRegionTimer = GpuRegionTimerWrapper<GpuFramework::OpenCL>;
 
 // cppcheck-suppress noConstructor
+/*! \libinternal \brief
+ * The OpenCL implementation of the GPU code region timing.
+ * With OpenCL, one has to use cl_event handle for each API call that has to be timed, and
+ * accumulate the timing afterwards. As we would like to avoid overhead on API calls,
+ * we only query and accumulate cl_event timing at the end of time steps, not after the API calls.
+ * Thus, this implementation does not reuse a single cl_event for multiple calls, but instead
+ * maintains an array of cl_events to be used within any single code region.
+ * The array size is fixed at a small but sufficiently large value for the number of cl_events
+ * that might contribute to a timer region, currently 10.
+ */
 template <> class GpuRegionTimerImpl<GpuFramework::OpenCL>
 {
     //! Short-hands
@@ -76,9 +86,11 @@ template <> class GpuRegionTimerImpl<GpuFramework::OpenCL>
 
     public:
 
+        /*! \brief Should be called before the region start. */
         inline void openTimingRegion(CommandStream){}
+        /*! \brief Should be called after the region end. */
         inline void closeTimingRegion(CommandStream){}
-
+        /*! \brief Returns the last measured region timespan (in milliseconds) and calls reset(). */
         inline double getLastRangeTime()
         {
             double milliseconds = 0.0;
@@ -101,21 +113,22 @@ template <> class GpuRegionTimerImpl<GpuFramework::OpenCL>
             reset();
             return milliseconds;
         }
-
+        /*! \brief Resets the internal state, releasing the used cl_events. */
         inline void reset()
         {
             for (size_t i = 0; i < currentEvent_; i++)
             {
                 if (events_[i]) // This conditional is ugly, but is required to make some tests (e.g. empty domain) pass
                 {
-                    GMX_ASSERT(CL_SUCCESS == clReleaseEvent(events_[i]), "OpenCL event release failure");
+                    cl_int gmx_unused cl_error = clReleaseEvent(events_[i]);
+                    GMX_ASSERT(CL_SUCCESS == cl_error, "OpenCL event release failure");
                 }
             }
             currentEvent_ = 0;
             // As long as we're doing nullptr checks, we might want to be extra cautious.
             events_.fill(nullptr);
         }
-
+        /*! \brief Provides next unused cl_event for OpenCL API consumption. */
         inline CommandEvent *fetchNextEvent()
         {
             GMX_ASSERT(currentEvent_ < events_.size(), "Increase c_maxEventNumber_ if needed");