Merge pull request #6740 from ghalliday/issue12708

HPCC-12708 Optimize jset and provide countTrailingZeros() helpers

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

system/jlib/jset.cpp

@@ -84,91 +84,60 @@ protected:
         unsigned j=from%BitsPerItem;
         // returns index of first = val >= from
         unsigned n=getWidth();
-        unsigned i;
-        for (i=from/BitsPerItem;i<n;i++)
+        unsigned i = from/BitsPerItem;
+        if (j != 0 && i < n)
         {
             bits_t m = getBits(i);
             if (m!=noMatchMask)
             {
-#if defined(__GNUC__)
-                //Use the __builtin_ffs instead of a loop to find the first bit set/cleared
-                bits_t testMask = m;
-                if (j != 0)
-                {
-                    //Set all the bottom bits to the value we're not searching for
-                    bits_t mask = (((bits_t)1)<<j)-1;
-                    if (tst)
-                        testMask &= ~mask;
-                    else
-                        testMask |= mask;
+                //Evaluate (testMask = tst ? m : ~m); without using a conditional.
+                //After this bits will be set wherever we want a match
+                bits_t testMask = m ^ noMatchMask;
 
-                    //May possibly match exactly - if so continue main loop
-                    if (testMask==noMatchMask)
-                    {
-                        j = 0;
-                        continue;
-                    }
-                }
+                //Clear all the bottom bits
+                bits_t mask = (((bits_t)1)<<j)-1;
+                testMask &= ~mask;
 
-                //Guaranteed a match at this point
-                if (tst)
+                //May possibly be empty now ignored bits are removed - if so continue main loop
+                if (testMask != 0)
                 {
-                    //Returns one plus the index of the least significant 1-bit of testMask
+                    //Guaranteed a match at this point
+                    //Returns one the index of the least significant 1-bit of testMask
                     //(testMask != 0) since that has been checked above (noMatchMask == 0)
-                    unsigned pos = __builtin_ffs(testMask)-1;
+                    unsigned pos = countTrailingUnsetBits(testMask);
                     if (scninv)
                     {
                         bits_t t = ((bits_t)1)<<pos;
-                        m &= ~t;
+                        m ^= t;
                         setBits(i, m);
                     }
                     return i*BitsPerItem+pos;
                 }
-                else
-                {
-                    //Same as above but invert the bitmask
-                    unsigned pos = __builtin_ffs(~testMask)-1;
-                    if (scninv)
-                    {
-                        bits_t t = ((bits_t)1)<<pos;
-                        m |= t;
-                        setBits(i, m);
-                    }
-                    return i*BitsPerItem+pos;
-                }
-#else
-                bits_t t = ((bits_t)1)<<j;
-                for (;j<BitsPerItem;j++)
+            }
+            j = 0;
+            i++;
+        }
+        for (;i<n;i++)
+        {
+            bits_t m = getBits(i);
+            if (m!=noMatchMask)
+            {
+                //Evaluate (testMask = tst ? m : ~m); without using a conditional.
+                //After this bits will be set wherever we want a match
+                bits_t testMask = m ^ noMatchMask;
+
+                //Guaranteed a match at this point
+                //Returns one the index of the least significant 1-bit of testMask
+                //(testMask != 0) since that has been checked above (noMatchMask == 0)
+                unsigned pos = countTrailingUnsetBits(testMask);
+                if (scninv)
                 {
-                    if (t&m)
-                    {
-                        if (tst)
-                        {
-                            if (scninv)
-                            {
-                                m &= ~t;
-                                setBits(i, m);
-                            }
-                            return i*BitsPerItem+j;
-                        }
-                    }
-                    else
-                    {
-                        if (!tst)
-                        {
-                            if (scninv)
-                            {
-                                m |= t;
-                                setBits(i, m);
-                            }
-                            return i*BitsPerItem+j;
-                        }
-                    }
-                    t <<= 1;
+                    bits_t t = ((bits_t)1)<<pos;
+                    m ^= t;
+                    setBits(i, m);
                 }
-#endif
+                return i*BitsPerItem+pos;
             }
-            j = 0;
         }
         if (tst)
             return (unsigned)-1;

system/jlib/jset.hpp

@@ -22,7 +22,78 @@
 
 #include "jiface.hpp"
 
+#if defined (_WIN32)
+#include <intrin.h>
+#endif
 
