Skip to content

Commit

Permalink
Use pointer to member function to reduce the footprint of collectCoun…
Browse files Browse the repository at this point in the history 
…ters() (sonic-net#488)

Each time a new counter/attribute type is added to flex counter infrastructure, collectCounters() is tailed with the logic of collecting the stats for the new type. As more and more possible id lists are being added to the flex counter, the collectCounters() member function is becoming bulky to check against all possible id lists, both existing and newly added ones.

Current list is already significantly long---port counters, queue counters, queue attributes, pg counters, pg attributes, and buffer pool counters. And most of them are irrelevant to a particular flex counter instance thread. E.g., For buffer pool watermark thread, only buffer pool counters matter, not the rest, port counters, queue counters, queue attributes, pg counters, pg attributes.
Improvement:

Separate each counter/attribute type to be a member function of FlexCounter with the same function prototype.

Have a data member (unordered_map) to maintain pointers to member functions for each flex counter instance, and install only the essential counter collection member functions at the counter/attribute list set operation.

Choice of data member structure: Originally considered unordered_set with element T to be a pointer to member function. However, the element type does not support a certain operations, and thus can not be taken as a key, but only as a value.
In doing so, we can avoid checking against irrelevant counter/attribute id lists, and keep a fixed size for collectCounters function.

Signed-off-by: Wenda Ni <wenni@microsoft.com>
  • Loading branch information
@wendani @lguohan
wendani authored and lguohan committed Aug 1, 2019
1 parent 6204031 commit 54725e7
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 2 deletions.
102 changes: 100 additions & 2 deletions syncd/syncd_flex_counter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,20 @@ void FlexCounter::updateFlexCounterStatsMode(
}
}

void FlexCounter::addCollectCountersHandler(const std::string &key, const collect_counters_handler_t &handler)
{
SWSS_LOG_ENTER();

m_collectCountersHandlers.emplace(key, handler);
}

void FlexCounter::removeCollectCountersHandler(const std::string &key)
{
SWSS_LOG_ENTER();

m_collectCountersHandlers.erase(key);
}

