HPCC-30156 Instrument CriticalSection

Rename ICriticalBlock
Clean up some unwanted changes
Add some comments

Signed-off-by: Richard Chapman <[email protected]>

CMakeLists.txt

@@ -85,8 +85,6 @@ unset ( ENV{CC} )
 unset ( ENV{CXX} )
 
 set(TOP_LEVEL_PROJECT ON)
-unset(PROFILING)
-set(PROFILING ON)
 if(NOT CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)
     set(TOP_LEVEL_PROJECT OFF)
 endif(NOT CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)

cmake_modules/commonSetup.cmake

@@ -257,7 +257,7 @@ IF ("${COMMONSETUP_DONE}" STREQUAL "")
       endif()
     endif()
   endif ()
-  if (CMAKE_COMPILER_IS_GNUCXX OR CMAKE_COMPILER_IS_CLANGXX)
+  if (PROFILING AND (CMAKE_COMPILER_IS_GNUCXX OR CMAKE_COMPILER_IS_CLANGXX))
     add_definitions (-fno-omit-frame-pointer)
     add_definitions (-D_PROFILING)
   endif ()

cmake_modules/options.cmake

@@ -68,7 +68,7 @@ option(SKIP_ECLWATCH "Skip building ECL Watch" OFF)
 option(USE_ADDRESS_SANITIZER "Use address sanitizer to spot leaks" OFF)
 option(INSTALL_VCPKG_CATALOG "Install vcpkg-catalog.txt" ON)
 option(PORTALURL "Set url to hpccsystems portal download page")
-option(PROFILING "Set to true if planning to profile so stacks are informative" ON)
+option(PROFILING "Set to true if planning to profile so stacks are informative" OFF)
 
 if ( NOT PORTALURL )
   set( PORTALURL "http://hpccsystems.com/download" )

common/thorhelper/roxiehelper.cpp

@@ -1795,7 +1795,7 @@ bool CSafeSocket::sendHeartBeat(const IContextLogger &logctx)
 class HttpResponseHandler
 {
 private:
-    ICriticalBlock c; // should not be anyone writing but better to be safe
+    CCriticalBlock c; // should not be anyone writing but better to be safe
     StringBuffer header;
     StringBuffer content;
     ISocket *sock = nullptr;

common/workunit/wujobq.cpp

@@ -972,13 +972,13 @@ class CJobQueue: public CJobQueueBase, implements IJobQueue
         }
     }
 
-    class Cconnlockblock: public ICriticalBlock
+    class Cconnlockblock: public CCriticalBlock
     {
         CJobQueue *parent;
         bool rollback;
     public:
         Cconnlockblock(CJobQueue *_parent,bool exclusive)
-            : ICriticalBlock(_parent->crit)
+            : CCriticalBlock(_parent->crit)
         {
             parent = _parent;
             parent->connlock(exclusive);

ecl/hql/hqlexpr.ipp

@@ -65,7 +65,7 @@ public:
 };
 
 #ifdef HQLEXPR_MULTI_THREADED
-typedef ICriticalBlock HqlCriticalBlock;
+typedef CCriticalBlock HqlCriticalBlock;
 #else
 typedef NullCriticalBlock HqlCriticalBlock;
 #endif

plugins/Rembed/Rembed.cpp

@@ -1437,7 +1437,7 @@ class REmbedFunctionContext: public CInterfaceOf<IEmbedFunctionContext>
     RInside &R;
     RInside::Proxy result;
     StringAttr func;
-    ICriticalBlock block;
+    CCriticalBlock block;
     Owned<REnvironment> env;
 };
 

roxie/roxiemem/roxiemem.cpp

@@ -3465,7 +3465,7 @@ class HeapCompactState
         }
     }
 public:
-    ICriticalBlock lock;
+    CCriticalBlock lock;
     unsigned numPagesEmptied;
     CHeap * heap;
     Heaplet * next; // which heaplet to try to compact into next.  Not so good if > 1 heaps in use.
@@ -3516,7 +3516,7 @@ class NewHeapCompactState
     bool processHeap(unsigned low, unsigned max);
 
 protected:
-    ICriticalBlock lock;
+    CCriticalBlock lock;
     PointerArrayOf<ChunkedHeaplet> heaplets;
 };
 

system/jlib/jmutex.cpp

@@ -136,7 +136,7 @@ MODULE_EXIT()
     }
 }
 