+//Return the number of trailing zeros. Deliberately undefined if value == 0
+inline unsigned countTrailingUnsetBits(unsigned value)
+{
+    dbgassertex(value != 0);
+#if defined(__GNUC__)
+    return __builtin_ctz(value);
+#elif defined (_WIN32)
+    unsigned long index;
+    _BitScanForward(&index, value);
+    return (unsigned)index;
+#else
+    unsigned mask = 1U;
+    unsigned i;
+    for (i=0; i < sizeof(unsigned)*8; i++)
+    {
+        if (value & mask)
+            return i;
+        mask = mask << 1;
+    }
+    return i;
+#endif
+}
+
+//Return the number of leading zeros. Deliberately undefined if value == 0
+inline unsigned countLeadingUnsetBits(unsigned value)
+{
+    dbgassertex(value != 0);
+#if defined(__GNUC__)
+    return __builtin_clz(value);
+#elif defined (_WIN32)
+    unsigned long index;
+    _BitScanReverse(&index, value);
+    return (unsigned)((sizeof(unsigned)*8)-1 - index);
+#else
+    unsigned mask = 1U << ((sizeof(unsigned)*8)-1);
+    unsigned i;
+    for (i=0; i < sizeof(unsigned)*8; i++)
+    {
+        if (value & mask)
+            return i;
+        mask = mask >> 1;
+    }
+    return i;
+#endif
+}
+
+//Return the number of bits including the first non-zero bit.  Undefined if value == 0
+inline unsigned getMostSignificantBit(unsigned value)
+{
+    dbgassertex(value != 0);
+#if defined(__GNUC__)
+    return (sizeof(unsigned)*8) - __builtin_clz(value);
+#elif defined (_WIN32)
+    unsigned long index;
+    _BitScanReverse(&index, value);
+    return (unsigned)index+1;
+#else
+    unsigned mask = 1U << ((sizeof(unsigned)*8)-1);
+    unsigned i;
+    for (i=0; i < sizeof(unsigned)*8; i++)
+    {
+        if (value & mask)
+            return sizeof(unsigned)*8-i;
+        mask = mask >> 1;
+    }
+    return 0;
+#endif
+}
 
 interface jlib_decl IBitSet : public IInterface 
 {

testing/unittests/jlibtests.cpp

@@ -90,16 +90,30 @@ class JlibSetTest : public CppUnit::TestFixture
 {
 public:
     CPPUNIT_TEST_SUITE(JlibSetTest);
+        CPPUNIT_TEST(testBitsetHelpers);
         CPPUNIT_TEST(testSimple);
     CPPUNIT_TEST_SUITE_END();
 
 protected:
 
+    void testBitsetHelpers()
+    {
+        CPPUNIT_ASSERT_EQUAL(0U, countTrailingUnsetBits(1));
+        CPPUNIT_ASSERT_EQUAL(31U, countLeadingUnsetBits(1));
+        CPPUNIT_ASSERT_EQUAL(1U, getMostSignificantBit(1));
+        CPPUNIT_ASSERT_EQUAL(4U, countTrailingUnsetBits(0x110));
+        CPPUNIT_ASSERT_EQUAL(23U, countLeadingUnsetBits(0x110));
+        CPPUNIT_ASSERT_EQUAL(9U, getMostSignificantBit(0x110));
+        CPPUNIT_ASSERT_EQUAL(0U, countTrailingUnsetBits(0xFFFFFFFFU));
+        CPPUNIT_ASSERT_EQUAL(0U, countLeadingUnsetBits(0xFFFFFFFFU));
+        CPPUNIT_ASSERT_EQUAL(32U, getMostSignificantBit(0xFFFFFFFFU));
+    }
+
     void testSet1(bool initial, IBitSet *bs, unsigned start, unsigned numBits, bool setValue, bool clearValue)
     {
         unsigned end = start+numBits;
         if (initial)
-            bs->incl(start, end);
+            bs->incl(start, end-1);
         for (unsigned i=start; i < end; i++)
         {
             ASSERT(bs->test(i) == clearValue);
@@ -111,8 +125,10 @@ protected:
             ASSERT(bs->scan(i+1, setValue) == i+5);
             bs->set(i, clearValue);
             bs->set(i+5, clearValue);
+            //Clearing i+5 above may extend the set - so need to calculate the end carefully
+            unsigned last = i+5 < end ? end : i + 6;
             unsigned match1 = bs->scan(0, setValue);
-            ASSERT(match1 == initial ? -1 : end);
+            CPPUNIT_ASSERT_EQUAL((unsigned)(initial ? last : -1), match1);
 
             bs->invert(i);
             ASSERT(bs->test(i) == setValue);