Merge pull request #6698 from ghalliday/issue12642

HPCC-12642 Add link/Release implementations that are not thread safe

Reviewed-By: Richard Chapman <rchapman@hpccsystems.com>

ecl/hql/hqlexpr.cpp

@@ -4421,7 +4421,9 @@ void CHqlExpression::beforeDispose()
 #endif
     if (infoFlags & HEFobserved)
     {
+#ifdef HQLEXPR_MULTI_THREADED
         CriticalBlock block(*exprCacheCS);
+#endif
         if (infoFlags & HEFobserved)
             exprCache->removeExact(this);
     }
@@ -4472,7 +4474,9 @@ IHqlExpression * CHqlExpression::commonUpExpression()
 
     IHqlExpression * match;
     {
+#ifdef HQLEXPR_MULTI_THREADED
         CriticalBlock block(*exprCacheCS);
+#endif
         match = exprCache->addOrFind(*this);
 #ifndef GATHER_COMMON_STATS
         if (match == this)

ecl/hql/hqlexpr.ipp

@@ -17,10 +17,16 @@
 #ifndef HQLEXPR_IPP_INCL
 #define HQLEXPR_IPP_INCL
 
+//The following needs to be defined to allow multi-threaded access to the expressions
+//Required inside esp and other areas for parsing record definitions etc.
+#define HQLEXPR_MULTI_THREADED
+
 #define NUM_PARALLEL_TRANSFORMS 1
 //I'm not sure if the following is needed or not - I'm slight concerned that remote scopes (e.g.,, plugins)
 //may be accessed in parallel from multiple threads, causing potential conflicts
+#ifdef HQLEXPR_MULTI_THREADED
 #define THREAD_SAFE_SYMBOLS
+#endif
 
 //The following flag is to allow tracing when expressions are created, linked, released, destroyed
 //It allocates a unique id to each expression that is created, then add the unique ids into the
@@ -145,11 +151,17 @@ protected:
     UsedExpressionHashTable newScopeTables;
 };
 
-class HQL_API CHqlExpression : public CInterfaceOf<IHqlExpression>
+#ifdef HQLEXPR_MULTI_THREADED
+typedef CInterfaceOf<IHqlExpression> LinkedBaseIHqlExpression;
+#else
+typedef CSingleThreadInterfaceOf<IHqlExpression> LinkedBaseIHqlExpression;
+#endif
+
+class HQL_API CHqlExpression : public LinkedBaseIHqlExpression
 {
 public:
     friend class CHqlExprMeta;
-    typedef CInterfaceOf<IHqlExpression> Parent;
+    typedef LinkedBaseIHqlExpression Parent;
 
 #ifdef USE_TBB
     void *operator new(size32_t size) { return scalable_malloc(size); }

ecl/hql/hqltrans.ipp

@@ -300,7 +300,11 @@ enum
     NTFselectorOriginal     = 0x0002,
 };
 
-class HQL_API ANewTransformInfo : implements CInterfaceOf<IInterface> 
+
+//Unless we start using tbb or something similar to parallelise transformations
+//[which would be tricky because queryTransformExtra() assumes single threaded access]
+//all Link() and Release() calls must be from the same thread => this can be lightweight
+class HQL_API ANewTransformInfo : implements CSingleThreadSimpleInterfaceOf<IInterface>
 {
 public:
     ANewTransformInfo(IHqlExpression * _original);

ecl/hqlcpp/hqlstmt.ipp

@@ -24,7 +24,8 @@
 class HqlStmts;
 class PeepHoleOptimizer;
 
-class HqlStmt : public CInterfaceOf<IHqlStmt>
+//All generation is single threaded.  Even if multiple wus were generated at once the statements aren't shared
+class HqlStmt : public CSingleThreadSimpleInterfaceOf<IHqlStmt>
 {
 public:
     HqlStmt(StmtKind _kind, HqlStmts * _container);

system/jlib/jiface.hpp

@@ -119,15 +119,43 @@ public:
     }
 };
 
+//---------------------------------------------------------------------------------------------------------------------
+
+// An implementation of IInterface for objects that are never shared.  Only likely to be used in a few situations.
+// It still allows use with IArrayOf and Owned classes etc.
+template <class INTERFACE>
+class CUnsharedInterfaceOf : public INTERFACE
+{
+public:
+    inline virtual ~CUnsharedInterfaceOf() {}
+
+    inline CUnsharedInterfaceOf()         { }
+    inline bool IsShared(void) const    { return false; }
+    inline int getLinkCount(void) const { return 1; }
+
+    inline void Link() const            { assert(!"Unshared::Link"); }
+
+    inline bool Release(void) const
+    {
+        delete this;
+        return true;
+    }
+};
+
 //- Template variants -------------------------------------------------------------------------------------------------
 
 template <class INTERFACE>
 class CInterfaceOf;
 
 template <class INTERFACE>