-thread_local CriticalBlockInstrumentation *__cbinst = nullptr;
+thread_local CriticalBlockInstrumentation *__cbinst = nullptr;   // Used as an extra parameter to CriticalBlock macro, so we don't have to change every call
 
 std::size_t CriticalBlockInstrumentation::StackHash::operator()(const Stack& k) const
 {

system/jlib/jmutex.hpp

@@ -45,7 +45,8 @@ extern jlib_decl void spinUntilReady(std::atomic_uint &value);
 #endif
 
 #if defined(_PROFILING)
-#define USE_INSTRUMENTED_CRITSECS
+// Instrumented critsecs work best if PROFILING Cmake option is in use
+//#define USE_INSTRUMENTED_CRITSECS
 #endif
 
 #ifdef SPINLOCK_USE_MUTEX
@@ -685,21 +686,29 @@ template<typename T> class CLeavableCriticalBlockOf
 typedef CLeavableCriticalBlockOf<UninstrumentedCriticalSection> UninstrumentedLeavableCriticalBlock;
 typedef CLeavableCriticalBlockOf<InstrumentedCriticalSection> InstrumentedLeavableCriticalBlock;
 
+// The CriticalBlock instrumentation works by replacing all references to the normal CriticalBlock constructor with a macro
+// that instantiates a new (local static) CriticalBlockInstrumentation object, and passes it in to the constructor via a thread-local variable.
+// It's done this way to avoid any overhead or code changes in the uninstrumented case, at the expense of some complexity and size overhead in the
+// instrumented case.
+//
+// Because CriticalBlock may be a macro in the instrumented case, any place that a parameter or member variable of type CriticalBlock is needed
+// must ust CCriticalBlock rather than CriticalBlock to avoid the macro expansion.
+
 #ifdef USE_INSTRUMENTED_CRITSECS
 #define __glue(a,b) a ## b
 #define glue(a,b) __glue(a,b)
 
 extern thread_local CriticalBlockInstrumentation* __cbinst;
 
 typedef InstrumentedCriticalSection CriticalSection;
-typedef InstrumentedCriticalBlock ICriticalBlock;
-#define CriticalBlock static CriticalBlockInstrumentation *glue(instrumenter,__LINE__) = new CriticalBlockInstrumentation(__FILE__, __func__, __LINE__); __cbinst = glue(instrumenter,__LINE__); ICriticalBlock
+typedef InstrumentedCriticalBlock CCriticalBlock;
+#define CriticalBlock static CriticalBlockInstrumentation *glue(instrumenter,__LINE__) = new CriticalBlockInstrumentation(__FILE__, __func__, __LINE__); __cbinst = glue(instrumenter,__LINE__); CCriticalBlock
 typedef InstrumentedCriticalUnblock CriticalUnblock;
 typedef InstrumentedLeavableCriticalBlock CLeavableCriticalBlock;
 #else
 typedef UninstrumentedCriticalSection CriticalSection;
-typedef UninstrumentedCriticalBlock ICriticalBlock;
-typedef ICriticalBlock CriticalBlock;
+typedef UninstrumentedCriticalBlock CCriticalBlock;
+typedef CCriticalBlock CriticalBlock;   // Use in place of CriticalBlock when declaring members/parameters, to avoid macro expansion
 typedef UninstrumentedCriticalUnblock CriticalUnblock;
 typedef UninstrumentedLeavableCriticalBlock CLeavableCriticalBlock;
 #endif

testing/unittests/jlibtests.cpp

@@ -2463,10 +2463,10 @@ class AtomicTimingTest : public CppUnit::TestFixture
     const unsigned numCores = std::max(getAffinityCpus(), 16U);
     void runAllTests()
     {
-        DO_TEST(CriticalSection, ICriticalBlock, unsigned __int64, 1, 1);
-        DO_TEST(CriticalSection, ICriticalBlock, unsigned __int64, 2, 1);
-        DO_TEST(CriticalSection, ICriticalBlock, unsigned __int64, 5, 1);
-        DO_TEST(CriticalSection, ICriticalBlock, unsigned __int64, 1, 2);
+        DO_TEST(CriticalSection, CCriticalBlock, unsigned __int64, 1, 1);
+        DO_TEST(CriticalSection, CCriticalBlock, unsigned __int64, 2, 1);
+        DO_TEST(CriticalSection, CCriticalBlock, unsigned __int64, 5, 1);
+        DO_TEST(CriticalSection, CCriticalBlock, unsigned __int64, 1, 2);
         DO_TEST(SpinLock, SpinBlock, unsigned __int64, 1, 1);
         DO_TEST(SpinLock, SpinBlock, unsigned __int64, 2, 1);
         DO_TEST(SpinLock, SpinBlock, unsigned __int64, 5, 1);