Remove global lock from OAMCache

Config now uses a single atomic variable to speed up its operations
Config has a special method to re-read a config file if it changed on disk

dbcon/joblist/resourcemanager.cpp

@@ -365,19 +365,16 @@ void  ResourceManager::hbrPredicate() { }
 bool  ResourceManager::getMysqldInfo(
     std::string& h, std::string& u, std::string& w, unsigned int& p) const
 {
-    h = getStringVal("CrossEngineSupport", "Host", "unassigned");
+    static const std::string hostUserUnassignedValue("unassigned");
+    // MCS will read username and pass from disk if the config changed.
+    bool reReadConfig = true;
+    u = getStringVal("CrossEngineSupport", "User", hostUserUnassignedValue, reReadConfig);
+    w = getStringVal("CrossEngineSupport", "Password", "", reReadConfig);
+    // MCS will not read username and pass from disk if the config changed.
+    h = getStringVal("CrossEngineSupport", "Host", hostUserUnassignedValue);
     p = getUintVal("CrossEngineSupport", "Port", 0);
-    u = getStringVal("CrossEngineSupport", "User", "unassigned");
-    w = getStringVal("CrossEngineSupport", "Password", "");
 
-    bool rc = true;
+    return h != hostUserUnassignedValue && u != hostUserUnassignedValue && p;
-
-    if ((h.compare("unassigned") == 0) ||
-            (u.compare("unassigned") == 0) ||
-            (p == 0))
-        rc = false;
-
-    return rc;
 }
 
 bool ResourceManager::queryStatsEnabled() const

dbcon/joblist/resourcemanager.h

@@ -35,6 +35,7 @@
 #include "calpontselectexecutionplan.h"
 #include "resourcedistributor.h"
 #include "installdir.h"
+#include "branchpred.h"
 
 #include "atomicops.h"
 
@@ -547,7 +548,10 @@ private:
      * @param name the param name whose value is to be returned
      * @param defVal the default value returned if the value is missing
      */
-    std::string getStringVal(const std::string& section, const std::string& name, const std::string& defVal) const;
+    std::string getStringVal(const std::string& section,
+                             const std::string& name,
+                             const std::string& defVal,
+                             const bool reReadConfigIfNeeded = false) const;
 
     template<typename IntType>
     IntType getUintVal(const std::string& section, const std::string& name, IntType defval) const;
@@ -603,13 +607,20 @@ private:
 };
 
 
-inline std::string ResourceManager::getStringVal(const std::string& section, const std::string& name, const std::string& defval) const
+inline std::string ResourceManager::getStringVal(const std::string& section,
+                                                 const std::string& name,
+                                                 const std::string& defval,
+                                                 const bool reReadConfigIfNeeded) const
 {
-    std::string val = fConfig->getConfig(section, name);
+    std::string val = UNLIKELY(reReadConfigIfNeeded)
+                        ? fConfig->getFromActualConfig(section, name)
+                        : fConfig->getConfig(section, name);
 #ifdef DEBUGRM
     std::cout << "RM getStringVal for " << section << " : " << name << " val: " << val << " default: " << defval << std::endl;
 #endif
-    return (0 == val.length() ? defval : val);
+    if (val.empty())
+        val = defval;
+    return val;
 }
 
 template<typename IntType>

oam/oamcpp/oamcache.cpp

@@ -21,10 +21,10 @@
 #include <cassert>
 #include <map>
 #include <set>
+#include <atomic>
 using namespace std;
 
 #include <boost/thread.hpp>
-#include <boost/shared_ptr.hpp>
 using namespace boost;
 
 #define OAMCACHE_DLLEXPORT
@@ -38,19 +38,27 @@ using namespace boost;
 
 namespace
 {
-oam::OamCache* oamCache = 0;
+    oam::OamCache* oamCache = nullptr;
-boost::mutex cacheLock;
+    boost::mutex cacheLock;
 }
 
 namespace oam
 {
+std::atomic_bool hasOAMCache{false};
 
 OamCache* OamCache::makeOamCache()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
+    if (!hasOAMCache.load(std::memory_order_relaxed))
+    {
+        boost::mutex::scoped_lock lk(cacheLock);
 
-    if (oamCache == 0)
+        if (oamCache == nullptr)
-        oamCache = new OamCache();
+        {
+            oamCache = new OamCache();
+            oamCache->checkReload();
+            hasOAMCache.store(true, std::memory_order_relaxed);
+        }
+    }
 
     return oamCache;
 }