+class CReusableInterfaceOf;
+
+// A thread safe basic implementation of IInterface that destroys the object when last Release() occurs.
+template <class INTERFACE>
 class CSimpleInterfaceOf : public INTERFACE
 {
     friend class CInterfaceOf<INTERFACE>;    // want to keep xxcount private outside this pair of classes
+    friend class CReusableInterfaceOf<INTERFACE>;
 public:
     inline virtual ~CSimpleInterfaceOf() {}
 
@@ -152,7 +180,7 @@ private:
 };
 
 // A more general implementation of IInterface that includes a virtual function beforeDispose().
-// beforeDispose() allows an a fully constructed object to be cleaned up (which means that virtual
+// beforeDispose() allows a fully constructed object to be cleaned up (which means that virtual
 // function calls still work (unlike a virtual destructor).
 // It makes it possible to implement a cache which doesn't link count the object, but is cleared when
 // the object is disposed without a critical section in Release().  (See pattern details below).
@@ -181,6 +209,95 @@ public:
     }
 };
 
+// An extension of CInterfaceOf that allows objects that are being destroyed to be added to a free list instead.
+// Before disposing addToFree() is called.  That function can link the object and return true which will prevent
+// the object being destroyed.
+template <class INTERFACE>
+class CReusableInterfaceOf : public CInterfaceOf<INTERFACE>
+{
+public:
+    //If this function returns true the object is added to a free list - if so it must be linked in the process.
+    //It *must not* link and release the object if it returns false.
+    virtual bool addToFreeList() const { return false; }
+
+    inline bool Release(void) const
+    {
+        if (atomic_dec_and_test(&this->xxcount))
+        {
+            if (addToFreeList())
+            {
+                //It could have been added, reused and disposed between the call and this return, so must return immediately
+                //and in particular cannot check/modify link count
+                return true;
+            }
+
+            //NOTE: This class cannot be used for caches which don't link the pointers => use atomic_set instead of cas
+            atomic_set(&this->xxcount, DEAD_PSEUDO_COUNT);
+            const_cast<CReusableInterfaceOf<INTERFACE> *>(this)->beforeDispose();
+            delete this;
+            return true;
+        }
+        return false;
+    }
+};
+
+//---------------------------------------------------------------------------------------------------------------------
+
+template <class INTERFACE>
+class CSingleThreadInterfaceOf;
+
+//An equivalent to CSimpleInterfaceOf that is not thread safe - but avoids the cost of atomic operations.
+template <class INTERFACE>
+class CSingleThreadSimpleInterfaceOf : public INTERFACE
+{
+    friend class CSingleThreadInterfaceOf<INTERFACE>;    // want to keep xxcount private outside this pair of classes
+public:
+    inline virtual ~CSingleThreadSimpleInterfaceOf() {}
+
+    inline CSingleThreadSimpleInterfaceOf() { xxcount = 1; }
+    inline bool IsShared(void) const    { return xxcount > 1; }
+    inline int getLinkCount(void) const { return xxcount; }
+
+    inline void Link() const            { ++xxcount; }
+
+    inline bool Release(void) const
+    {
+        if (--xxcount == 0)
+        {
+            delete this;
+            return true;
+        }
+        return false;
+    }
+
+private:
+    mutable unsigned xxcount;
+};
+
+//An equivalent to CInterfaceOf that is not thread safe - but avoids the cost of atomic operations.
+template <class INTERFACE>
+class CSingleThreadInterfaceOf : public CSingleThreadSimpleInterfaceOf<INTERFACE>
+{
+public:
+    virtual void beforeDispose() {}
+
+    inline bool isAlive() const         { return this->xxcount < DEAD_PSEUDO_COUNT; }       //only safe if Link() is called first
+
+    inline bool Release(void) const
+    {
+        if (--this->xxcount == 0)
+        {
+            //Because beforeDispose could cause this object to be linked/released or call isAlive(), xxcount is set
+            //to a a high mid-point positive number to avoid poss. of releasing again.
+            this->xxcount = DEAD_PSEUDO_COUNT;
+            const_cast<CSingleThreadInterfaceOf<INTERFACE> *>(this)->beforeDispose();
+            delete this;
+            return true;
+        }
+        return false;
+    }
+};
+
 
 /***
 A pattern for implementing a non-linking cached to a shared object.  Hash table variants are a slightly more complex variant.