HPCC-17625 JPTree memory usage and contention issues

Store short strings in place of the pointer, for names, attribute keys, and
values.

Reduces memory usage by another 50% or so on my tests...

Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>

system/jlib/jptree.cpp

@@ -1735,11 +1735,11 @@ IAttributeIterator *PTree::getAttributes(bool sorted) const
         virtual bool isValid() override { return cur ? true : false; }
         virtual const char *queryName() const override
         {
-            return cur->key->get();
+            return cur->key.get();
         }
         virtual const char *queryValue() const override
         {
-            return cur->value->get();
+            return cur->value.get();
         }
         virtual StringBuffer &getValue(StringBuffer &out) override
         {
@@ -1761,7 +1761,7 @@ IAttributeIterator *PTree::getAttributes(bool sorted) const
 
         static int compareAttrs(AttrValue * const *ll, AttrValue * const *rr)
         {
-            return stricmp((*ll)->key->get(), (*rr)->key->get());
+            return stricmp((*ll)->key.get(), (*rr)->key.get());
         };
 
         CSortedAttributeIterator(const PTree *_parent) : cur(NULL), iter(NULL), parent(_parent)
@@ -1804,12 +1804,12 @@ IAttributeIterator *PTree::getAttributes(bool sorted) const
         virtual const char *queryName() const override
         {
             assertex(cur);
-            return cur->key->get();
+            return cur->key.get();
         }
         virtual const char *queryValue() const override
         {
             assertex(cur);
-            return cur->value->get();
+            return cur->value.get();
         }
         virtual StringBuffer &getValue(StringBuffer &out) override
         {
@@ -2822,7 +2822,7 @@ AttrValue *PTree::findAttribute(const char *key) const
     {
         while (a-- != attrs)
         {
-            if (strieq(a->key->get(), key))
+            if (strieq(a->key.get(), key))
                 return a;
         }
     }
@@ -2830,7 +2830,7 @@ AttrValue *PTree::findAttribute(const char *key) const
     {
         while (a-- != attrs)
         {
-            if (streq(a->key->get(), key))
+            if (streq(a->key.get(), key))
                 return a;
         }
     }
@@ -2841,7 +2841,7 @@ const char *PTree::getAttributeValue(const char *key) const
 {
     AttrValue *e = findAttribute(key);
     if (e)
-        return e->value->get();
+        return e->value.get();
     return nullptr;
 }
 
@@ -2877,59 +2877,32 @@ LocalPTree::LocalPTree(const char *_name, byte _flags, IPTArrayValue *_value, Ch
 
 LocalPTree::~LocalPTree()
 {
-    if (name_ptr)
-    {
-#ifdef TRACE_NAME_SIZE
-        totsize -= strlen(name_ptr)+1;
-#endif
-        free((char *) name_ptr);
-    }
+    name.destroy();
     if (!attrs)
         return;
     AttrValue *a = attrs+numAttrs;
     while (a--!=attrs)
     {
-        AttrStr::destroy(a->key);
-        AttrStr::destroy(a->value);
+        a->key.destroy();
+        a->value.destroy();
     }
     free(attrs);
 }
 
 const char *LocalPTree::queryName() const
 {
-    return name_ptr;
+    return name.get();
 }
 
-void LocalPTree::setName(const char *name)
+void LocalPTree::setName(const char *_name)
 {
-    if (name==name_ptr)
+    if (_name==name.get())
         return;
-    const char *oname = name_ptr;
-    // Don't free until after we copy - they could overlap
-    if (!name)
-        name_ptr = nullptr;
-    else
-    {
-#ifdef TRACE_ALL_NAME
-        DBGLOG("LocalPTree::setName %s", name);
-#endif
-#ifdef TRACE_NAME_SIZE
-        totsize += strlen(name)+1;
-        if (totsize > maxsize)
-        {
-            maxsize.store(totsize);
-            DBGLOG("Name total size now %" I64F "d", maxsize.load());
-        }
-#endif
-        name_ptr = strdup(name);
-    }
+    AttrStr *oname = name.getPtr();  // Don't free until after we copy - they could overlap
+    if (!name.set(_name))
+        name.setPtr(AttrStr::create(_name));
     if (oname)
-    {
-#ifdef TRACE_NAME_SIZE
-        totsize -= strlen(oname)+1;
-#endif
-        free((char *) oname);
-    }
+        AttrStr::destroy(oname);
 }
 
 bool LocalPTree::removeAttribute(const char *key)
@@ -2939,8 +2912,8 @@ bool LocalPTree::removeAttribute(const char *key)
         return false;
     numAttrs--;
     unsigned pos = del-attrs;
-    AttrStr::destroy(del->key);
-    AttrStr::destroy(del->value);
+    del->key.destroy();
+    del->value.destroy();
     memmove(attrs+pos, attrs+pos+1, (numAttrs-pos)*sizeof(AttrValue));
     return true;
 }
@@ -2954,39 +2927,36 @@ void LocalPTree::setAttribute(const char *key, const char *val)
     if (!val)
         val = "";  // cannot have NULL value
     AttrValue *v = findAttribute(key);
+    AttrStr *goer = nullptr;
     if (v)
     {
-        if (streq(v->value->get(), val))
+        if (streq(v->value.get(), val))
             return;
-        AttrStr::destroy(v->value);
-        v->value = AttrStr::create(val);
+        goer = v->value.getPtr();
     }
     else
     {
         attrs = (AttrValue *)realloc(attrs, (numAttrs+1)*sizeof(AttrValue));
-        attrs[numAttrs].key = isnocase() ? AttrStr::createNC(key) : AttrStr::create(key);
-        attrs[numAttrs].value = AttrStr::create(val);
-        numAttrs++;
+        v = &attrs[numAttrs++];
+        if (!v->key.set(key))
+            v->key.setPtr(isnocase() ? AttrStr::createNC(key) : AttrStr::create(key));
     }
+    if (!v->value.set(val))
+        v->value.setPtr(AttrStr::create(val));
+    if (goer)
+        AttrStr::destroy(goer);
 }
 
-#ifdef TRACE_ATTRSTR_SIZE
+#ifdef TRACE_STRING_SIZE
 std::atomic<__int64> AttrStr::totsize { 0 };
 std::atomic<__int64> AttrStr::maxsize { 0 };
 #endif
 
-#ifdef TRACE_ATTRATOM_SIZE
+#ifdef TRACE_ATOM_SIZE
 std::atomic<__int64> AttrStrAtom::totsize { 0 };
 std::atomic<__int64> AttrStrAtom::maxsize { 0 };
 #endif
 
-#ifdef TRACE_NAME_SIZE
-std::atomic<__int64> CAtomPTree::totsize { 0 };
-std::atomic<__int64> CAtomPTree::maxsize { 0 };
-std::atomic<__int64> LocalPTree::totsize { 0 };
-std::atomic<__int64> LocalPTree::maxsize { 0 };
-#endif
-
 ///////////////////
 
 CAtomPTree::CAtomPTree(const char *_name, byte _flags, IPTArrayValue *_value, ChildMap *_children) : PTree(_flags, _value, _children)
@@ -2998,14 +2968,17 @@ CAtomPTree::CAtomPTree(const char *_name, byte _flags, IPTArrayValue *_value, Ch
 CAtomPTree::~CAtomPTree()
 {
     bool nc = isnocase();
+    HashKeyElement *name_ptr = name.getPtr();
     if (name_ptr)
     {
         AtomRefTable *kT = nc?keyTableNC:keyTable;
-#ifdef TRACE_NAME_SIZE
-        if (name_ptr->queryReferences()==1)  // Not strictly accurate but good enough
-            totsize -= sizeof(HashKeyElement)+strlen(name_ptr->get())+1;
-#endif
+#ifdef TRACE_ATOM_SIZE
+        size_t gosize = sizeof(HashKeyElement)+strlen(name_ptr->get())+1;
+        if (kT->releaseKey(name_ptr))
+            AttrStrAtom::totsize -= gosize;
+#else
         kT->releaseKey(name_ptr);
+#endif
     }
     if (!attrs)
         return;
@@ -3014,8 +2987,10 @@ CAtomPTree::~CAtomPTree()
         CriticalBlock block(hashcrit);
         while (a--!=attrs)
         {
-            attrHT->removekey(a->key, nc);
-            attrHT->removeval(a->value);
+            if (a->key.isPtr())
+                attrHT->removekey(a->key.getPtr(), nc);
+            if (a->value.isPtr())
+                attrHT->removeval(a->value.getPtr());
         }
         freeAttrArray(attrs, numAttrs);
     }
@@ -3024,51 +2999,65 @@ CAtomPTree::~CAtomPTree()
 void CAtomPTree::setName(const char *_name)
 {
     AtomRefTable *kT = isnocase()?keyTableNC:keyTable;
-    HashKeyElement *oname = name_ptr;
+    HashKeyElement *oname = name.getPtr(); // NOTE - don't release yet as could overlap source name
     if (!_name)
-        name_ptr = nullptr;
+        name.setPtr(nullptr);
     else
     {
         if (!validateXMLTag(_name))
             throw MakeIPTException(PTreeExcpt_InvalidTagName, ": %s", _name);
-        name_ptr = kT->queryCreate(_name);
-#ifdef TRACE_ALL_NAME
-        DBGLOG("CAtomPTree::setName %s", _name);
-#endif
-#ifdef TRACE_NAME_SIZE
-        if (name_ptr->queryReferences()==1)  // Not strictly accurate but good enough
+        if (!name.set(_name))
         {
-            totsize += sizeof(HashKeyElement)+strlen(_name)+1;
-            if (totsize > maxsize)
+#ifdef TRACE_ALL_ATOM
+            DBGLOG("TRACE_ALL_ATOM: %s", _name);
+#endif
+#ifdef TRACE_ATOM_SIZE
+            bool didCreate;
+            name.setPtr(kT->queryCreate(_name, didCreate));
+            if (didCreate)
             {
-                maxsize.store(totsize);
-                DBGLOG("AtomName total size now %" I64F "d", maxsize.load());
+                AttrStrAtom::totsize += sizeof(HashKeyElement)+strlen(_name)+1;
+                if (AttrStrAtom::totsize > AttrStrAtom::maxsize)
+                {
+                    AttrStrAtom::maxsize.store(AttrStrAtom::totsize);
+                    DBGLOG("TRACE_ATOM_SIZE: total size now %" I64F "d", AttrStrAtom::maxsize.load());
+                }
             }
-        }
+#else
+            name.setPtr(kT->queryCreate(_name));
 #endif
+        }
     }
     if (oname)
     {
-#ifdef TRACE_NAME_SIZE
-        if (oname->queryReferences()==1)  // Not strictly accurate but good enough
-            totsize -= sizeof(HashKeyElement)+strlen(oname->get())+1;
-#endif
+#ifdef TRACE_ATOM_SIZE
+        size_t gosize = sizeof(HashKeyElement)+strlen(oname->get())+1;
+        if (kT->releaseKey(oname))
+            AttrStrAtom::totsize -= gosize;
+#else
         kT->releaseKey(oname);
+#endif
     }
 }
 
 const char *CAtomPTree::queryName() const
 {
-    if (!name_ptr)
-        return nullptr;
-    else
-        return name_ptr->get();
+    return name.get();
 }
 
 unsigned CAtomPTree::queryHash() const
 {
-    assertex(name_ptr);
-    return name_ptr->queryHash();
+    if (name.isPtr())
+    {
+        assert(name.getPtr());
+        return name.getPtr()->queryHash();
+    }
+    else
+    {
+        const char *_name = name.get();
+        size32_t nl = strlen(_name);
+        return isnocase() ? hashnc((const byte *) _name, nl, 0): hashc((const byte *) _name, nl, 0);
+    }
 }
 
 AttrValue *CAtomPTree::newAttrArray(unsigned n)
@@ -3113,19 +3102,24 @@ void CAtomPTree::setAttribute(const char *key, const char *val)
     AttrValue *v = findAttribute(key);
     if (v)
     {
-        if (streq(v->value->get(), val))
+        if (streq(v->value.get(), val))
             return;
         CriticalBlock block(hashcrit);
-        attrHT->removeval(v->value);
-        v->value = attrHT->addval(val);
+        if (v->value.isPtr())
+            attrHT->removeval(v->value.getPtr());
+        if (!v->value.set(val))
+            v->value.setPtr(attrHT->addval(val));
     }
     else
     {
         CriticalBlock block(hashcrit);
         AttrValue *newattrs = newAttrArray(numAttrs+1);
         memcpy(newattrs, attrs, numAttrs*sizeof(AttrValue));
-        newattrs[numAttrs].key = attrHT->addkey(key, isnocase());
-        newattrs[numAttrs].value = attrHT->addval(val);
+        v = &newattrs[numAttrs];
+        if (!v->key.set(key))
+            v->key.setPtr(attrHT->addkey(key, isnocase()));
+        if (!v->value.set(val))
+            v->value.setPtr(attrHT->addval(val));
         freeAttrArray(attrs, numAttrs);
         numAttrs++;
         attrs = newattrs;
@@ -3139,8 +3133,10 @@ bool CAtomPTree::removeAttribute(const char *key)
         return false;
     numAttrs--;
     CriticalBlock block(hashcrit);
-    attrHT->removekey(del->key, isnocase());
-    attrHT->removeval(del->value);
+    if (del->key.isPtr())
+        attrHT->removekey(del->key.getPtr(), isnocase());
+    if (del->value.isPtr())
+        attrHT->removeval(del->value.getPtr());
     AttrValue *newattrs = newAttrArray(numAttrs);
     if (newattrs)
     {

system/jlib/jptree.ipp

@@ -223,12 +223,10 @@ private:
 // (see class AttrStrAtom below).
 // This requires some care - in particular must use the right method to destroy the objects, and must not add any virtual methods to either class
 
-//#define TRACE_ATTRSTR_SIZE
-//#define TRACE_ATTRATOM_SIZE
-//#define TRACE_NAME_SIZE
-//#define TRACE_ALL_ATTRATOM
-//#define TRACE_ALL_ATTRSTR
-//#define TRACE_ALL_NAME
+//#define TRACE_STRING_SIZE
+//#define TRACE_ATOM_SIZE
+//#define TRACE_ALL_STRING
+//#define TRACE_ALL_ATOM
 
 struct AttrStr
 {
@@ -241,15 +239,15 @@ struct AttrStr
     static AttrStr *create(const char *k)
     {
         size32_t kl = k ? strlen(k) : 0;
-#ifdef TRACE_ALL_ATTRSTR
-        DBGLOG("AttrStr::Create %s", k);
+#ifdef TRACE_ALL_STRING
+        DBGLOG("TRACE_ALL_STRING: %s", k);
 #endif
-#ifdef TRACE_ATTRSTR_SIZE
+#ifdef TRACE_STRING_SIZE
         totsize += kl+1;
         if (totsize > maxsize)
         {
             maxsize.store(totsize);
-            DBGLOG("AttrStr total size now %" I64F "d", maxsize.load());
+            DBGLOG("TRACE_STRING_SIZE: total size now %" I64F "d", maxsize.load());
         }
 #endif
         AttrStr *ret = (AttrStr *) malloc(kl+1);
@@ -265,22 +263,121 @@ struct AttrStr
 
     static void destroy(AttrStr *a)
     {
-#ifdef TRACE_ATTRSTR_SIZE
+#ifdef TRACE_STRING_SIZE
         totsize -= strlen(a->str_DO_NOT_USE_DIRECTLY)+1;
 #endif
         free(a);
     }
 
-#ifdef TRACE_ATTRSTR_SIZE
+#ifdef TRACE_STRING_SIZE
     static std::atomic<__int64> totsize;
     static std::atomic<__int64> maxsize;
 #endif
 };
 
+// NOTE - hairy code alert!
+// To save on storage (and contention) we store short string values in same slot as the pointer to longer
+// ones would occupy. This relies on the assumption that the pointers you want to store are always AT LEAST
+// 2-byte aligned. This should be the case on anything coming from malloc on any modern architecture.
+
+#define USE_STRUNION
+
+template<class PTR>
+struct PtrStrUnion
+{
+#ifdef USE_STRUNION
+    union
+    {
+        PTR *ptr;
+        struct
+        {
+#ifdef LITTLE_ENDIAN
+            char flag;
+            char chars[sizeof(PTR *)-1];
+#else
+            char chars[sizeof(PTR *)-1];
+            char flag;
+#endif
+        };
+    };
+    inline PtrStrUnion<PTR>() : ptr(nullptr) {}
+    inline bool isPtr() const
+    {
+        return (flag&1) == 0;
+    }
+    inline const char *get() const
+    {
+        if (!isPtr())
+            return chars;
+        else if (ptr)
+            return ptr->get();
+        else
+            return nullptr;
+    }
+    inline void destroy()
+    {
+        if (isPtr() && ptr)
+            PTR::destroy(ptr);
+    }
+    bool set(const char *key)
+    {
+        if (key)
+        {
+            size32_t l = strlen(key);
+            if (l <= sizeof(PTR *)-2)
+            {
+                flag=1;
+                memmove(chars, key, l);  // Technically, they could overlap
+                chars[l]=0;
+                return true;
+            }
+        }
+        return false;
+    }
+    inline void setPtr(PTR *a)
+    {
+        ptr = a;
+        assert(isPtr());
+    }
+#else
+    PTR *ptr = nullptr;
+    inline bool isPtr()
+    {
+        return true;
+    }
+    inline const char *get()
+    {
+        if (ptr)
+            return ptr->get();
+        else
+            return nullptr;
+    }
+    inline void destroy()
+    {
+        if (ptr)
+            PTR::destroy(ptr);
+    }
+    bool set(const char *key)
+    {
+        return false;
+    }
+    inline void setPtr(PTR *a)
+    {
+        ptr = a;
+    }
+#endif
+    inline PTR *getPtr() const
+    {
+        return isPtr() ? ptr : nullptr;
+    }
+};
+
+typedef PtrStrUnion<AttrStr> AttrStrUnion;
+
 struct AttrValue
 {
-    AttrStr *key;
-    AttrStr *value;
+    AttrStrUnion key;
+    AttrStrUnion value;
 };
 
 
@@ -432,15 +529,15 @@ struct AttrStrAtom
 
     static AttrStrAtom *create(const char *k, size32_t kl, hashfunc _hash)
     {
-#ifdef TRACE_ALL_ATTRATOM
-        DBGLOG("AttrStr::Create %s", k);
+#ifdef TRACE_ALL_ATOM
+        DBGLOG("TRACE_ALL_ATOM: %s", k);
 #endif
-#ifdef TRACE_ATTRATOM_SIZE
+#ifdef TRACE_ATOM_SIZE
         totsize += sizeof(AttrStrAtom)+kl+1;
         if (totsize > maxsize)
         {
             maxsize.store(totsize);
-            DBGLOG("AttrStrAtom total size now %" I64F "d", maxsize.load());
+            DBGLOG("TRACE_ATOM_SIZE: total size now %" I64F "d", maxsize.load());
         }
 #endif
         AttrStrAtom *ret = (AttrStrAtom *) malloc(offsetof(AttrStrAtom, str_DO_NOT_USE_DIRECTLY)+kl+1);
@@ -452,7 +549,7 @@ struct AttrStrAtom
     }
     static void destroy(AttrStrAtom *a)
     {
-#ifdef TRACE_ATTRATOM_SIZE
+#ifdef TRACE_ATOM_SIZE
         totsize -= sizeof(AttrStrAtom)+strlen(a->str_DO_NOT_USE_DIRECTLY)+1;
 #endif
         free(a);
@@ -466,7 +563,7 @@ struct AttrStrAtom
     {
         return (AttrStrAtom *)(&a->str_DO_NOT_USE_DIRECTLY - offsetof(AttrStrAtom, str_DO_NOT_USE_DIRECTLY));
     }
-#ifdef TRACE_ATTRATOM_SIZE
+#ifdef TRACE_ATOM_SIZE
     static std::atomic<__int64> totsize;
     static std::atomic<__int64> maxsize;
 #endif
@@ -556,14 +653,9 @@ public:
 
 class jlib_decl CAtomPTree : public PTree
 {
-#ifdef TRACE_NAME_SIZE
-    static std::atomic<__int64> totsize;
-    static std::atomic<__int64> maxsize;
-#endif
     AttrValue *newAttrArray(unsigned n);
     void freeAttrArray(AttrValue *a, unsigned n);
-
-    HashKeyElement *name_ptr = nullptr;
+    PtrStrUnion<HashKeyElement> name;
 protected:
     virtual void setAttribute(const char *attr, const char *val) override;
     virtual bool removeAttribute(const char *k) override;
@@ -592,13 +684,9 @@ jlib_decl IPropertyTree *createPropBranch(IPropertyTree *tree, const char *xpath
 class jlib_decl LocalPTree : public PTree
 {
 protected:
-#ifdef TRACE_NAME_SIZE
-    static std::atomic<__int64> totsize;
-    static std::atomic<__int64> maxsize;
-#endif
     virtual void setAttribute(const char *attr, const char *val) override;
     virtual bool removeAttribute(const char *k) override;
-    const char *name_ptr = nullptr;
+    AttrStrUnion name;
 public:
     LocalPTree(const char *name=nullptr, byte flags=ipt_none, IPTArrayValue *value=nullptr, ChildMap *children=nullptr);
     ~LocalPTree();

system/jlib/jsuperhash.hpp

@@ -530,20 +530,38 @@ public:
         return key;
     }
 
+    inline HashKeyElement *queryCreate(const char *_key, bool &didCreate)
+    {
+        CriticalBlock b(crit);
+        HashKeyElement *key = find(*_key);
+        if (key)
+        {
+            didCreate = false;
+            key->linkCount++;
+        }
+        else
+        {
+            didCreate = true;
+            key = createKeyElement(_key);
+        }
+        return key;
+    }
+
     inline void linkKey(const char *key)
     {
         queryCreate(key);
     }
 
-    inline void releaseKey(HashKeyElement *key)
+    inline bool releaseKey(HashKeyElement *key)
     {
         CriticalBlock b(crit);
         if (0 == key->linkCount)
         {
             verifyex(removeExact(key));
-            return;
+            return true;
         }
         --key->linkCount;
+        return false;
     }
 
 protected:

testing/unittests/unittests.cpp

@@ -326,7 +326,7 @@ class PtreeThreadingTest : public CppUnit::TestFixture
             {
                 for (unsigned i = 0; i < iterations; i++)
                 {
-                    Owned<IPropertyTree> p = mode >= 3 ? nullptr : createPTreeFromXMLString(
+                    Owned<IPropertyTree> p = mode >= 3 ? nullptr : createPTreeFromXMLString(false ? "<helloxx there='1'/>" :
                             "<W_LOCAL buildVersion='community_6.0.0-trunk0Debug[heads/cass-wu-part3-0-g10b954-dirty]'"
                             "         cloneable='1'"
                             "         clusterName=''"
@@ -539,10 +539,10 @@ class PtreeThreadingTest : public CppUnit::TestFixture
           csome("control some",flags,3,200),
           cmin("control min",flags,4,200),
           seq("single",flags,0,1000);
-        max.For(8,8);
-        some.For(8,8,csome.For(8,8));
-        min.For(8,8,cmin.For(8,8));
-        seq.For(8,1);
+          max.For(8,8);
+          some.For(8,8,csome.For(8,8));
+          min.For(8,8,cmin.For(8,8));
+          seq.For(8,1);
     }
 };