@@ -80,7 +88,7 @@ void OamCache::checkReload()
 
     dbRootPMMap.reset(new map<int, int>());
 
-    //cout << "reloading oamcache\n";
+    //cerr << "reloading oamcache\n";
     for (uint32_t i = 0; i < dbroots.size(); i++)
     {
         oam.getDbrootPmConfig(dbroots[i], temp);
@@ -154,64 +162,41 @@ void OamCache::checkReload()
 
 OamCache::dbRootPMMap_t OamCache::getDBRootToPMMap()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
-
-    checkReload();
     return dbRootPMMap;
 }
 
 OamCache::dbRootPMMap_t OamCache::getDBRootToConnectionMap()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
-
-    checkReload();
     return dbRootConnectionMap;
 }
 
 OamCache::PMDbrootsMap_t OamCache::getPMToDbrootsMap()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
-
-    checkReload();
     return pmDbrootsMap;
 }
 
 uint32_t OamCache::getDBRootCount()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
-
-    checkReload();
     return numDBRoots;
 }
 
 DBRootConfigList& OamCache::getDBRootNums()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
-
-    checkReload();
     return dbroots;
 }
 
 std::vector<int>& OamCache::getModuleIds()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
-
-    checkReload();
     return moduleIds;
 }
 
 std::string OamCache::getOAMParentModuleName()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
-
-    checkReload();
     return OAMParentModuleName;
 }
 
 int OamCache::getLocalPMId()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
-
     // This comes from the file /var/lib/columnstore/local/module, not from the xml.
     // Thus, it's not refreshed during checkReload().
     if (mLocalPMId > 0)
@@ -252,16 +237,11 @@ int OamCache::getLocalPMId()
 
 string OamCache::getSystemName()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
