HPCC-26443 Post review changes

Signed-off-by: Jake Smith <jake.smith@lexisnexisrisk.com>

helm/hpcc/values.schema.json

@@ -484,7 +484,8 @@
           "type": "string",
           "description": "The amount of overall resource memory to reserve for 3rd party use"
         }
-      }
+      },
+      "additionalProperties": false
     },
     "secrets": {
       "oneOf": [

thorlcr/graph/thgraph.cpp

@@ -2740,32 +2740,33 @@ CJobBase::CJobBase(ILoadedDllEntry *_querySo, const char *_graphName) : querySo(
         throwUnexpected();
 }
 
-void CJobBase::applyMemorySettings(float recommendReservePercentage, const char *context)
+void CJobBase::applyMemorySettings(float recommendedMaxPercentage, const char *context)
 {
+    // NB: 'total' memory has been calculated in advance from either resource settings or from system memory.
     VStringBuffer totalMemorySetting("%sMemory/@total", context);
     unsigned totalMemoryMB = globals->getPropInt(totalMemorySetting);
 
-    unsigned recommendReservedMemoryMB = totalMemoryMB * recommendReservePercentage / 100;
+    unsigned recommendedMaxMemoryMB = totalMemoryMB * recommendedMaxPercentage / 100;
 #ifdef _CONTAINERIZED
-    /* only "query" memory is actually used (if set configures Thor roxiemem limit)
-     * others are only advisory, but totalled and checked if within the total limit.
+    /* only "query" memory is actually used (if set, configures Thor roxiemem limit)
+     * others are only advisory, but totalled and checked to ensure within the total limit.
      */
-    std::vector<std::string> memorySettings = { "query", "thirdParty" };
+    std::initializer_list<const char *> memorySettings = { "query", "thirdParty" };
     offset_t totalRequirements = 0;
-    for (const auto &setting : memorySettings)
+    for (auto setting : memorySettings)
     {
-        VStringBuffer workunitSettingName("%smemory.%s", context, setting.c_str()); // NB: workunit options are case insensitive
+        VStringBuffer workunitSettingName("%smemory.%s", context, setting); // NB: workunit options are case insensitive
         StringBuffer memString;
         getWorkUnitValue(workunitSettingName, memString);
         if (0 == memString.length())
         {
-            VStringBuffer globalSettingName("%sMemory/@%s", context, setting.c_str());
+            VStringBuffer globalSettingName("%sMemory/@%s", context, setting);
             globals->getProp(globalSettingName, memString);
         }
         if (memString.length())
         {
             offset_t memBytes = friendlyStringToSize(memString);
-            if (streq("query", setting.c_str()))
+            if (streq("query", setting))
                 queryMemoryMB = (unsigned)(memBytes / 0x100000);
             totalRequirements += memBytes;
         }
@@ -2774,21 +2775,18 @@ void CJobBase::applyMemorySettings(float recommendReservePercentage, const char
     if (totalRequirementsMB > totalMemoryMB)
         throw makeStringExceptionV(0, "The total memory requirements of the query (%u MB) exceeds the %s memory limit (%u MB)", totalRequirementsMB, context, totalMemoryMB);
 
-    unsigned remainingMB = totalMemoryMB - totalRequirementsMB;
-    if (remainingMB < recommendReservedMemoryMB)
+    if (totalRequirementsMB > recommendedMaxMemoryMB)
     {
-        WARNLOG("The total memory requirements of the query (%u MB) exceed the recommended reserve limits for %s (total memory: %u MB, reserve recommendation: %.2f%%)", totalRequirementsMB, context, totalMemoryMB, recommendReservePercentage);
+        WARNLOG("The total memory requirements of the query (%u MB) exceed the recommended reserve limits for %s (total memory: %u MB, recommended max percentage : %.2f%%)", totalRequirementsMB, context, totalMemoryMB, recommendedMaxPercentage);
+    
+        // if "query" memory has not been defined, then use the remaining memory
         if (0 == queryMemoryMB)
-            queryMemoryMB = remainingMB; // probably not recommended - use all of remaining in this case for query memory.
+            queryMemoryMB = totalMemoryMB - totalRequirementsMB;
     }
     else if (0 == queryMemoryMB)
-        queryMemoryMB = remainingMB - recommendReservedMemoryMB;
+        queryMemoryMB = recommendedMaxMemoryMB - totalRequirementsMB;
 #else
-    if (totalMemoryMB < recommendReservedMemoryMB)
-        throw makeStringExceptionV(0, "The total memory (%u MB) is less than recommendReservedMemoryMB (%u MB), recommendReservePercentage (%.2f%%)", totalMemoryMB, recommendReservedMemoryMB, recommendReservePercentage);
-
-    unsigned remainingMB = totalMemoryMB;
-    queryMemoryMB = totalMemoryMB - recommendReservedMemoryMB;
+    queryMemoryMB = recommendedMaxMemoryMB;
 #endif
 
     bool gmemAllowHugePages = globals->getPropBool("@heapUseHugePages", false);
@@ -2797,7 +2795,7 @@ void CJobBase::applyMemorySettings(float recommendReservePercentage, const char
     bool gmemRetainMemory = globals->getPropBool("@heapRetainMemory", false);
     roxiemem::setTotalMemoryLimit(gmemAllowHugePages, gmemAllowTransparentHugePages, gmemRetainMemory, ((memsize_t)queryMemoryMB) * 0x100000, 0, thorAllocSizes, NULL);
 
-    PROGLOG("Total memory = %u MB, query memory = %u MB, memory spill at = %u (reserve = %.2f%%, reserveMB = %u, remainingMB = %u)", totalMemoryMB, queryMemoryMB, memorySpillAtPercentage, recommendReservePercentage, recommendReservedMemoryMB, remainingMB);
+    PROGLOG("Total memory = %u MB, query memory = %u MB, memory spill at = %u", totalMemoryMB, queryMemoryMB, memorySpillAtPercentage);
 }
 
 void CJobBase::init()

thorlcr/graph/thgraph.hpp

@@ -73,16 +73,22 @@
  * from total system memory.
  *
  * For historical reasons the default in bare-metal has always been a
- * conservative reserve of 25%.
+ * conservative 75% of system memory, leaving 25% free for the heap/OS etc.
+ * In container mode a more aggresive default of 90% is used.
  *
- * NB: These percentages do not apply if the memory amount has been configured
- * manually via 'globalMemorySize' and 'masterMemorySize'
+ * In bare-metal, these percentages do not apply if
+ * 'globalMemorySize' and/or 'masterMemorySize' are configured.
+ * 
+ * In container mode, workerMemory and/or masterMemory can be used to override
+ * these default percentages. However, the defaults percentages will stil be
+ * used to give a warning if the workerMemory or masterMemory totals exceed the
+ * defaults.
  */
 
 #ifdef _CONTAINERIZED
-constexpr float roxieMemPercentage = 10.0;
+constexpr float defaultPctSysMemForRoxie = 90.0;
 #else
-constexpr float roxieMemPercentage = 25.0;
+constexpr float defaultPctSysMemForRoxie = 75.0;
 #endif
 
 

thorlcr/graph/thgraphmaster.cpp

@@ -1347,13 +1347,13 @@ CJobMaster::CJobMaster(IConstWorkUnit &_workunit, const char *graphName, ILoaded
         loadPlugin(pluginMap, pluginsDir.str(), name.str());
     }
 
-    unsigned recommendReservePercentage = roxieMemPercentage;
+    float recommendedMaxPercentage = defaultPctSysMemForRoxie;
 #ifndef _CONTAINERIZED
-    // Weird @localThor mode, where it 50% of memory is dedicated to slaves, 25% to manager and 25% reserved (for OS etc.)
+    // @localThor mode - 25% is used for manager and 50% is used for workers
     if (globals->getPropBool("@localThor") && (0 == globals->getPropInt("@masterMemorySize")))
-        recommendReservePercentage = 25 + 50;
+        recommendedMaxPercentage = 25.0;
 #endif
-    applyMemorySettings(recommendReservePercentage, "manager");
+    applyMemorySettings(recommendedMaxPercentage, "manager");
     sharedAllocator.setown(::createThorAllocator(queryMemoryMB, 0, 1, memorySpillAtPercentage, *logctx, crcChecking, usePackedAllocator));
     Owned<IMPServer> mpServer = getMPServer();
     CJobChannel *channel = addChannel(mpServer);

thorlcr/graph/thgraphslave.cpp

@@ -1687,20 +1687,41 @@ CJobSlave::CJobSlave(ISlaveWatchdog *_watchdog, IPropertyTree *_workUnitInfo, co
     }
     tmpHandler.setown(createTempHandler(true));
 
-    float recommendReservePercentage = roxieMemPercentage;
-#ifndef _CONTAINERIZED
-    unsigned numWorkers = globals->getPropInt("@slavesPerNode", 1);
+    /*
+     * Calculate maximum recommeded memory for this worker.
+     * In container mode, there is only ever 1 worker per container,
+     * recommendedMaxPercentage = defaultPctSysMemForRoxie
+     * In bare-metal slavesPerNode is taken into account and @localThor if used.
+     * 
+     * recommendedMaxPercentage is used by applyMemorySettings to calculate the
+     * max amount of meemory that should be used (allowing enough left for heap/OS etc.)
+     */
+#ifdef _CONTAINERIZED
+    float recommendedMaxPercentage = defaultPctSysMemForRoxie;
+#else
+    // bare-metal only
+
+    float recommendedMaxPercentage = defaultPctSysMemForRoxie;
+    unsigned numWorkersPerNode = globals->getPropInt("@slavesPerNode", 1);
 
-    // Weird @localThor mode, where <roxieMemPercentage>(25%) of memory is reserved for OS, <roxieMemPercentage>(25%) is reserved for master, and reset fo slaves
+    // @localThor mode - 25% is used for manager and 50% is used for workers
     if (globals->getPropBool("@localThor") && (0 == globals->getPropInt("@masterMemorySize")))
     {
-        // reserve is <roxieMemPercentage>% (for OS) + <roxieMemPercentage>% (for manager) + [other workers * slave of rest(50%)]
-        recommendReservePercentage = roxieMemPercentage + roxieMemPercentage + ((numWorkers-1) * ((100-roxieMemPercentage-roxieMemPercentage) / ((float)numWorkers)));
+        /* In this mode, 25% is reserved for manager,
+         * 50% for the workers.
+         * Meaning this workers' recommendedMaxPercentage is remaining percentage */
+        recommendedMaxPercentage -= 25.0; // for manager
+        float pctPerWorker = 50.0 / numWorkersPerNode;
+        recommendedMaxPercentage -= pctPerWorker * (numWorkersPerNode-1);
     }
     else
-        recommendReservePercentage = roxieMemPercentage + ((numWorkers-1) * ((100-roxieMemPercentage) / ((float)numWorkers)));
+    {
+        // deduct percentage for all other workers from max percentage
+        float pctPerWorker = defaultPctSysMemForRoxie / numWorkersPerNode;
+        recommendedMaxPercentage -= pctPerWorker * (numWorkersPerNode-1);
+    }
 #endif
-    applyMemorySettings(recommendReservePercentage, "worker");
+    applyMemorySettings(recommendedMaxPercentage, "worker");
 
     unsigned sharedMemoryLimitPercentage = (unsigned)getWorkUnitValueInt("globalMemoryLimitPC", globals->getPropInt("@sharedMemoryLimit", 90));
     unsigned sharedMemoryMB = queryMemoryMB*sharedMemoryLimitPercentage/100;