/* The current implementation of 'setPortCounterList' and 'setQueueCounterList' are
* not the same. Need to refactor these two functions to have the similar logic.
* Either the full SAI attributes are queried once, or each of the needed counters
Expand Down Expand Up @@ -188,6 +202,8 @@ void FlexCounter::setPortCounterList(
auto portCounterIds = std::make_shared<PortCounterIds>(portId, supportedIds);
fc.m_portCounterIdsMap.emplace(portVid, portCounterIds);

fc.addCollectCountersHandler(PORT_COUNTER_ID_LIST, &FlexCounter::collectPortCounters);

// Start flex counter thread in case it was not running due to empty counter IDs map
if (fc.m_pollInterval > 0)
{
Expand Down Expand Up @@ -263,6 +279,8 @@ void FlexCounter::setQueueCounterList(
auto queueCounterIds = std::make_shared<QueueCounterIds>(queueId, supportedIds);
fc.m_queueCounterIdsMap.emplace(queueVid, queueCounterIds);

fc.addCollectCountersHandler(QUEUE_COUNTER_ID_LIST, &FlexCounter::collectQueueCounters);

// Start flex counter thread in case it was not running due to empty counter IDs map
if (fc.m_pollInterval > 0)
{
Expand Down Expand Up @@ -292,6 +310,8 @@ void FlexCounter::setQueueAttrList(
auto queueAttrIds = std::make_shared<QueueAttrIds>(queueId, attrIds);
fc.m_queueAttrIdsMap.emplace(queueVid, queueAttrIds);

fc.addCollectCountersHandler(QUEUE_ATTR_ID_LIST, &FlexCounter::collectQueueAttrs);

// Start flex counter thread in case it was not running due to empty counter IDs map
if (fc.m_pollInterval > 0)
{
Expand Down Expand Up @@ -367,6 +387,8 @@ void FlexCounter::setPriorityGroupCounterList(
auto priorityGroupCounterIds = std::make_shared<IngressPriorityGroupCounterIds>(priorityGroupId, supportedIds);
fc.m_priorityGroupCounterIdsMap.emplace(priorityGroupVid, priorityGroupCounterIds);

fc.addCollectCountersHandler(PG_COUNTER_ID_LIST, &FlexCounter::collectPriorityGroupCounters);

// Start flex counter thread in case it was not running due to empty counter IDs map
if (fc.m_pollInterval > 0)
{
Expand Down Expand Up @@ -396,6 +418,8 @@ void FlexCounter::setPriorityGroupAttrList(
auto priorityGroupAttrIds = std::make_shared<IngressPriorityGroupAttrIds>(priorityGroupId, attrIds);
fc.m_priorityGroupAttrIdsMap.emplace(priorityGroupVid, priorityGroupAttrIds);

fc.addCollectCountersHandler(PG_ATTR_ID_LIST, &FlexCounter::collectPriorityGroupAttrs);

// Start flex counter thread in case it was not running due to empty counter IDs map
if (fc.m_pollInterval > 0)
{
Expand Down Expand Up @@ -454,6 +478,8 @@ void FlexCounter::setRifCounterList(
auto rifCounterIds = std::make_shared<RifCounterIds>(rifId, supportedIds);
fc.m_rifCounterIdsMap.emplace(rifVid, rifCounterIds);

fc.addCollectCountersHandler(RIF_COUNTER_ID_LIST, &FlexCounter::collectRifCounters);

// Start flex counter thread in case it was not running due to empty counter IDs map
if (fc.m_pollInterval > 0)
{
Expand Down Expand Up @@ -523,6 +549,8 @@ void FlexCounter::setBufferPoolCounterList(
auto bufferPoolCounterIds = std::make_shared<BufferPoolCounterIds>(bufferPoolId, supportedIds, bufferPoolStatsMode);
fc.m_bufferPoolCounterIdsMap.emplace(bufferPoolVid, bufferPoolCounterIds);

fc.addCollectCountersHandler(BUFFER_POOL_COUNTER_ID_LIST, &FlexCounter::collectBufferPoolCounters);

// Start flex counter thread in case it was not running due to empty counter IDs map
if (fc.m_pollInterval > 0)
{
Expand All @@ -544,6 +572,7 @@ void FlexCounter::removePort(
if (it == fc.m_portCounterIdsMap.end())
{
SWSS_LOG_NOTICE("Trying to remove nonexisting port counter Ids 0x%lx", portVid);

// Remove flex counter if all counter IDs and plugins are unregistered
if (fc.isEmpty())
{
Expand All @@ -554,6 +583,10 @@ void FlexCounter::removePort(
}

fc.m_portCounterIdsMap.erase(it);
if (fc.m_portCounterIdsMap.empty())
{
fc.removeCollectCountersHandler(PORT_COUNTER_ID_LIST);
}

// Remove flex counter if all counter IDs and plugins are unregistered
if (fc.isEmpty())
Expand All @@ -578,13 +611,21 @@ void FlexCounter::removeQueue(
if (counterIter != fc.m_queueCounterIdsMap.end())
{
fc.m_queueCounterIdsMap.erase(counterIter);
if (fc.m_queueCounterIdsMap.empty())
{
fc.removeCollectCountersHandler(QUEUE_COUNTER_ID_LIST);
}
found = true;
}

auto attrIter = fc.m_queueAttrIdsMap.find(queueVid);
if (attrIter != fc.m_queueAttrIdsMap.end())
{
fc.m_queueAttrIdsMap.erase(attrIter);
if (fc.m_queueAttrIdsMap.empty())
{
fc.removeCollectCountersHandler(QUEUE_ATTR_ID_LIST);
}
found = true;
}

Expand Down Expand Up @@ -617,13 +658,21 @@ void FlexCounter::removePriorityGroup(
if (counterIter != fc.m_priorityGroupCounterIdsMap.end())
{
fc.m_priorityGroupCounterIdsMap.erase(counterIter);
if (fc.m_priorityGroupCounterIdsMap.empty())
{
fc.removeCollectCountersHandler(PG_COUNTER_ID_LIST);
}
found = true;
}

auto attrIter = fc.m_priorityGroupAttrIdsMap.find(priorityGroupVid);
if (attrIter != fc.m_priorityGroupAttrIdsMap.end())
{
fc.m_priorityGroupAttrIdsMap.erase(attrIter);
if (fc.m_priorityGroupAttrIdsMap.empty())
{
fc.removeCollectCountersHandler(PG_ATTR_ID_LIST);
}
found = true;
}

Expand Down Expand Up @@ -655,6 +704,7 @@ void FlexCounter::removeRif(
if (it == fc.m_rifCounterIdsMap.end())
{
SWSS_LOG_NOTICE("Trying to remove nonexisting router interface counter from Id 0x%lx", rifVid);

// Remove flex counter if all counter IDs and plugins are unregistered
if (fc.isEmpty())
{
Expand All @@ -665,6 +715,10 @@ void FlexCounter::removeRif(
}

fc.m_rifCounterIdsMap.erase(it);
if (fc.m_rifCounterIdsMap.empty())
{
fc.removeCollectCountersHandler(RIF_COUNTER_ID_LIST);
}

// Remove flex counter if all counter IDs and plugins are unregistered
if (fc.isEmpty())
Expand All @@ -689,6 +743,10 @@ void FlexCounter::removeBufferPool(
if (it != fc.m_bufferPoolCounterIdsMap.end())
{
fc.m_bufferPoolCounterIdsMap.erase(it);
if (fc.m_bufferPoolCounterIdsMap.empty())
{
fc.removeCollectCountersHandler(BUFFER_POOL_COUNTER_ID_LIST);
}
found = true;
}

Expand Down Expand Up @@ -943,6 +1001,18 @@ void FlexCounter::collectCounters(
{
SWSS_LOG_ENTER();

for (const auto &it : m_collectCountersHandlers)
{
(this->*(it.second))(countersTable);
}

countersTable.flush();
}

void FlexCounter::collectPortCounters(_In_ swss::Table &countersTable)
{
SWSS_LOG_ENTER();

// Collect stats for every registered port
for (const auto &kv: m_portCounterIdsMap)
{
Expand Down Expand Up @@ -978,6 +1048,11 @@ void FlexCounter::collectCounters(

countersTable.set(portVidStr, values, "");
}
}

void FlexCounter::collectQueueCounters(_In_ swss::Table &countersTable)
{
SWSS_LOG_ENTER();

// Collect stats for every registered queue
for (const auto &kv: m_queueCounterIdsMap)
Expand Down Expand Up @@ -1034,6 +1109,11 @@ void FlexCounter::collectCounters(

countersTable.set(queueVidStr, values, "");
}
}

void FlexCounter::collectQueueAttrs(_In_ swss::Table &countersTable)
{
SWSS_LOG_ENTER();

// Collect attrs for every registered queue
for (const auto &kv: m_queueAttrIdsMap)
Expand Down Expand Up @@ -1076,6 +1156,11 @@ void FlexCounter::collectCounters(

countersTable.set(queueVidStr, values, "");
}
}

void FlexCounter::collectPriorityGroupCounters(_In_ swss::Table &countersTable)
{
SWSS_LOG_ENTER();

// Collect stats for every registered ingress priority group
for (const auto &kv: m_priorityGroupCounterIdsMap)
Expand Down Expand Up @@ -1125,6 +1210,11 @@ void FlexCounter::collectCounters(

countersTable.set(priorityGroupVidStr, values, "");
}
}

void FlexCounter::collectPriorityGroupAttrs(_In_ swss::Table &countersTable)
{
SWSS_LOG_ENTER();

// Collect attrs for every registered priority group
for (const auto &kv: m_priorityGroupAttrIdsMap)
Expand Down Expand Up @@ -1167,6 +1257,11 @@ void FlexCounter::collectCounters(

countersTable.set(priorityGroupVidStr, values, "");
}
}

void FlexCounter::collectRifCounters(_In_ swss::Table &countersTable)
{
SWSS_LOG_ENTER();

// Collect stats for every registered router interface
for (const auto &kv: m_rifCounterIdsMap)
Expand Down Expand Up @@ -1203,6 +1298,11 @@ void FlexCounter::collectCounters(

countersTable.set(rifVidStr, values, "");
}
}

void FlexCounter::collectBufferPoolCounters(_In_ swss::Table &countersTable)
{
SWSS_LOG_ENTER();

// Collect stats for every registered buffer pool
for (const auto &it : m_bufferPoolCounterIdsMap)
Expand Down Expand Up @@ -1259,8 +1359,6 @@ void FlexCounter::collectCounters(

countersTable.set(sai_serialize_object_id(bufferPoolVid), fvTuples);
}

countersTable.flush();
}

void FlexCounter::runPlugins(
Expand Down
17 changes: 17 additions & 0 deletions syncd/syncd_flex_counter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ extern "C" {
#include <vector>
#include <set>
#include <condition_variable>
#include <unordered_map>
#include "swss/table.h"

class FlexCounter
Expand Down Expand Up @@ -198,6 +199,20 @@ class FlexCounter
bool allIdsEmpty();
bool allPluginsEmpty();

typedef void (FlexCounter::*collect_counters_handler_t)(_In_ swss::Table &countersTable);
typedef std::unordered_map<std::string, collect_counters_handler_t> collect_counters_handler_unordered_map_t;

void collectPortCounters(_In_ swss::Table &countersTable);
void collectQueueCounters(_In_ swss::Table &countersTable);
void collectQueueAttrs(_In_ swss::Table &countersTable);
void collectPriorityGroupCounters(_In_ swss::Table &countersTable);
void collectPriorityGroupAttrs(_In_ swss::Table &countersTable);
void collectRifCounters(_In_ swss::Table &countersTable);
void collectBufferPoolCounters(_In_ swss::Table &countersTable);

void addCollectCountersHandler(const std::string &key, const collect_counters_handler_t &handler);
void removeCollectCountersHandler(const std::string &key);

// Key is a Virtual ID
std::map<sai_object_id_t, std::shared_ptr<PortCounterIds>> m_portCounterIdsMap;
std::map<sai_object_id_t, std::shared_ptr<QueueCounterIds>> m_queueCounterIdsMap;
Expand All @@ -224,6 +239,8 @@ class FlexCounter
std::string m_instanceId;
sai_stats_mode_t m_statsMode;
bool m_enable = false;

collect_counters_handler_unordered_map_t m_collectCountersHandlers;
};

#endif

0 comments on commit 54725e7

Please sign in to comment.