From a08cb4fe19475faeeb2e8be3829c7f5310cf2f01 Mon Sep 17 00:00:00 2001
From: Brad House <brad@brad-house.com>
Date: Wed, 15 Jan 2025 08:42:10 -0500
Subject: [PATCH] lazily cache lagMembers

---
 cfgmgr/portmgr.cpp | 56 ++++++++++++++++++++++++++++------------------
 cfgmgr/portmgr.h   |  4 ++--
 2 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp
index 7ccf616caf..d72420d9bf 100644
--- a/cfgmgr/portmgr.cpp
+++ b/cfgmgr/portmgr.cpp
@@ -7,6 +7,7 @@
 #include "exec.h"
 #include "shellcmd.h"
 #include <swss/redisutility.h>
+#include <unordered_map>
 
 using namespace std;
 using namespace swss;
@@ -22,30 +23,40 @@ PortMgr::PortMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c
 {
 }
 
-bool PortMgr::isLagMember(const std::string &alias, const vector<string> &lagMembers)
+bool PortMgr::isLagMember(const std::string &alias, std::unordered_map<std::string, int> &lagMembers)
 {
-    /* Lag members are cached in doTask() as an optimization for high port
-     * count switches.
-     *  vector<string> lagMembers;
-     *  m_cfgLagMemberTable.getKeys(lagMembers);
+    /* Lag members are lazily loaded on the first call to isLagMember and cached
+     * within a variable inside of doTask() for future calls.
      */
-
-    for (auto key: keys)
+    if (lagMembers.empty())
     {
-        auto tokens = tokenize(key, config_db_key_delimiter);
-        auto lag = tokens[0];
-        auto member = tokens[1];
-
-        if (alias == member)
+        vector<string> keys;
+        m_cfgLagMemberTable.getKeys(keys);
+        for (auto key: keys)
         {
-            return true;
+            auto tokens = tokenize(key, config_db_key_delimiter);
+            std::string member = tokens[1];
+            if (!member.empty()) {
+                lagMembers[member] = 1;
+            }
         }
+
+        /* placeholder to state we already read lagmembers even though there are
+         * none */
+        if (lagMembers.empty()) {
+            lagMembers["none"] = 1;
+        }
+    }
+
+    if (lagMembers.find(alias) != lagMembers.end())
+    {
+        return true;
     }
 
     return false;
 }
 
-bool PortMgr::setPortMtu(const string &alias, const string &mtu, const vector<string> &lagMembers)
+bool PortMgr::setPortMtu(const string &alias, const string &mtu, std::unordered_map<std::string, int> &lagMembers)
 {
     stringstream cmd;
     string res, cmd_str;
@@ -66,7 +77,7 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu, const vector<st
         SWSS_LOG_WARN("Setting mtu to alias:%s netdev failed with cmd:%s, rc:%d, error:%s", alias.c_str(), cmd_str.c_str(), ret, res.c_str());
         return false;
     }
-    else if (isLagMember(lagMembers, alias))
+    else if (isLagMember(alias, lagMembers))
     {
         // Catch user improperly specified an MTU on the PortChannel
         SWSS_LOG_WARN("Setting mtu to alias:%s which is a member of a PortChannel is invalid", alias.c_str());
@@ -162,6 +173,12 @@ void PortMgr::doTask(Consumer &consumer)
 {
     SWSS_LOG_ENTER();
 
+    /* Variable to lazily cache lag members upon first call into isLagMember(). We
+     * don't want to always query for lag members if not needed, and we also don't
+     * want to query it for each call to isLagMember() which may be on every port.
+     */
+    std::unordered_map<std::string, int> lagMembers;
+
     auto table = consumer.getTableName();
     if (table == CFG_SEND_TO_INGRESS_PORT_TABLE_NAME)
     {
@@ -169,11 +186,6 @@ void PortMgr::doTask(Consumer &consumer)
         return;
     }
 
-    /* Cache lag members outside of task iteration as optimization for large
-     * port count devices rather than doing it within isLagMember() */
-    vector<string> lagMembers;
-    m_cfgLagMemberTable.getKeys(lagMembers);
-
     auto it = consumer.m_toSync.begin();
     while (it != consumer.m_toSync.end())
     {
@@ -229,7 +241,7 @@ void PortMgr::doTask(Consumer &consumer)
                     field_values.emplace_back("admin_status", admin_status);
                 }
 
-                if (mtu.empty() && !isLagMember(lagMembers, alias))
+                if (mtu.empty() && !isLagMember(alias, lagMembers))
                 {
                     mtu = DEFAULT_MTU_STR;
                     field_values.emplace_back("mtu", mtu);
@@ -257,7 +269,7 @@ void PortMgr::doTask(Consumer &consumer)
             if (!mtu.empty())
             {
                 SWSS_LOG_NOTICE("Configure %s MTU to %s", alias.c_str(), mtu.c_str());
-                setPortMtu(lagMembers, alias, mtu);
+                setPortMtu(alias, mtu, lagMembers);
             }
 
             if (!admin_status.empty())
diff --git a/cfgmgr/portmgr.h b/cfgmgr/portmgr.h
index 5c6429efff..a35795b07c 100644
--- a/cfgmgr/portmgr.h
+++ b/cfgmgr/portmgr.h
@@ -34,10 +34,10 @@ class PortMgr : public Orch
     void doSendToIngressPortTask(Consumer &consumer);
     bool writeConfigToAppDb(const std::string &alias, const std::string &field, const std::string &value);
     bool writeConfigToAppDb(const std::string &alias, std::vector<FieldValueTuple> &field_values);
-    bool setPortMtu(const std::string &alias, const std::string &mtu, const vector<string> &lagMembers);
+    bool setPortMtu(const std::string &alias, const std::string &mtu, std::unordered_map<std::string, int> &lagMembers);
     bool setPortAdminStatus(const std::string &alias, const bool up);
     bool isPortStateOk(const std::string &alias);
-    bool isLagMember(const std::string &alias, const vector<string> &lagMembers);
+    bool isLagMember(const std::string &alias, std::unordered_map<std::string, int> &lagMembers);
 };
 
 }