Remove templating of interval/timeperiod/length

Summary:
We don't need to template the length of the ringbuffer since we're using a std::list and not a fixed-size array. Using templated lengths prevents us from changing the polling interval with runtime flags

Update the code to remove the template arguments

Reviewed By: harshitgulati18

Differential Revision: D60997263

fbshipit-source-id: abc46a3f9f99ad3aa24de3f423de767c73b81ad7

cmake/Agent.cmake

@@ -293,6 +293,7 @@ set(core_libs
   l2learn_event_observer
   agent_fsdb_sync_manager
   fboss_event_base
+  phy_snapshot_manager
 )
 
 target_link_libraries(core ${core_libs})

cmake/LibPhy.cmake

@@ -26,6 +26,7 @@ target_link_libraries(phy_management_base
   Folly::folly
   io_stats_cpp2
   fb303::fb303
+  phy_snapshot_manager
 )
 
 add_library(phy_utils

cmake/LibPhySnapshotManager.cmake

@@ -0,0 +1,14 @@
+# CMake to build libraries and binaries from fboss/agent/PhySnapshotManager
+
+# In general, libraries and binaries in fboss/foo/bar are built by
+# cmake/FooBar.cmake
+
+add_library(phy_snapshot_manager
+  fboss/agent/PhySnapshotManager.cpp
+)
+
+target_link_libraries(phy_snapshot_manager
+  phy_cpp2
+  snapshot_manager
+  state
+)

fboss/agent/BUCK

@@ -596,9 +596,9 @@ cpp_library(
 
 cpp_library(
     name = "phy_snapshot_lib",
+    srcs = ["PhySnapshotManager.cpp"],
     headers = [
         "PhySnapshotManager.h",
-        "PhySnapshotManager-defs.h",
     ],
     exported_deps = [
         "//fboss/agent/state:state",
@@ -788,7 +788,6 @@ cpp_library(
         "//fboss/lib:radix_tree",
         "//fboss/lib:thread_heartbeat",
         "//fboss/lib/config:fboss_config_utils",
-        "//fboss/lib/link_snapshots:snapshot_manager",
         "//fboss/lib/phy:phy-cpp2-types",
         "//fboss/lib/phy:prbs-cpp2-types",
         "//fboss/lib/platforms:product-info",

fboss/agent/PhySnapshotManager-defs.h → fboss/agent/PhySnapshotManager.cpp

@@ -1,14 +1,10 @@
 // (c) Facebook, Inc. and its affiliates. Confidential and proprietary.
 
-#pragma once
-
 #include "fboss/agent/PhySnapshotManager.h"
-#include "fboss/agent/state/Port.h"
 
 namespace facebook::fboss {
 
-template <size_t intervalSeconds>
-void PhySnapshotManager<intervalSeconds>::updatePhyInfoLocked(
+void PhySnapshotManager::updatePhyInfoLocked(
     const SnapshotMapWLockedPtr& lockedSnapshotMap,
     PortID portID,
     const phy::PhyInfo& phyInfo) {
@@ -18,32 +14,30 @@ void PhySnapshotManager<intervalSeconds>::updatePhyInfoLocked(
 
   CHECK(!phyInfo.state()->get_name().empty());
   auto result = lockedSnapshotMap->try_emplace(
-      portID, std::set<std::string>({phyInfo.state()->get_name()}));
+      portID,
+      std::set<std::string>({phyInfo.state()->get_name()}),
+      intervalSeconds_);
   auto iter = result.first;
   auto& value = iter->second;
   value.addSnapshot(snapshot);
 }
 
-template <size_t intervalSeconds>
-void PhySnapshotManager<intervalSeconds>::updatePhyInfo(
+void PhySnapshotManager::updatePhyInfo(
     PortID portID,
     const phy::PhyInfo& phyInfo) {
   auto lockedSnapshotMap = snapshots_.wlock();
   updatePhyInfoLocked(lockedSnapshotMap, portID, phyInfo);
 }
 
-template <size_t intervalSeconds>
-void PhySnapshotManager<intervalSeconds>::updatePhyInfos(
+void PhySnapshotManager::updatePhyInfos(
     const std::map<PortID, phy::PhyInfo>& phyInfos) {
   auto lockedSnapshotMap = snapshots_.wlock();
   for (const auto& [portID, phyInfo] : phyInfos) {
     updatePhyInfoLocked(lockedSnapshotMap, portID, phyInfo);
   }
 }
 
-template <size_t intervalSeconds>
-std::optional<phy::PhyInfo>
-PhySnapshotManager<intervalSeconds>::getPhyInfoLocked(
+std::optional<phy::PhyInfo> PhySnapshotManager::getPhyInfoLocked(
     const SnapshotMapRLockedPtr& lockedSnapshotMap,
     PortID portID) const {
   std::optional<phy::PhyInfo> phyInfo;
@@ -59,16 +53,13 @@ PhySnapshotManager<intervalSeconds>::getPhyInfoLocked(
   return phyInfo;
 }
 
-template <size_t intervalSeconds>
-std::optional<phy::PhyInfo> PhySnapshotManager<intervalSeconds>::getPhyInfo(
+std::optional<phy::PhyInfo> PhySnapshotManager::getPhyInfo(
     PortID portID) const {
   const auto& lockedSnapshotMap = snapshots_.rlock();
   return getPhyInfoLocked(lockedSnapshotMap, portID);
 }
 
-template <size_t intervalSeconds>
-std::map<PortID, const phy::PhyInfo>
-PhySnapshotManager<intervalSeconds>::getPhyInfos(
+std::map<PortID, const phy::PhyInfo> PhySnapshotManager::getPhyInfos(
     const std::vector<PortID>& portIDs) const {
   std::map<PortID, const phy::PhyInfo> infoMap;
   auto lockedSnapshotMap = snapshots_.rlock();
@@ -81,9 +72,8 @@ PhySnapshotManager<intervalSeconds>::getPhyInfos(
   return infoMap;
 }
 
-template <size_t intervalSeconds>
-std::map<PortID, const phy::PhyInfo>
-PhySnapshotManager<intervalSeconds>::getAllPhyInfos() const {
+std::map<PortID, const phy::PhyInfo> PhySnapshotManager::getAllPhyInfos()
+    const {
   std::map<PortID, const phy::PhyInfo> infoMap;
   auto lockedSnapshotMap = snapshots_.rlock();
   for (auto it = lockedSnapshotMap->begin(); it != lockedSnapshotMap->end();
@@ -96,8 +86,7 @@ PhySnapshotManager<intervalSeconds>::getAllPhyInfos() const {
   return infoMap;
 }
 
-template <size_t intervalSeconds>
-void PhySnapshotManager<intervalSeconds>::publishSnapshots(PortID port) {
+void PhySnapshotManager::publishSnapshots(PortID port) {
   auto lockedSnapshotMap = snapshots_.wlock();
   if (auto it = lockedSnapshotMap->find(port); it != lockedSnapshotMap->end()) {
     it->second.publishAllSnapshots();

fboss/agent/PhySnapshotManager.h

@@ -3,21 +3,20 @@
 #pragma once
 
 #include "fboss/agent/state/Port.h"
-#include "fboss/lib/link_snapshots/SnapshotManager-defs.h"
+#include "fboss/lib/link_snapshots/SnapshotManager.h"
 #include "fboss/lib/phy/gen-cpp2/phy_types.h"
 
 namespace facebook::fboss {
 
-// intervalSeconds is the interval between Phy stat collections
-template <size_t intervalSeconds>
 class PhySnapshotManager {
-  using PhySnapshotCache = SnapshotManager<intervalSeconds>;
   using SnapshotMapRLockedPtr = typename folly::Synchronized<
-      std::map<PortID, PhySnapshotCache>>::ConstRLockedPtr;
+      std::map<PortID, SnapshotManager>>::ConstRLockedPtr;
   using SnapshotMapWLockedPtr = typename folly::Synchronized<
-      std::map<PortID, PhySnapshotCache>>::WLockedPtr;
+      std::map<PortID, SnapshotManager>>::WLockedPtr;
 
  public:
+  explicit PhySnapshotManager(size_t intervalSeconds)
+      : intervalSeconds_(intervalSeconds) {}
   void updatePhyInfo(PortID portID, const phy::PhyInfo& phyInfo);
   void updatePhyInfos(const std::map<PortID, phy::PhyInfo>& phyInfo);
   std::optional<phy::PhyInfo> getPhyInfo(PortID portID) const;
@@ -36,7 +35,8 @@ class PhySnapshotManager {
       PortID portID) const;
 
   // Map of portID to last few phy diagnostic snapshots
-  folly::Synchronized<std::map<PortID, PhySnapshotCache>> snapshots_;
+  folly::Synchronized<std::map<PortID, SnapshotManager>> snapshots_;
+  size_t intervalSeconds_;
 };
 
 } // namespace facebook::fboss

fboss/agent/SwAgentInitializer.cpp

@@ -115,7 +115,7 @@ void SwSwitchInitializer::initThread(
 void SwSwitchInitializer::init(
     HwSwitchCallback* hwSwitchCallback,
     const HwWriteBehavior& hwWriteBehavior) {
-  auto startTime = steady_clock::now();
+  auto startTime = std::chrono::steady_clock::now();
   std::lock_guard<std::mutex> g(initLock_);
   initImpl(hwSwitchCallback, hwWriteBehavior);
   sw_->applyConfig("apply initial config");

fboss/agent/SwSwitch.cpp

@@ -53,7 +53,7 @@
 #include "fboss/agent/NeighborUpdater.h"
 #include "fboss/agent/PacketLogger.h"
 #include "fboss/agent/PacketObserver.h"
-#include "fboss/agent/PhySnapshotManager-defs.h"
+#include "fboss/agent/PhySnapshotManager.h"
 #include "fboss/agent/PortStats.h"
 #include "fboss/agent/PortUpdateHandler.h"
 #include "fboss/agent/ResolvedNexthopMonitor.h"
@@ -387,8 +387,7 @@ SwSwitch::SwSwitch(
       lookupClassRouteUpdater_(new LookupClassRouteUpdater(this)),
       staticL2ForNeighborObserver_(new StaticL2ForNeighborObserver(this)),
       macTableManager_(new MacTableManager(this)),
-      phySnapshotManager_(
-          new PhySnapshotManager<kIphySnapshotIntervalSeconds>()),
+      phySnapshotManager_(new PhySnapshotManager(kIphySnapshotIntervalSeconds)),
       aclNexthopHandler_(new AclNexthopHandler(this)),
       teFlowNextHopHandler_(new TeFlowNexthopHandler(this)),
       dsfSubscriber_(new DsfSubscriber(this)),

fboss/agent/SwSwitch.h

@@ -30,7 +30,6 @@
 #include "fboss/agent/types.h"
 #include "fboss/lib/HwWriteBehavior.h"
 #include "fboss/lib/ThreadHeartbeat.h"
-#include "fboss/lib/link_snapshots/SnapshotManager-defs.h"
 #include "fboss/lib/phy/gen-cpp2/phy_types.h"
 
 #include <folly/IntrusiveList.h>
@@ -75,7 +74,6 @@ class RouteUpdateLogger;
 class StateObserver;
 class TunManager;
 class MirrorManager;
-template <size_t interval>
 class PhySnapshotManager;
 class AclNexthopHandler;
 class LookupClassUpdater;
@@ -1224,8 +1222,7 @@ class SwSwitch : public HwSwitchCallback {
 
   static constexpr auto kIphySnapshotIntervalSeconds = 1;
 
-  std::unique_ptr<PhySnapshotManager<kIphySnapshotIntervalSeconds>>
-      phySnapshotManager_;
+  std::unique_ptr<PhySnapshotManager> phySnapshotManager_;
   std::unique_ptr<AclNexthopHandler> aclNexthopHandler_;
   folly::Synchronized<std::unique_ptr<FsdbSyncer>> fsdbSyncer_;
   std::unique_ptr<TeFlowNexthopHandler> teFlowNextHopHandler_;

fboss/agent/mnpu/SplitSwAgentInitializer.cpp

@@ -14,6 +14,8 @@
 #endif
 #endif
 
+using namespace std::chrono;
+
 namespace facebook::fboss {
 
 void SplitSwSwitchInitializer::initImpl(

fboss/agent/test/MKAServiceManagerTest.cpp

@@ -26,6 +26,7 @@
 using ::testing::_;
 using ::testing::AtLeast;
 using namespace facebook::fboss;
+using namespace std::chrono;
 
 namespace {
 #if FOLLY_HAS_COROUTINES

fboss/agent/test/MultiNodeLacpTests.cpp

@@ -36,6 +36,7 @@
 #include "folly/MacAddress.h"
 
 using namespace facebook::fboss;
+using namespace std::chrono;
 
 DECLARE_bool(enable_lacp);
 

fboss/agent/test/MultiNodeMacsecTests.cpp

@@ -21,6 +21,7 @@
 #include "fboss/mka_service/if/facebook/gen-cpp2/mka_config_types.h"
 
 using namespace facebook::fboss;
+using namespace std::chrono;
 
 using mka::Cak;
 using mka::MACSecCapability;

fboss/agent/test/MultiNodeOpenrTests.cpp

@@ -28,6 +28,7 @@
 #include "fboss/agent/RouteUpdateWrapper.h"
 
 using namespace facebook::fboss;
+using namespace std::chrono;
 
 static constexpr int kOpenrThriftPort{2018};
 

fboss/agent/test/agent_hw_tests/AgentPacketSendTests.cpp

@@ -13,6 +13,8 @@
 
 #include <chrono>
 
+using namespace std::chrono;
+
 namespace {
 constexpr int kAggId{500};
 } // unnamed namespace

fboss/agent/test/integration_tests/AgentBgpIntegrationTests.cpp

@@ -40,6 +40,7 @@ auto constexpr kTestPrefixLength = 120;
 
 using namespace facebook::neteng::fboss::bgp::thrift;
 using namespace facebook::neteng::fboss::bgp_attr;
+using namespace std::chrono;
 
 using StateUpdateFn = facebook::fboss::SwSwitch::StateUpdateFn;
 

fboss/agent/test/integration_tests/TeAgentIntegrationTests.cpp

@@ -41,6 +41,7 @@ static int kTeFlowEntries(9000);
 } // namespace
 
 using namespace facebook::neteng::ai;
+using namespace std::chrono;
 
 using StateUpdateFn = facebook::fboss::SwSwitch::StateUpdateFn;
 

fboss/agent/test/soak_tests/SoakTest.cpp

@@ -11,6 +11,8 @@ DEFINE_string(
     "1s",
     "Total time to run soak test, <\\d+>[smhd], s-sec, m-min, h-hour, d-day");
 
+using namespace std::chrono;
+
 namespace facebook::fboss {
 
 uint64_t SoakTest::SoakTimeStrToSeconds(std::string timeStr) {

fboss/lib/link_snapshots/BUCK

@@ -22,7 +22,6 @@ cpp_library(
     ],
     headers = [
         "SnapshotManager.h",
-        "SnapshotManager-defs.h",
     ],
     exported_deps = [
         ":ring_buffer",

fboss/lib/link_snapshots/RingBuffer-defs.h

@@ -14,57 +14,55 @@
 
 namespace facebook::fboss {
 
-template <typename T, size_t length>
-void RingBuffer<T, length>::write(T val) {
-  if (buf.size() == length) {
+template <typename T>
+void RingBuffer<T>::write(T val) {
+  if (buf.size() == maxLength_) {
     buf.pop_front();
   }
   buf.push_back(val);
 }
 
-template <typename T, size_t length>
-const T RingBuffer<T, length>::last() const {
+template <typename T>
+const T RingBuffer<T>::last() const {
   if (buf.empty()) {
     throw FbossError("Attempted to read from empty RingBuffer");
   }
   return buf.back();
 }
 
-template <typename T, size_t length>
-bool RingBuffer<T, length>::empty() const {
+template <typename T>
+bool RingBuffer<T>::empty() const {
   return buf.empty();
 }
 
-template <typename T, size_t length>
-typename RingBuffer<T, length>::iterator RingBuffer<T, length>::begin() {
+template <typename T>
+typename RingBuffer<T>::iterator RingBuffer<T>::begin() {
   return buf.begin();
 }
 
-template <typename T, size_t length>
-typename RingBuffer<T, length>::iterator RingBuffer<T, length>::end() {
+template <typename T>
+typename RingBuffer<T>::iterator RingBuffer<T>::end() {
   return buf.end();
 }
 
-template <typename T, size_t length>
-typename RingBuffer<T, length>::const_iterator RingBuffer<T, length>::begin()
-    const {
+template <typename T>
+typename RingBuffer<T>::const_iterator RingBuffer<T>::begin() const {
   return buf.begin();
 }
 
-template <typename T, size_t length>
-typename RingBuffer<T, length>::const_iterator RingBuffer<T, length>::end()
-    const {
+template <typename T>
+typename RingBuffer<T>::const_iterator RingBuffer<T>::end() const {
   return buf.end();
 }
 
-template <typename T, size_t length>
-size_t RingBuffer<T, length>::size() const {
+template <typename T>
+size_t RingBuffer<T>::size() const {
   return buf.size();
 }
 
-template <typename T, size_t length>
-size_t RingBuffer<T, length>::maxSize() const {
-  return length;
+template <typename T>
+size_t RingBuffer<T>::maxSize() const {
+  return maxLength_;
 }
 
 } // namespace facebook::fboss