-
-    checkReload();
     return systemName;
 }
 
 string OamCache::getModuleName()
 {
-    boost::mutex::scoped_lock lk(cacheLock);
-
     if (!moduleName.empty())
         return moduleName;
 

utils/configcpp/configcpp.cpp

@@ -51,6 +51,7 @@ namespace fs = boost::filesystem;
 #include <cstring>
 //#define NDEBUG
 #include <cassert>
+#include <atomic>
 
 #include "configcpp.h"
 
@@ -64,51 +65,52 @@ namespace fs = boost::filesystem;
 
 namespace
 {
-const fs::path defaultCalpontConfigFile("Columnstore.xml");
+  static const std::string configDefaultFileName("Columnstore.xml");
+  const fs::path defaultConfigFilePath(configDefaultFileName);
 }
 
 namespace config
 {
-Config::configMap_t Config::fInstanceMap;
+  Config* globConfigInstancePtr = nullptr;
-boost::mutex Config::fInstanceMapMutex;
+  Config::configMap_t Config::fInstanceMap;
-boost::mutex Config::fXmlLock;
+  boost::mutex Config::fInstanceMapMutex;
-boost::mutex Config::fWriteXmlLock;
+  // duplicate to that in the Config class
+  boost::mutex Config::fXmlLock;
+  // duplicate to that in the Config class
+  boost::mutex Config::fWriteXmlLock;
+  std::atomic_bool globHasConfig;
 
 Config* Config::makeConfig(const string& cf)
 {
-    return makeConfig(cf.c_str());
+    if (cf.empty() || cf == configDefaultFileName)
+    {
+        if (!globHasConfig.load(std::memory_order_relaxed))
+        {
+            // To save against the moment zero race when multiple threads hits
+            // this scope.
+            boost::mutex::scoped_lock lk(fInstanceMapMutex);
+            if (globConfigInstancePtr)
+                return globConfigInstancePtr;
+
+            // Make this configurable at least at compile-time.
+            std::string configFilePath = std::string(MCSSYSCONFDIR) + std::string("/columnstore/") + configDefaultFileName;
+            globConfigInstancePtr = new Config(configFilePath);
+            globHasConfig.store(true, std::memory_order_relaxed);
+        }
+        return globConfigInstancePtr;
+    }
+
+    boost::mutex::scoped_lock lk(fInstanceMapMutex);
+    if (fInstanceMap.find(cf) == fInstanceMap.end())
+    {
+        fInstanceMap[cf] = new Config(cf);
+    }
+    return fInstanceMap[cf];
 }
 
 Config* Config::makeConfig(const char* cf)
 {
-    boost::mutex::scoped_lock lk(fInstanceMapMutex);
+    return cf ? makeConfig(std::string(cf)) : makeConfig(std::string(""));
-
-    static string defaultFilePath;
-
-    if (cf == 0 || *cf == 0)
-    {
-        fs::path configFilePath;
-        configFilePath = fs::path(MCSSYSCONFDIR) / fs::path("columnstore") / defaultCalpontConfigFile;
-        defaultFilePath = configFilePath.string();
-
-        if (fInstanceMap.find(defaultFilePath) == fInstanceMap.end())
-        {
-            Config* instance = new Config(defaultFilePath);
-            fInstanceMap[defaultFilePath] = instance;
-        }
-
-        return fInstanceMap[defaultFilePath];
-    }
-
-    string configFile(cf);
-
-    if (fInstanceMap.find(configFile) == fInstanceMap.end())
-    {
-        Config* instance = new Config(configFile);
-        fInstanceMap[configFile] = instance;
-    }
-
-    return fInstanceMap[configFile];
 }
 
 Config::Config(const string& configFile) :
@@ -216,8 +218,6 @@ void Config::closeConfig(void)
 
 const string Config::getConfig(const string& section, const string& name)
 {
-    boost::recursive_mutex::scoped_lock lk(fLock);
-
     if (section.length() == 0 || name.length() == 0)
         throw invalid_argument("Config::getConfig: both section and name must have a length");
 
@@ -226,46 +226,57 @@ const string Config::getConfig(const string& section, const string& name)
         throw runtime_error("Config::getConfig: no XML document!");
     }
 
-    struct stat statbuf;
-
-    if (stat(fConfigFile.c_str(), &statbuf) == 0)
-    {
-        if (statbuf.st_mtime != fMtime)
-        {
-            closeConfig();
-            fMtime = statbuf.st_mtime;
-            parseDoc();
-        }
-    }
-
     return fParser.getConfig(fDoc, section, name);
 }
 
 void Config::getConfig(const string& section, const string& name, vector<string>& values)
 {
-    boost::recursive_mutex::scoped_lock lk(fLock);
-
     if (section.length() == 0)
         throw invalid_argument("Config::getConfig: section must have a length");
 
     if (fDoc == 0)
         throw runtime_error("Config::getConfig: no XML document!");
 
+    fParser.getConfig(fDoc, section, name, values);
+}
+
+const string Config::getFromActualConfig(const string& section, const string& name)
+{
+    if (section.length() == 0 || name.length() == 0)
+        throw invalid_argument("Config::getFromActualConfig: both section and name must have a length");
+
+    if (fDoc == 0)
+    {
+        throw runtime_error("Config::getFromActualConfig: no XML document!");
+    }
+
     struct stat statbuf;
 
     if (stat(fConfigFile.c_str(), &statbuf) == 0)
     {
+        // Config was changed on disk since last read.
         if (statbuf.st_mtime != fMtime)
         {
-            closeConfig();
+            boost::recursive_mutex::scoped_lock lk(fLock);
-            fMtime = statbuf.st_mtime;
+            // To protect the potential race that happens right after
-            parseDoc();
+            // the config was changed.
+            if (!stat(fConfigFile.c_str(), &statbuf) &&
+                statbuf.st_mtime != fMtime)
+            {
+                closeConfig();
+                fMtime = statbuf.st_mtime;
+                parseDoc();
+            }
         }
     }
 
-    fParser.getConfig(fDoc, section, name, values);
+    return fParser.getConfig(fDoc, section, name);
 }
 
+
+// NB The only utility that uses setConfig is setConfig binary.
+// !!!Don't ever ever use this in the engine code b/c it might result in a race
+// b/w getConfig and setConfig methods.!!!
 void Config::setConfig(const string& section, const string& name, const string& value)
 {
     boost::recursive_mutex::scoped_lock lk(fLock);
@@ -281,7 +292,6 @@ void Config::setConfig(const string& section, const string& name, const string&
     struct stat statbuf;
 
     memset(&statbuf, 0, sizeof(statbuf));
-
     if (stat(fConfigFile.c_str(), &statbuf) == 0)
     {
         if (statbuf.st_mtime != fMtime)
@@ -356,14 +366,14 @@ void Config::writeConfig(const string& configFile) const
     fs::rename(outFilePth, configFilePth);
 #else
 
-    const fs::path defaultCalpontConfigFileTemp("Columnstore.xml.temp");
+    const fs::path defaultConfigFilePathTemp("Columnstore.xml.temp");
     const fs::path saveCalpontConfigFileTemp("Columnstore.xml.columnstoreSave");
     const fs::path tmpCalpontConfigFileTemp("Columnstore.xml.temp1");
 
     fs::path etcdir = fs::path(MCSSYSCONFDIR) / fs::path("columnstore");
 
-    fs::path dcf = etcdir / fs::path(defaultCalpontConfigFile);
+    fs::path dcf = etcdir / fs::path(defaultConfigFilePath);
-    fs::path dcft = etcdir / fs::path(defaultCalpontConfigFileTemp);
+    fs::path dcft = etcdir / fs::path(defaultConfigFilePathTemp);
     fs::path scft = etcdir / fs::path(saveCalpontConfigFileTemp);
     fs::path tcft = etcdir / fs::path(tmpCalpontConfigFileTemp);
 
@@ -548,6 +558,12 @@ void Config::deleteInstanceMap()
     }
 
     fInstanceMap.clear();
+
+    if (globConfigInstancePtr)
+    {
+        delete globConfigInstancePtr;
+        globConfigInstancePtr = nullptr;
+    }
 }
 
 /* static */
@@ -600,8 +616,6 @@ int64_t Config::fromText(const std::string& text)
 
 time_t Config::getCurrentMTime()
 {
-    boost::recursive_mutex::scoped_lock lk(fLock);
-
     struct stat statbuf;
 
     if (stat(fConfigFile.c_str(), &statbuf) == 0)
@@ -610,6 +624,7 @@ time_t Config::getCurrentMTime()
         return 0;
 }
 
+// Utilized by getConfig only
 const vector<string> Config::enumConfig()
 {
     boost::recursive_mutex::scoped_lock lk(fLock);
@@ -634,6 +649,7 @@ const vector<string> Config::enumConfig()
     return fParser.enumConfig(fDoc);
 }
 
+// Utilized by getConfig only
 const vector<string> Config::enumSection(const string& section)
 {
     boost::recursive_mutex::scoped_lock lk(fLock);

utils/configcpp/configcpp.h

@@ -87,6 +87,15 @@ public:
     */
     EXPORT const std::string getConfig(const std::string& section, const std::string& name);
 
+    /** @brief get name's value from section
+    *
+    * get name's value from section in the current config file re-reading the
+    * config file if it was updated.
+    * @param section the name of the config file section to search
+    * @param name the param name whose value is to be returned
+    */
+    const std::string getFromActualConfig(const std::string& section, const std::string& name);
+
     /** @brief get all name's values from a section
     *
     * get name's values from section in the current config file.
@@ -104,6 +113,8 @@ public:
     * @param name the param name whose value is to be updated
     * @param value the param value
     */
+    // !!!Don't ever ever use this in the engine code b/c it might result in a race
+    // b/w getConfig and setConfig methods.!!!
     EXPORT void setConfig(const std::string& section, const std::string& name, const std::string& value);
 
     /** @brief delete name from section