From 63bf401f83d7c9955d6c2f94511273d2d327d11f Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Mon, 2 Apr 2018 18:40:36 -0700 Subject: [PATCH 01/13] Refactor select and selectable. Now they support priority --- common/consumerstatetable.cpp | 4 +- common/consumerstatetable.h | 2 +- common/consumertable.cpp | 4 +- common/consumertable.h | 2 +- common/consumertablebase.cpp | 4 +- common/consumertablebase.h | 2 +- common/ipaddress.h | 2 +- common/ipprefix.cpp | 5 +- common/logger.cpp | 6 +- common/logger.h | 6 +- common/netlink.cpp | 25 ++-- common/netlink.h | 8 +- common/notificationconsumer.cpp | 98 +++++++------- common/notificationconsumer.h | 9 +- common/rediscommand.cpp | 1 + common/redispipeline.h | 2 +- common/redisselect.cpp | 58 ++++---- common/redisselect.h | 14 +- common/select.cpp | 167 ++++++++++++++--------- common/select.h | 30 +++-- common/selectable.h | 53 ++++++-- common/selectableevent.cpp | 20 +-- common/selectableevent.h | 8 +- common/selectabletimer.cpp | 20 +-- common/selectabletimer.h | 8 +- common/subscriberstatetable.cpp | 56 ++++---- common/subscriberstatetable.h | 11 +- common/table.h | 2 +- tests/Makefile.am | 9 +- tests/redis_piped_state_ut.cpp | 4 +- tests/redis_piped_ut.cpp | 2 +- tests/redis_state_ut.cpp | 4 +- tests/redis_ut.cpp | 2 +- tests/selectable_priority.cpp | 227 ++++++++++++++++++++++++++++++++ 34 files changed, 578 insertions(+), 297 deletions(-) create mode 100644 tests/selectable_priority.cpp diff --git a/common/consumerstatetable.cpp b/common/consumerstatetable.cpp index 3768646c0..15d74d003 100644 --- a/common/consumerstatetable.cpp +++ b/common/consumerstatetable.cpp @@ -11,8 +11,8 @@ namespace swss { -ConsumerStateTable::ConsumerStateTable(DBConnector *db, std::string tableName, int popBatchSize) - : ConsumerTableBase(db, tableName, popBatchSize) +ConsumerStateTable::ConsumerStateTable(DBConnector *db, std::string tableName, int popBatchSize, int pri) + : ConsumerTableBase(db, tableName, popBatchSize, pri) , TableName_KeySet(tableName) { for (;;) diff --git a/common/consumerstatetable.h b/common/consumerstatetable.h index bf1f1db7e..2fd954ee8 100644 --- a/common/consumerstatetable.h +++ b/common/consumerstatetable.h @@ -10,7 +10,7 @@ namespace swss { class ConsumerStateTable : public ConsumerTableBase, public TableName_KeySet { public: - ConsumerStateTable(DBConnector *db, std::string tableName, int popBatchSize = DEFAULT_POP_BATCH_SIZE); + ConsumerStateTable(DBConnector *db, std::string tableName, int popBatchSize = DEFAULT_POP_BATCH_SIZE, int pri = 0); /* Get multiple pop elements */ void pops(std::deque &vkco, std::string prefix = EMPTY_PREFIX); diff --git a/common/consumertable.cpp b/common/consumertable.cpp index 995af697b..0ac8bbbb2 100644 --- a/common/consumertable.cpp +++ b/common/consumertable.cpp @@ -14,8 +14,8 @@ using namespace std; namespace swss { -ConsumerTable::ConsumerTable(DBConnector *db, string tableName, int popBatchSize) - : ConsumerTableBase(db, tableName, popBatchSize) +ConsumerTable::ConsumerTable(DBConnector *db, string tableName, int popBatchSize, int pri) + : ConsumerTableBase(db, tableName, popBatchSize, pri) , TableName_KeyValueOpQueues(tableName) { for (;;) diff --git a/common/consumertable.h b/common/consumertable.h index 70a4b7d81..94a00be59 100644 --- a/common/consumertable.h +++ b/common/consumertable.h @@ -13,7 +13,7 @@ namespace swss { class ConsumerTable : public ConsumerTableBase, public TableName_KeyValueOpQueues { public: - ConsumerTable(DBConnector *db, std::string tableName, int popBatchSize = DEFAULT_POP_BATCH_SIZE); + ConsumerTable(DBConnector *db, std::string tableName, int popBatchSize = DEFAULT_POP_BATCH_SIZE, int pri = 0); /* Get multiple pop elements */ void pops(std::deque &vkco, std::string prefix = EMPTY_PREFIX); diff --git a/common/consumertablebase.cpp b/common/consumertablebase.cpp index 530f4578f..77f96d19f 100644 --- a/common/consumertablebase.cpp +++ b/common/consumertablebase.cpp @@ -2,8 +2,8 @@ namespace swss { -ConsumerTableBase::ConsumerTableBase(DBConnector *db, std::string tableName, int popBatchSize): - TableConsumable(tableName), +ConsumerTableBase::ConsumerTableBase(DBConnector *db, std::string tableName, int popBatchSize, int pri): + TableConsumable(tableName, pri), RedisTransactioner(db), POP_BATCH_SIZE(popBatchSize) { diff --git a/common/consumertablebase.h b/common/consumertablebase.h index 5580dd413..10979b8cd 100644 --- a/common/consumertablebase.h +++ b/common/consumertablebase.h @@ -10,7 +10,7 @@ class ConsumerTableBase: public TableConsumable, public RedisTransactioner public: const int POP_BATCH_SIZE; - ConsumerTableBase(DBConnector *db, std::string tableName, int popBatchSize = DEFAULT_POP_BATCH_SIZE); + ConsumerTableBase(DBConnector *db, std::string tableName, int popBatchSize = DEFAULT_POP_BATCH_SIZE, int pri = 0); virtual ~ConsumerTableBase(); diff --git a/common/ipaddress.h b/common/ipaddress.h index ccb370e4b..7ad5295d6 100644 --- a/common/ipaddress.h +++ b/common/ipaddress.h @@ -20,7 +20,7 @@ class IpAddress { public: IpAddress() {} - IpAddress(const ip_addr_t &ip) { m_ip = ip; } + IpAddress(const ip_addr_t &ip) : m_ip(ip) {} IpAddress(uint32_t ip); IpAddress(const std::string &ipStr); diff --git a/common/ipprefix.cpp b/common/ipprefix.cpp index 7e06091c8..56fe96258 100644 --- a/common/ipprefix.cpp +++ b/common/ipprefix.cpp @@ -45,10 +45,9 @@ IpPrefix::IpPrefix( } } -IpPrefix::IpPrefix(uint32_t ipPrefix, int mask) +IpPrefix::IpPrefix(uint32_t ipPrefix, int mask) : m_ip(ipPrefix), + m_mask(mask) { - m_ip = IpAddress(ipPrefix); - m_mask = mask; if (!isValid()) { throw std::invalid_argument("Invalid IpPrefix from prefix and mask"); diff --git a/common/logger.cpp b/common/logger.cpp index 13429d1ea..018e2d855 100644 --- a/common/logger.cpp +++ b/common/logger.cpp @@ -70,7 +70,7 @@ void Logger::swssOutputNotify(std::string component, std::string outputStr) } } -void Logger::linkToDbWithOutput(const std::string dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio, const OutputChangeNotify& outputNotify, const std::string& defOutput) +void Logger::linkToDbWithOutput(const std::string& dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio, const OutputChangeNotify& outputNotify, const std::string& defOutput) { auto& logger = getInstance(); @@ -122,12 +122,12 @@ void Logger::linkToDbWithOutput(const std::string dbName, const PriorityChangeNo outputNotify(key, output); } -void Logger::linkToDb(const std::string dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio) +void Logger::linkToDb(const std::string& dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio) { linkToDbWithOutput(dbName, prioNotify, defPrio, swssOutputNotify, "SYSLOG"); } -void Logger::linkToDbNative(const std::string dbName) +void Logger::linkToDbNative(const std::string& dbName) { auto& logger = getInstance(); diff --git a/common/logger.h b/common/logger.h index 664d24deb..8b0933fab 100644 --- a/common/logger.h +++ b/common/logger.h @@ -55,10 +55,10 @@ class Logger static Logger &getInstance(); static void setMinPrio(Priority prio); static Priority getMinPrio(); - static void linkToDbWithOutput(const std::string dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio, const OutputChangeNotify& outputNotify, const std::string& defOutput); - static void linkToDb(const std::string dbName, const PriorityChangeNotify& notify, const std::string& defPrio); + static void linkToDbWithOutput(const std::string& dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio, const OutputChangeNotify& outputNotify, const std::string& defOutput); + static void linkToDb(const std::string& dbName, const PriorityChangeNotify& notify, const std::string& defPrio); // Must be called after all linkToDb to start select from DB - static void linkToDbNative(const std::string dbName); + static void linkToDbNative(const std::string& dbName); void write(Priority prio, const char *fmt, ...) #ifdef __GNUC__ __attribute__ ((format (printf, 3, 4))) diff --git a/common/netlink.cpp b/common/netlink.cpp index e761770db..6940de3e2 100644 --- a/common/netlink.cpp +++ b/common/netlink.cpp @@ -9,8 +9,8 @@ using namespace swss; using namespace std; -NetLink::NetLink() : - m_socket(NULL) +NetLink::NetLink(int pri) : + Selectable(pri), m_socket(NULL) { m_socket = nl_socket_alloc(); if (!m_socket) @@ -71,24 +71,21 @@ void NetLink::dumpRequest(int rtmGetCommand) } } -void NetLink::addFd(fd_set *fd) +int NetLink::getFd() { - FD_SET(nl_socket_get_fd(m_socket), fd); + return nl_socket_get_fd(m_socket); } -bool NetLink::isMe(fd_set *fd) +void NetLink::readData() { - return FD_ISSET(nl_socket_get_fd(m_socket), fd); -} + int err; -int NetLink::readCache() -{ - return NODATA; -} + do + { + err = nl_recvmsgs_default(m_socket); + } + while(err == -EINTR); // Retry if the process was interrupted by a signal -void NetLink::readMe() -{ - int err = nl_recvmsgs_default(m_socket); if (err < 0) { if (err == -NLE_NOMEM) diff --git a/common/netlink.h b/common/netlink.h index 4ff1614e9..3cdb9d788 100644 --- a/common/netlink.h +++ b/common/netlink.h @@ -9,16 +9,14 @@ namespace swss { class NetLink : public Selectable { public: - NetLink(); + NetLink(int pri = 0); virtual ~NetLink(); void registerGroup(int rtnlGroup); void dumpRequest(int rtmGetCommand); - virtual void addFd(fd_set *fd); - virtual bool isMe(fd_set *fd); - virtual int readCache(); - virtual void readMe(); + int getFd() override; + void readData() override; private: static int onNetlinkMsg(struct nl_msg *msg, void *arg); diff --git a/common/notificationconsumer.cpp b/common/notificationconsumer.cpp index 3bd8694ef..0faec47d3 100644 --- a/common/notificationconsumer.cpp +++ b/common/notificationconsumer.cpp @@ -6,7 +6,8 @@ #define REDIS_PUBLISH_MESSAGE_INDEX (2) #define REDIS_PUBLISH_MESSAGE_ELEMNTS (3) -swss::NotificationConsumer::NotificationConsumer(swss::DBConnector *db, std::string channel): +swss::NotificationConsumer::NotificationConsumer(swss::DBConnector *db, std::string channel, int pri): + Selectable(pri), m_db(db), m_subscribe(NULL), m_channel(channel) @@ -56,11 +57,51 @@ void swss::NotificationConsumer::subscribe() SWSS_LOG_INFO("subscribed to %s", m_channel.c_str()); } -void swss::NotificationConsumer::addFd(fd_set *fd) +int swss::NotificationConsumer::getFd() +{ + return m_subscribe->getContext()->fd; +} + +void swss::NotificationConsumer::readData() { SWSS_LOG_ENTER(); - FD_SET(m_subscribe->getContext()->fd, fd); + redisReply *reply = nullptr; + + if (redisGetReply(m_subscribe->getContext(), reinterpret_cast(&reply)) != REDIS_OK) + { + SWSS_LOG_ERROR("failed to read redis reply on channel %s", m_channel.c_str()); + + throw std::runtime_error("Unable to read redis reply"); + } + else + { + RedisReply r(reply); + processReply(reply); + } + + reply = nullptr; + int status; + do + { + status = redisGetReplyFromReader(m_subscribe->getContext(), reinterpret_cast(&reply)); + if(reply != nullptr && status == REDIS_OK) + { + RedisReply r(reply); + processReply(reply); + } + } + while(reply != nullptr && status == REDIS_OK); + + if (status != REDIS_OK) + { + throw std::runtime_error("Unable to read redis reply"); + } +} + +bool swss::NotificationConsumer::hasCachedData() +{ + return m_queue.size() > 1; } void swss::NotificationConsumer::processReply(redisReply *reply) @@ -91,60 +132,11 @@ void swss::NotificationConsumer::processReply(redisReply *reply) m_queue.push(msg); } -int swss::NotificationConsumer::readCache() -{ - SWSS_LOG_ENTER(); - - if (m_queue.size() > 0) - { - return Selectable::DATA; - } - - redisReply *reply = NULL; - - if (redisGetReplyFromReader(m_subscribe->getContext(), (void**)&reply) != REDIS_OK) - { - SWSS_LOG_ERROR("failed to read redis reply on channel %s", m_channel.c_str()); - - return Selectable::ERROR; - } - else if (reply != NULL) - { - RedisReply r(reply); - processReply(reply); - return Selectable::DATA; - } - - return Selectable::NODATA; -} - -void swss::NotificationConsumer::readMe() -{ - SWSS_LOG_ENTER(); - - redisReply *reply = NULL; - - if (redisGetReply(m_subscribe->getContext(), (void**)&reply) != REDIS_OK) - { - SWSS_LOG_ERROR("failed to read redis reply on channel %s", m_channel.c_str()); - - throw std::runtime_error("Unable to read redis reply"); - } - - RedisReply r(reply); - processReply(reply); -} - -bool swss::NotificationConsumer::isMe(fd_set *fd) -{ - return FD_ISSET(m_subscribe->getContext()->fd, fd); -} - void swss::NotificationConsumer::pop(std::string &op, std::string &data, std::vector &values) { SWSS_LOG_ENTER(); - if (m_queue.size() == 0) + if (m_queue.empty()) { SWSS_LOG_ERROR("notification queue is empty, can't pop"); throw std::runtime_error("notification queue is empty, can't pop"); diff --git a/common/notificationconsumer.h b/common/notificationconsumer.h index 52f1df174..2498d3cb4 100644 --- a/common/notificationconsumer.h +++ b/common/notificationconsumer.h @@ -19,16 +19,15 @@ namespace swss { class NotificationConsumer : public Selectable { public: - NotificationConsumer(swss::DBConnector *db, std::string channel); + NotificationConsumer(swss::DBConnector *db, std::string channel, int pri = 100); void pop(std::string &op, std::string &data, std::vector &values); virtual ~NotificationConsumer(); - virtual void addFd(fd_set *fd); - virtual bool isMe(fd_set *fd); - virtual int readCache(); - virtual void readMe(); + int getFd() override; + void readData() override; + bool hasCachedData() override; private: diff --git a/common/rediscommand.cpp b/common/rediscommand.cpp index 888a7b815..10d96c7d1 100644 --- a/common/rediscommand.cpp +++ b/common/rediscommand.cpp @@ -21,6 +21,7 @@ void RedisCommand::format(const char *fmt, ...) va_list ap; va_start(ap, fmt); int len = redisvFormatCommand(&temp, fmt, ap); + va_end(ap); if (len == -1) { throw std::bad_alloc(); } else if (len == -2) { diff --git a/common/redispipeline.h b/common/redispipeline.h index 88cfd23f0..2cb7753c1 100644 --- a/common/redispipeline.h +++ b/common/redispipeline.h @@ -64,7 +64,7 @@ class RedisPipeline { if (m_remaining == 0) return NULL; redisReply *reply; - redisGetReply(m_db->getContext(), (void**)&reply); + redisGetReply(m_db->getContext(), reinterpret_cast(&reply)); RedisReply r(reply); m_remaining--; diff --git a/common/redisselect.cpp b/common/redisselect.cpp index 3931426fd..6523a2048 100644 --- a/common/redisselect.cpp +++ b/common/redisselect.cpp @@ -8,51 +8,57 @@ namespace swss { -RedisSelect::RedisSelect() +RedisSelect::RedisSelect(int pri) : Selectable(pri), m_queueLength(-1) { } -void RedisSelect::addFd(fd_set *fd) +int RedisSelect::getFd() { - FD_SET(m_subscribe->getContext()->fd, fd); + return m_subscribe->getContext()->fd; } -int RedisSelect::readCache() +void RedisSelect::readData() { - redisReply *reply = NULL; + redisReply *reply = nullptr; - /* Read the messages in queue before subscribe command execute */ - if (m_queueLength) { - m_queueLength--; - return Selectable::DATA; - } + if (redisGetReply(m_subscribe->getContext(), reinterpret_cast(&reply)) != REDIS_OK) + throw std::runtime_error("Unable to read redis reply"); - if (redisGetReplyFromReader(m_subscribe->getContext(), - (void**)&reply) != REDIS_OK) - { - return Selectable::ERROR; - } else if (reply != NULL) + freeReplyObject(reply); + m_queueLength++; + + reply = nullptr; + int status; + do { - freeReplyObject(reply); - return Selectable::DATA; + status = redisGetReplyFromReader(m_subscribe->getContext(), reinterpret_cast(&reply)); + if(reply != nullptr && status == REDIS_OK) + { + m_queueLength++; + freeReplyObject(reply); + } } + while(reply != nullptr && status == REDIS_OK); - return Selectable::NODATA; + if (status != REDIS_OK) + { + throw std::runtime_error("Unable to read redis reply"); + } } -void RedisSelect::readMe() +bool RedisSelect::hasCachedData() { - redisReply *reply = NULL; - - if (redisGetReply(m_subscribe->getContext(), (void**)&reply) != REDIS_OK) - throw "Unable to read redis reply"; + return m_queueLength > 1; +} - freeReplyObject(reply); +bool RedisSelect::initializedWithData() +{ + return m_queueLength > 0; } -bool RedisSelect::isMe(fd_set *fd) +void RedisSelect::updateAfterRead() { - return FD_ISSET(m_subscribe->getContext()->fd, fd); + m_queueLength--; } /* Create a new redisContext, SELECT DB and SUBSCRIBE */ diff --git a/common/redisselect.h b/common/redisselect.h index 589d99e0a..49f99e295 100644 --- a/common/redisselect.h +++ b/common/redisselect.h @@ -12,15 +12,13 @@ class RedisSelect : public Selectable /* The database is already alive and kicking, no need for more than a second */ static constexpr unsigned int SUBSCRIBE_TIMEOUT = 1000; - RedisSelect(); + RedisSelect(int pri = 0); - void addFd(fd_set *fd); - - int readCache(); - - void readMe(); - - bool isMe(fd_set *fd); + int getFd() override; + void readData() override; + bool hasCachedData() override; + bool initializedWithData() override; + void updateAfterRead() override; /* Create a new redisContext, SELECT DB and SUBSCRIBE */ void subscribe(DBConnector* db, std::string channelName); diff --git a/common/select.cpp b/common/select.cpp index f0e0cf8b3..adbb15eb5 100644 --- a/common/select.cpp +++ b/common/select.cpp @@ -5,24 +5,78 @@ #include #include #include +#include +#include +#include using namespace std; namespace swss { +Select::Select() +{ + m_epoll_fd = ::epoll_create1(0); + if (m_epoll_fd == -1) + { + std::string error = std::string("Select::constructor:epoll_create1: error=(" + + std::to_string(errno) + "}:" + + strerror(errno)); + throw std::runtime_error(error); + } +} + +Select::~Select() +{ + (void)::close(m_epoll_fd); +} + void Select::addSelectable(Selectable *selectable) { - if(find(m_objects.begin(), m_objects.end(), selectable) != m_objects.end()) + const int fd = selectable->getFd(); + + if(m_objects.find(fd) != m_objects.end()) { SWSS_LOG_WARN("Selectable is already added to the list, ignoring."); return; } - m_objects.push_back(selectable); + + m_objects[fd] = selectable; + + if (selectable->initializedWithData()) + { + m_ready.insert(selectable); + } + + struct epoll_event ev = { + .events = EPOLLIN, + .data = { .fd = fd, }, + }; + + int res = ::epoll_ctl(m_epoll_fd, EPOLL_CTL_ADD, fd, &ev); + if (res == -1) + { + std::string error = std::string("Select::add_fd:epoll_ctl: error=(" + + std::to_string(errno) + "}:" + + strerror(errno)); + throw std::runtime_error(error); + } } -void Select::removeSelectable(Selectable *c) +void Select::removeSelectable(Selectable *selectable) { - m_objects.erase(remove(m_objects.begin(), m_objects.end(), c), m_objects.end()); + const int fd = selectable->getFd(); + + m_objects.erase(fd); + m_ready.erase(selectable); + + int res = ::epoll_ctl(m_epoll_fd, EPOLL_CTL_DEL, fd, NULL); + if (res == -1) + { + std::string error = std::string("Select::del_fd:epoll_ctl: error=(" + + std::to_string(errno) + "}:" + + strerror(errno)); + throw std::runtime_error(error); + } } void Select::addSelectables(vector selectables) @@ -33,77 +87,72 @@ void Select::addSelectables(vector selectables) } } -void Select::addFd(int fd) +int Select::select1(Selectable **c, unsigned int timeout) { - m_fds.push_back(fd); -} + int sz_selectables = static_cast(m_objects.size()); + std::vector events(sz_selectables); + int ret; -int Select::select(Selectable **c, int *fd, unsigned int timeout) -{ - SWSS_LOG_ENTER(); + do + { + ret = ::epoll_wait(m_epoll_fd, events.data(), sz_selectables, timeout); + } + while(ret == -1 && errno == EINTR); // Retry the select if the process was interrupted by a signal - struct timeval t = {0, (suseconds_t)(timeout)*1000}; - struct timeval *pTimeout = NULL; - fd_set fs; - int err; + if (ret < 0) + return Select::ERROR; - FD_ZERO(&fs); - *c = NULL; - *fd = 0; - if (timeout != numeric_limits::max()) - pTimeout = &t; + for (int i = 0; i < ret; ++i) + { + int fd = events[i].data.fd; + Selectable* sel = m_objects[fd]; + sel->readData(); + m_ready.insert(sel); + } - /* Checking caching from reader */ - for (Selectable *i : m_objects) + while (!m_ready.empty()) { - err = i->readCache(); + auto sel = *m_ready.begin(); + + *c = sel; - if (err == Selectable::ERROR) - return Select::ERROR; - else if (err == Selectable::DATA) { - *c = i; - return Select::OBJECT; + if (!sel->hasCachedData()) + { + // remove Selectable from the ready when there're no messages anymore + m_ready.erase(sel); } - /* else, timeout = no data */ - i->addFd(&fs); - } + sel->updateAfterRead(); - for (int fdn : m_fds) - { - FD_SET(fdn, &fs); + return Select::OBJECT; } - do - { - err = ::select(FD_SETSIZE, &fs, NULL, NULL, pTimeout); - } - while(err == -1 && errno == EINTR); // Retry the select if the process was interrupted by a signal + return Select::TIMEOUT; +} - if (err < 0) - return Select::ERROR; - if (err == 0) - return Select::TIMEOUT; +int Select::select(Selectable **c, int *fd, unsigned int timeout) +{ + SWSS_LOG_ENTER(); + *fd = 0; - /* Check other consumer-table */ - for (Selectable *i : m_objects) - if (i->isMe(&fs)) - { - i->readMe(); - *c = i; - return Select::OBJECT; - } + int ret; - /* Check other FDs */ - for (int f : m_fds) - if (FD_ISSET(f, &fs)) - { - *fd = f; - return Select::FD; - } + *c = NULL; + if (timeout == numeric_limits::max()) + timeout = -1; + + /* check if we have some data */ + ret = select1(c, 0); + + /* return if we have data, we have an error or desired timeout was 0 */ + if (ret != Select::TIMEOUT || timeout == 0) + return ret; + + /* wait for data */ + ret = select1(c, timeout); + + return ret; - /* Shouldn't reach here */ - return Select::ERROR; } }; diff --git a/common/select.h b/common/select.h index af10577e7..c33a78e56 100644 --- a/common/select.h +++ b/common/select.h @@ -3,6 +3,9 @@ #include #include +#include +#include +#include #include #include #include "selectable.h" @@ -12,29 +15,32 @@ namespace swss { class Select { public: + Select(); + ~Select(); + /* Add object for select */ void addSelectable(Selectable *selectable); + + /* Remove object from select */ void removeSelectable(Selectable *selectable); - void addSelectables(std::vector selectables); - /* Add file-descriptor for select */ - void addFd(int fd); + /* Add multiple messages for select */ + void addSelectables(std::vector selectables); - /* - * Wait until data will arrived, returns the object on which select() - * was signaled. - */ enum { OBJECT = 0, - FD = 1, - ERROR = 2, - TIMEOUT = 3 + ERROR = 1, + TIMEOUT = 2, }; + int select(Selectable **c, int *fd, unsigned int timeout = std::numeric_limits::max()); private: - std::vector m_objects; - std::vector m_fds; + int select1(Selectable **c, unsigned int timeout); + + int m_epoll_fd; + std::unordered_map m_objects; + std::set m_ready; }; } diff --git a/common/selectable.h b/common/selectable.h index e14341a63..fbd28169f 100644 --- a/common/selectable.h +++ b/common/selectable.h @@ -11,23 +11,50 @@ namespace swss { class Selectable { public: + Selectable(int pri = 0) : m_priority(pri) {} virtual ~Selectable() {}; - enum { - DATA = 0, - ERROR = 1, - NODATA = 2 + /* return file handler for the Selectable */ + virtual int getFd() = 0; + + /* Read all data from the fd assicaited with Selectable */ + virtual void readData() = 0; + + /* true if Selectable has data in its cache */ + virtual bool hasCachedData() + { + return false; + } + + /* true if Selectable was initialized with data */ + virtual bool initializedWithData() + { + return false; + } + + /* run this function after every read */ + virtual void updateAfterRead() + { + } + + int getPri() const + { + return m_priority; + } + + struct cmp + { + bool operator()(const Selectable* a, const Selectable* b) const + { + if (a->getPri() == b->getPri()) + return a > b; + else + return a->getPri() > b->getPri(); + } }; - /* Implements FD_SET */ - virtual void addFd(fd_set *fd) = 0; - virtual bool isMe(fd_set *fd) = 0; - - /* Read and empty socket caching (if exists) */ - virtual int readCache() = 0; - - /* Read a message from the socket */ - virtual void readMe() = 0; +private: + int m_priority; }; } diff --git a/common/selectableevent.cpp b/common/selectableevent.cpp index c90e9076e..ed940756b 100644 --- a/common/selectableevent.cpp +++ b/common/selectableevent.cpp @@ -11,8 +11,8 @@ namespace swss { -SelectableEvent::SelectableEvent() : - m_efd(0) +SelectableEvent::SelectableEvent(int pri) : + Selectable(pri), m_efd(0) { m_efd = eventfd(0, 0); @@ -35,17 +35,12 @@ SelectableEvent::~SelectableEvent() while(err == -1 && errno == EINTR); } -void SelectableEvent::addFd(fd_set *fd) +int SelectableEvent::getFd() { - FD_SET(m_efd, fd); + return m_efd; } -int SelectableEvent::readCache() -{ - return Selectable::NODATA; -} - -void SelectableEvent::readMe() +void SelectableEvent::readData() { uint64_t r; @@ -64,11 +59,6 @@ void SelectableEvent::readMe() } } -bool SelectableEvent::isMe(fd_set *fd) -{ - return FD_ISSET(m_efd, fd); -} - void SelectableEvent::notify() { SWSS_LOG_ENTER(); diff --git a/common/selectableevent.h b/common/selectableevent.h index 37c6484b1..13e996bbd 100644 --- a/common/selectableevent.h +++ b/common/selectableevent.h @@ -12,15 +12,13 @@ namespace swss { class SelectableEvent : public Selectable { public: - SelectableEvent(); + SelectableEvent(int pri = 0); virtual ~SelectableEvent(); void notify(); - virtual void addFd(fd_set *fd); - virtual bool isMe(fd_set *fd); - virtual int readCache(); - virtual void readMe(); + int getFd() override; + void readData() override; private: int m_efd; diff --git a/common/selectabletimer.cpp b/common/selectabletimer.cpp index 736ddce72..aedc98c62 100644 --- a/common/selectabletimer.cpp +++ b/common/selectabletimer.cpp @@ -11,8 +11,8 @@ namespace swss { -SelectableTimer::SelectableTimer(const timespec& interval) - : m_zero({{0, 0}, {0, 0}}) +SelectableTimer::SelectableTimer(const timespec& interval, int pri) + : Selectable(pri), m_zero({{0, 0}, {0, 0}}) { // Create the timer m_tfd = timerfd_create(CLOCK_REALTIME, 0); @@ -67,17 +67,12 @@ void SelectableTimer::setInterval(const timespec& interval) m_interval.it_interval = interval; } -void SelectableTimer::addFd(fd_set *fd) +int SelectableTimer::getFd() { - FD_SET(m_tfd, fd); + return m_tfd; } -int SelectableTimer::readCache() -{ - return Selectable::NODATA; -} - -void SelectableTimer::readMe() +void SelectableTimer::readData() { uint64_t r; @@ -95,9 +90,6 @@ void SelectableTimer::readMe() } } -bool SelectableTimer::isMe(fd_set *fd) -{ - return FD_ISSET(m_tfd, fd); -} +// FIXME: if timer events are read slower than timer frequency we will lost time events } diff --git a/common/selectabletimer.h b/common/selectabletimer.h index ecc02359d..d6279d0c0 100644 --- a/common/selectabletimer.h +++ b/common/selectabletimer.h @@ -11,17 +11,15 @@ namespace swss { class SelectableTimer : public Selectable { public: - SelectableTimer(const timespec& interval); + SelectableTimer(const timespec& interval, int pri = 50); virtual ~SelectableTimer(); void start(); void stop(); void reset(); void setInterval(const timespec& interval); - virtual void addFd(fd_set *fd); - virtual bool isMe(fd_set *fd); - virtual int readCache(); - virtual void readMe(); + int getFd() override; + void readData() override; private: int m_tfd; diff --git a/common/subscriberstatetable.cpp b/common/subscriberstatetable.cpp index cdaaa08d5..78887c001 100644 --- a/common/subscriberstatetable.cpp +++ b/common/subscriberstatetable.cpp @@ -14,8 +14,9 @@ using namespace std; namespace swss { -SubscriberStateTable::SubscriberStateTable(DBConnector *db, string tableName) - : ConsumerTableBase(db, tableName), m_table(db, tableName, CONFIGDB_TABLE_NAME_SEPARATOR) +SubscriberStateTable::SubscriberStateTable(DBConnector *db, string tableName, int popBatchSize, int pri) + : ConsumerTableBase(db, tableName, popBatchSize, pri), + m_table(db, tableName, CONFIGDB_TABLE_NAME_SEPARATOR) { m_keyspace = "__keyspace@"; @@ -42,49 +43,48 @@ SubscriberStateTable::SubscriberStateTable(DBConnector *db, string tableName) } } -int SubscriberStateTable::readCache() +void SubscriberStateTable::readData() { - redisReply *reply = NULL; + redisReply *reply = nullptr; - /* If buffers already contain data notify caller about this */ - if (!m_buffer.empty() || !m_keyspace_event_buffer.empty()) + /* Read data from redis. This call is non blocking. This method + * is called from Select framework when data is available in socket. + * NOTE: All data should be stored in event buffer. It won't be possible to + * read them second time. */ + if (redisGetReply(m_subscribe->getContext(), reinterpret_cast(&reply)) != REDIS_OK) { - return Selectable::DATA; + throw std::runtime_error("Unable to read redis reply"); } + m_keyspace_event_buffer.push_back(shared_ptr(make_shared(reply))); + /* Try to read data from redis cacher. * If data exists put it to event buffer. * NOTE: Keyspace event is not persistent and it won't * be possible to read it second time. If it is not stared in * the buffer it will be lost. */ - if (redisGetReplyFromReader(m_subscribe->getContext(), - (void**)&reply) != REDIS_OK) + + reply = nullptr; + int status; + do { - return Selectable::ERROR; + status = redisGetReplyFromReader(m_subscribe->getContext(), reinterpret_cast(&reply)); + if(reply != nullptr && status == REDIS_OK) + { + m_keyspace_event_buffer.push_back(shared_ptr(make_shared(reply))); + } } - else if (reply != NULL) + while(reply != nullptr && status == REDIS_OK); + + if (status != REDIS_OK) { - m_keyspace_event_buffer.push_back(shared_ptr(make_shared(reply))); - return Selectable::DATA; + throw std::runtime_error("Unable to read redis reply"); } - - return Selectable::NODATA; } -void SubscriberStateTable::readMe() +bool SubscriberStateTable::hasCachedData() { - redisReply *reply = NULL; - - /* Read data from redis. This call is non blocking. This method - * is called from Select framework when data is available in socket. - * NOTE: All data should be stored in event buffer. It won't be possible to - * read them second time. */ - if (redisGetReply(m_subscribe->getContext(), (void**)&reply) != REDIS_OK) - { - throw runtime_error("Unable to read redis reply"); - } - - m_keyspace_event_buffer.push_back(shared_ptr(make_shared(reply))); + return m_buffer.size() > 1 || m_keyspace_event_buffer.size() > 1; } void SubscriberStateTable::pops(deque &vkco, string /*prefix*/) diff --git a/common/subscriberstatetable.h b/common/subscriberstatetable.h index 1871df3a7..5d47e2cdf 100644 --- a/common/subscriberstatetable.h +++ b/common/subscriberstatetable.h @@ -11,15 +11,18 @@ namespace swss { class SubscriberStateTable : public ConsumerTableBase { public: - SubscriberStateTable(DBConnector *db, std::string tableName); + SubscriberStateTable(DBConnector *db, std::string tableName, int popBatchSize = DEFAULT_POP_BATCH_SIZE, int pri = 0); /* Get all elements available */ void pops(std::deque &vkco, std::string prefix = EMPTY_PREFIX); - /* Verify if cache contains data */ - int readCache(); /* Read keyspace event from redis */ - void readMe(); + void readData() override; + bool hasCachedData() override; + bool initializedWithData() override + { + return !m_buffer.empty(); + } private: /* Pop keyspace event from event buffer. Caller should free resources. */ diff --git a/common/table.h b/common/table.h index 413406e93..3bee9efde 100644 --- a/common/table.h +++ b/common/table.h @@ -95,7 +95,7 @@ class TableConsumable : public TableBase, public TableEntryPoppable, public Redi /* The default value of pop batch size is 128 */ static constexpr int DEFAULT_POP_BATCH_SIZE = 128; - TableConsumable(std::string tableName) : TableBase(tableName) { } + TableConsumable(std::string tableName, int pri) : TableBase(tableName), RedisSelect(pri) { } }; class TableEntryEnumerable { diff --git a/tests/Makefile.am b/tests/Makefile.am index 0bdf874fa..92ecf388a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -24,9 +24,10 @@ tests_SOURCES = redis_ut.cpp \ macaddress_ut.cpp \ converter_ut.cpp \ exec_ut.cpp \ - redis_subscriber_state_ut.cpp + redis_subscriber_state_ut.cpp \ + selectable_priority.cpp -tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) -tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) -tests_LDADD = $(LDADD_GTEST) -lpthread -L$(top_srcdir)/common -lswsscommon +tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(LIBNL_CFLAGS) +tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(LIBNL_CFLAGS) +tests_LDADD = $(LDADD_GTEST) -lpthread -L$(top_srcdir)/common -lswsscommon $(LIBNL_LIBS) diff --git a/tests/redis_piped_state_ut.cpp b/tests/redis_piped_state_ut.cpp index 3904322ee..09c012724 100644 --- a/tests/redis_piped_state_ut.cpp +++ b/tests/redis_piped_state_ut.cpp @@ -135,7 +135,7 @@ static void consumerWorker(int index) } EXPECT_TRUE(numberOfKeysSet <= numberOfKeyDeleted); - EXPECT_EQ(ret, Selectable::DATA); + EXPECT_EQ(ret, Select::OBJECT); } static inline void clearDB() @@ -427,7 +427,7 @@ TEST(ConsumerStateTable, async_singlethread) } EXPECT_TRUE(numberOfKeysSet <= numberOfKeyDeleted); - EXPECT_EQ(ret, Selectable::DATA); + EXPECT_EQ(ret, Select::OBJECT); cout << "Done. Waiting for all job to finish " << NUMBER_OF_OPS << " jobs." << endl; diff --git a/tests/redis_piped_ut.cpp b/tests/redis_piped_ut.cpp index 353a6de9a..05f8bb53f 100644 --- a/tests/redis_piped_ut.cpp +++ b/tests/redis_piped_ut.cpp @@ -133,7 +133,7 @@ static void consumerWorker(int index) break; } - EXPECT_EQ(ret, Selectable::DATA); + EXPECT_EQ(ret, Select::OBJECT); } static void clearDB() diff --git a/tests/redis_state_ut.cpp b/tests/redis_state_ut.cpp index 80e117635..cb89c7e93 100644 --- a/tests/redis_state_ut.cpp +++ b/tests/redis_state_ut.cpp @@ -133,7 +133,7 @@ static void consumerWorker(int index) } EXPECT_TRUE(numberOfKeysSet <= numberOfKeyDeleted); - EXPECT_EQ(ret, Selectable::DATA); + EXPECT_EQ(ret, Select::OBJECT); } static inline void clearDB() @@ -416,7 +416,7 @@ TEST(ConsumerStateTable, singlethread) } EXPECT_TRUE(numberOfKeysSet <= numberOfKeyDeleted); - EXPECT_EQ(ret, Selectable::DATA); + EXPECT_EQ(ret, Select::OBJECT); cout << "Done. Waiting for all job to finish " << NUMBER_OF_OPS << " jobs." << endl; diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp index 2ba577952..b46f9fe02 100644 --- a/tests/redis_ut.cpp +++ b/tests/redis_ut.cpp @@ -133,7 +133,7 @@ void consumerWorker(int index) break; } - EXPECT_EQ(ret, Selectable::DATA); + EXPECT_EQ(ret, Select::OBJECT); } void clearDB() diff --git a/tests/selectable_priority.cpp b/tests/selectable_priority.cpp new file mode 100644 index 000000000..2e68dda92 --- /dev/null +++ b/tests/selectable_priority.cpp @@ -0,0 +1,227 @@ +#include "common/dbconnector.h" +#include "common/consumertable.h" +#include "common/notificationconsumer.h" +#include "common/select.h" +#include "common/selectableevent.h" +#include "common/selectabletimer.h" +#include "common/subscriberstatetable.h" +#include "common/netmsg.h" +#include "common/netlink.h" +#include "gtest/gtest.h" + + +using namespace std; +using namespace swss; + + +#define TEST_VIEW (7) +#define DEFAULT_POP_BATCH_SIZE (10) + + +TEST(Prioriry, default_pri_values) +{ + std::string tableName = "tableName"; + + DBConnector db(TEST_VIEW, "localhost", 6379, 0); + + timespec interval = { .tv_sec = 1, .tv_nsec = 0 }; + + NetLink nl; + ConsumerStateTable cst(&db, tableName); + ConsumerTable ct(&db, tableName); + NotificationConsumer nc(&db, tableName); + RedisSelect rs; + SelectableEvent se; + SelectableTimer st(interval); + SubscriberStateTable sst(&db, tableName); + + EXPECT_EQ(nl.getPri(), 0); + EXPECT_EQ(cst.getPri(), 0); + EXPECT_EQ(ct.getPri(), 0); + EXPECT_EQ(nc.getPri(), 100); + EXPECT_EQ(rs.getPri(), 0); + EXPECT_EQ(se.getPri(), 0); + EXPECT_EQ(st.getPri(), 50); + EXPECT_EQ(sst.getPri(), 0); +} + +TEST(Prioriry, set_pri_values) +{ + std::string tableName = "tableName"; + + DBConnector db(TEST_VIEW, "localhost", 6379, 0); + + timespec interval = { .tv_sec = 1, .tv_nsec = 0 }; + + NetLink nl(101); + ConsumerStateTable cst(&db, tableName, DEFAULT_POP_BATCH_SIZE, 102); + ConsumerTable ct(&db, tableName, DEFAULT_POP_BATCH_SIZE, 103); + NotificationConsumer nc(&db, tableName, 104); + RedisSelect rs(105); + SelectableEvent se(106); + SelectableTimer st(interval, 107); + SubscriberStateTable sst(&db, tableName, DEFAULT_POP_BATCH_SIZE, 108); + + EXPECT_EQ(nl.getPri(), 101); + EXPECT_EQ(cst.getPri(), 102); + EXPECT_EQ(ct.getPri(), 103); + EXPECT_EQ(nc.getPri(), 104); + EXPECT_EQ(rs.getPri(), 105); + EXPECT_EQ(se.getPri(), 106); + EXPECT_EQ(st.getPri(), 107); + EXPECT_EQ(sst.getPri(), 108); +} + +TEST(Prioriry, priority_select_1) +{ + Select cs; + Selectable *selectcs; + + SelectableEvent s1(100); + SelectableEvent s2(1000); + SelectableEvent s3(10000); + + cs.addSelectable(&s1); + cs.addSelectable(&s2); + cs.addSelectable(&s3); + + s1.notify(); + s2.notify(); + s3.notify(); + + int tmpfd; + int ret; + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_EQ(selectcs, &s3); + + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_EQ(selectcs, &s2); + + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_EQ(selectcs, &s1); +} + +TEST(Prioriry, priority_select_2) +{ + Select cs; + Selectable *selectcs; + + SelectableEvent s1(100); + SelectableEvent s2(1000); + SelectableEvent s3(10000); + + s1.notify(); + s2.notify(); + s3.notify(); + + cs.addSelectable(&s1); + cs.addSelectable(&s2); + cs.addSelectable(&s3); + + int tmpfd; + int ret; + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_EQ(selectcs, &s3); + + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_EQ(selectcs, &s2); + + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_EQ(selectcs, &s1); +} + +TEST(Prioriry, priority_select_3) +{ + Select cs; + Selectable *selectcs; + + SelectableEvent s1(10); + SelectableEvent s2(10); + SelectableEvent s3(10000); + + cs.addSelectable(&s1); + cs.addSelectable(&s2); + cs.addSelectable(&s3); + + s1.notify(); + s2.notify(); + s3.notify(); + + int tmpfd; + int ret; + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_EQ(selectcs, &s3); + + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_TRUE(selectcs==&s1 || selectcs==&s2); + + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_TRUE(selectcs==&s1 || selectcs==&s2); +} + +TEST(Prioriry, priority_select_4) +{ + Select cs; + Selectable *selectcs; + + SelectableEvent s1(10); + SelectableEvent s2(10000); + SelectableEvent s3(10000); + + cs.addSelectable(&s1); + cs.addSelectable(&s2); + cs.addSelectable(&s3); + + s1.notify(); + s2.notify(); + s3.notify(); + + int tmpfd; + int ret; + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_TRUE(selectcs==&s2 || selectcs==&s3); + + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_TRUE(selectcs==&s2 || selectcs==&s3); + + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_EQ(selectcs, &s1); +} + +TEST(Prioriry, priority_select_5) +{ + Select cs; + Selectable *selectcs; + + SelectableEvent s1(150); + SelectableEvent s2(1000); + + cs.addSelectable(&s1); + cs.addSelectable(&s2); + + s1.notify(); + s2.notify(); + + int tmpfd; + int ret; + ret = cs.select(&selectcs, &tmpfd); + EXPECT_EQ(ret, Select::OBJECT); + EXPECT_EQ(selectcs, &s2); + + cs.removeSelectable(&s1); + + ret = cs.select(&selectcs, &tmpfd, 1000); + EXPECT_EQ(ret, Select::TIMEOUT); +} From cfee528e3141d9411101b777d051cc127798acf4 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Tue, 3 Apr 2018 13:19:27 -0700 Subject: [PATCH 02/13] Remove fd parameter from Select.select(). This parameter was never used --- common/logger.cpp | 3 +-- common/select.cpp | 3 +-- common/select.h | 2 +- tests/ntf_ut.cpp | 4 +--- tests/redis_piped_state_ut.cpp | 26 +++++++++-------------- tests/redis_piped_ut.cpp | 13 +++++------- tests/redis_state_ut.cpp | 26 +++++++++-------------- tests/redis_subscriber_state_ut.cpp | 20 ++++++----------- tests/redis_ut.cpp | 25 ++++++++++------------ tests/selectable_priority.cpp | 33 ++++++++++++----------------- tests/test_redis_ut.py | 4 ++-- 11 files changed, 63 insertions(+), 96 deletions(-) diff --git a/common/logger.cpp b/common/logger.cpp index 018e2d855..f24f7cbbd 100644 --- a/common/logger.cpp +++ b/common/logger.cpp @@ -166,10 +166,9 @@ Logger::Priority Logger::getMinPrio() while(true) { - int fd = 0; Selectable *selectable = nullptr; - int ret = select.select(&selectable, &fd); + int ret = select.select(&selectable); if (ret == Select::ERROR) { diff --git a/common/select.cpp b/common/select.cpp index adbb15eb5..996fa1ac1 100644 --- a/common/select.cpp +++ b/common/select.cpp @@ -130,10 +130,9 @@ int Select::select1(Selectable **c, unsigned int timeout) return Select::TIMEOUT; } -int Select::select(Selectable **c, int *fd, unsigned int timeout) +int Select::select(Selectable **c, unsigned int timeout) { SWSS_LOG_ENTER(); - *fd = 0; int ret; diff --git a/common/select.h b/common/select.h index c33a78e56..3b5438e6b 100644 --- a/common/select.h +++ b/common/select.h @@ -33,7 +33,7 @@ class Select TIMEOUT = 2, }; - int select(Selectable **c, int *fd, unsigned int timeout = std::numeric_limits::max()); + int select(Selectable **c, unsigned int timeout = std::numeric_limits::max()); private: int select1(Selectable **c, unsigned int timeout); diff --git a/tests/ntf_ut.cpp b/tests/ntf_ut.cpp index fb30ae8bf..a450ed66e 100644 --- a/tests/ntf_ut.cpp +++ b/tests/ntf_ut.cpp @@ -33,9 +33,7 @@ void ntf_thread() { swss::Selectable *sel; - int fd; - - int result = s.select(&sel, &fd); + int result = s.select(&sel); if (result == swss::Select::OBJECT) { diff --git a/tests/redis_piped_state_ut.cpp b/tests/redis_piped_state_ut.cpp index 09c012724..3dbed3be3 100644 --- a/tests/redis_piped_state_ut.cpp +++ b/tests/redis_piped_state_ut.cpp @@ -108,14 +108,13 @@ static void consumerWorker(int index) ConsumerStateTable c(&db, tableName); Select cs; Selectable *selectcs; - int tmpfd; int numberOfKeysSet = 0; int numberOfKeyDeleted = 0; int ret, i = 0; KeyOpFieldsValuesTuple kco; cs.addSelectable(&c); - while ((ret = cs.select(&selectcs, &tmpfd)) == Select::OBJECT) + while ((ret = cs.select(&selectcs)) == Select::OBJECT) { c.pop(kco); if (kfvOp(kco) == "SET") @@ -186,11 +185,10 @@ TEST(ConsumerStateTable, async_double_set) Select cs; Selectable *selectcs; cs.addSelectable(&c); - int tmpfd; /* First pop operation */ { - int ret = cs.select(&selectcs, &tmpfd); + int ret = cs.select(&selectcs); EXPECT_TRUE(ret == Select::OBJECT); KeyOpFieldsValuesTuple kco; c.pop(kco); @@ -218,7 +216,7 @@ TEST(ConsumerStateTable, async_double_set) /* Second select operation */ { - int ret = cs.select(&selectcs, &tmpfd, 1000); + int ret = cs.select(&selectcs, 1000); EXPECT_TRUE(ret == Select::TIMEOUT); } } @@ -256,11 +254,10 @@ TEST(ConsumerStateTable, async_set_del) Select cs; Selectable *selectcs; cs.addSelectable(&c); - int tmpfd; /* First pop operation */ { - int ret = cs.select(&selectcs, &tmpfd); + int ret = cs.select(&selectcs); EXPECT_TRUE(ret == Select::OBJECT); KeyOpFieldsValuesTuple kco; c.pop(kco); @@ -273,7 +270,7 @@ TEST(ConsumerStateTable, async_set_del) /* Second select operation */ { - int ret = cs.select(&selectcs, &tmpfd, 1000); + int ret = cs.select(&selectcs, 1000); EXPECT_TRUE(ret == Select::TIMEOUT); } } @@ -322,11 +319,10 @@ TEST(ConsumerStateTable, async_set_del_set) Select cs; Selectable *selectcs; cs.addSelectable(&c); - int tmpfd; /* First pop operation */ { - int ret = cs.select(&selectcs, &tmpfd); + int ret = cs.select(&selectcs); EXPECT_TRUE(ret == Select::OBJECT); KeyOpFieldsValuesTuple kco; c.pop(kco); @@ -350,7 +346,7 @@ TEST(ConsumerStateTable, async_set_del_set) /* Second select operation */ { - int ret = cs.select(&selectcs, &tmpfd, 1000); + int ret = cs.select(&selectcs, 1000); EXPECT_TRUE(ret == Select::TIMEOUT); } } @@ -384,13 +380,12 @@ TEST(ConsumerStateTable, async_singlethread) ConsumerStateTable c(&db, tableName); Select cs; Selectable *selectcs; - int tmpfd; int ret, i = 0; KeyOpFieldsValuesTuple kco; cs.addSelectable(&c); int numberOfKeysSet = 0; - while ((ret = cs.select(&selectcs, &tmpfd)) == Select::OBJECT) + while ((ret = cs.select(&selectcs)) == Select::OBJECT) { c.pop(kco); EXPECT_TRUE(kfvOp(kco) == "SET"); @@ -413,7 +408,7 @@ TEST(ConsumerStateTable, async_singlethread) p.flush(); int numberOfKeyDeleted = 0; - while ((ret = cs.select(&selectcs, &tmpfd)) == Select::OBJECT) + while ((ret = cs.select(&selectcs)) == Select::OBJECT) { c.pop(kco); EXPECT_TRUE(kfvOp(kco) == "DEL"); @@ -490,9 +485,8 @@ TEST(ConsumerStateTable, async_multitable) while (1) { Selectable *is; - int fd; - ret = cs.select(&is, &fd); + ret = cs.select(&is); EXPECT_EQ(ret, Select::OBJECT); ((ConsumerStateTable *)is)->pop(kco); diff --git a/tests/redis_piped_ut.cpp b/tests/redis_piped_ut.cpp index 05f8bb53f..dc05af7bf 100644 --- a/tests/redis_piped_ut.cpp +++ b/tests/redis_piped_ut.cpp @@ -106,14 +106,13 @@ static void consumerWorker(int index) ConsumerTable c(&db, tableName); Select cs; Selectable *selectcs; - int tmpfd; int numberOfKeysSet = 0; int numberOfKeyDeleted = 0; int ret, i = 0; KeyOpFieldsValuesTuple kco; cs.addSelectable(&c); - while ((ret = cs.select(&selectcs, &tmpfd)) == Select::OBJECT) + while ((ret = cs.select(&selectcs)) == Select::OBJECT) { c.pop(kco); if (kfvOp(kco) == "SET") @@ -199,9 +198,8 @@ TEST(DBConnector, piped_multitable) while (1) { Selectable *is; - int fd; - ret = cs.select(&is, &fd); + ret = cs.select(&is); EXPECT_EQ(ret, Select::OBJECT); ((ConsumerTable *)is)->pop(kco); @@ -255,13 +253,13 @@ TEST(DBConnector, piped_notifications) Select s; s.addSelectable(&nc); Selectable *sel; - int fd, value = 1; + int value = 1; clearDB(); thread np(notificationProducer); - int result = s.select(&sel, &fd, 2000); + int result = s.select(&sel, 2000); if (result == Select::OBJECT) { cout << "Got notification from producer" << endl; @@ -291,11 +289,10 @@ static void selectableEventThread(Selectable *ev, int *value) Select s; s.addSelectable(ev); Selectable *sel; - int fd; cout << "Starting listening ... " << endl; - int result = s.select(&sel, &fd, 2000); + int result = s.select(&sel, 2000); if (result == Select::OBJECT) { if (sel == ev) diff --git a/tests/redis_state_ut.cpp b/tests/redis_state_ut.cpp index cb89c7e93..99549e686 100644 --- a/tests/redis_state_ut.cpp +++ b/tests/redis_state_ut.cpp @@ -106,14 +106,13 @@ static void consumerWorker(int index) ConsumerStateTable c(&db, tableName); Select cs; Selectable *selectcs; - int tmpfd; int numberOfKeysSet = 0; int numberOfKeyDeleted = 0; int ret, i = 0; KeyOpFieldsValuesTuple kco; cs.addSelectable(&c); - while ((ret = cs.select(&selectcs, &tmpfd)) == Select::OBJECT) + while ((ret = cs.select(&selectcs)) == Select::OBJECT) { c.pop(kco); if (kfvOp(kco) == "SET") @@ -182,11 +181,10 @@ TEST(ConsumerStateTable, double_set) Select cs; Selectable *selectcs; cs.addSelectable(&c); - int tmpfd; /* First pop operation */ { - int ret = cs.select(&selectcs, &tmpfd); + int ret = cs.select(&selectcs); EXPECT_TRUE(ret == Select::OBJECT); KeyOpFieldsValuesTuple kco; c.pop(kco); @@ -214,7 +212,7 @@ TEST(ConsumerStateTable, double_set) /* Second select operation */ { - int ret = cs.select(&selectcs, &tmpfd, 1000); + int ret = cs.select(&selectcs, 1000); EXPECT_TRUE(ret == Select::TIMEOUT); } } @@ -250,11 +248,10 @@ TEST(ConsumerStateTable, set_del) Select cs; Selectable *selectcs; cs.addSelectable(&c); - int tmpfd; /* First pop operation */ { - int ret = cs.select(&selectcs, &tmpfd); + int ret = cs.select(&selectcs); EXPECT_TRUE(ret == Select::OBJECT); KeyOpFieldsValuesTuple kco; c.pop(kco); @@ -267,7 +264,7 @@ TEST(ConsumerStateTable, set_del) /* Second select operation */ { - int ret = cs.select(&selectcs, &tmpfd, 1000); + int ret = cs.select(&selectcs, 1000); EXPECT_TRUE(ret == Select::TIMEOUT); } } @@ -314,11 +311,10 @@ TEST(ConsumerStateTable, set_del_set) Select cs; Selectable *selectcs; cs.addSelectable(&c); - int tmpfd; /* First pop operation */ { - int ret = cs.select(&selectcs, &tmpfd); + int ret = cs.select(&selectcs); EXPECT_TRUE(ret == Select::OBJECT); KeyOpFieldsValuesTuple kco; c.pop(kco); @@ -342,7 +338,7 @@ TEST(ConsumerStateTable, set_del_set) /* Second select operation */ { - int ret = cs.select(&selectcs, &tmpfd, 1000); + int ret = cs.select(&selectcs, 1000); EXPECT_TRUE(ret == Select::TIMEOUT); } } @@ -374,13 +370,12 @@ TEST(ConsumerStateTable, singlethread) ConsumerStateTable c(&db, tableName); Select cs; Selectable *selectcs; - int tmpfd; int ret, i = 0; KeyOpFieldsValuesTuple kco; cs.addSelectable(&c); int numberOfKeysSet = 0; - while ((ret = cs.select(&selectcs, &tmpfd)) == Select::OBJECT) + while ((ret = cs.select(&selectcs)) == Select::OBJECT) { c.pop(kco); EXPECT_TRUE(kfvOp(kco) == "SET"); @@ -402,7 +397,7 @@ TEST(ConsumerStateTable, singlethread) } int numberOfKeyDeleted = 0; - while ((ret = cs.select(&selectcs, &tmpfd)) == Select::OBJECT) + while ((ret = cs.select(&selectcs)) == Select::OBJECT) { c.pop(kco); EXPECT_TRUE(kfvOp(kco) == "DEL"); @@ -479,9 +474,8 @@ TEST(ConsumerStateTable, multitable) while (1) { Selectable *is; - int fd; - ret = cs.select(&is, &fd); + ret = cs.select(&is); EXPECT_EQ(ret, Select::OBJECT); ((ConsumerStateTable *)is)->pop(kco); diff --git a/tests/redis_subscriber_state_ut.cpp b/tests/redis_subscriber_state_ut.cpp index 25d1398b8..2b9fd7e67 100644 --- a/tests/redis_subscriber_state_ut.cpp +++ b/tests/redis_subscriber_state_ut.cpp @@ -128,7 +128,6 @@ static void subscriberWorker(int index, int *status) SubscriberStateTable c(&db, testTableName); Select cs; Selectable *selectcs; - int tmpfd; int numberOfKeysSet = 0; int numberOfKeyDeleted = 0; int ret, i = 0; @@ -138,7 +137,7 @@ static void subscriberWorker(int index, int *status) status[index] = 1; - while ((ret = cs.select(&selectcs, &tmpfd, 10000)) == Select::OBJECT) + while ((ret = cs.select(&selectcs, 10000)) == Select::OBJECT) { c.pop(kco); if (kfvOp(kco) == "SET") @@ -167,7 +166,7 @@ static void subscriberWorker(int index, int *status) /* Verify that all data are read */ { - ret = cs.select(&selectcs, &tmpfd, 1000); + ret = cs.select(&selectcs, 1000); EXPECT_TRUE(ret == Select::TIMEOUT); } } @@ -200,11 +199,9 @@ TEST(SubscriberStateTable, set) p.set(key, fields); } - int tmpfd; - /* Pop operation */ { - int ret = cs.select(&selectcs, &tmpfd); + int ret = cs.select(&selectcs); EXPECT_TRUE(ret == Select::OBJECT); KeyOpFieldsValuesTuple kco; c.pop(kco); @@ -255,11 +252,9 @@ TEST(SubscriberStateTable, del) p.set(key, fields); } - int tmpfd; - /* Pop operation for set */ { - int ret = cs.select(&selectcs, &tmpfd); + int ret = cs.select(&selectcs); EXPECT_TRUE(ret == Select::OBJECT); KeyOpFieldsValuesTuple kco; c.pop(kco); @@ -271,7 +266,7 @@ TEST(SubscriberStateTable, del) /* Pop operation for del */ { - int ret = cs.select(&selectcs, &tmpfd); + int ret = cs.select(&selectcs); EXPECT_TRUE(ret == Select::OBJECT); KeyOpFieldsValuesTuple kco; c.pop(kco); @@ -311,14 +306,13 @@ TEST(SubscriberStateTable, table_state) SubscriberStateTable c(&db, testTableName); Select cs; Selectable *selectcs; - int tmpfd; int ret, i = 0; KeyOpFieldsValuesTuple kco; cs.addSelectable(&c); int numberOfKeysSet = 0; - while ((ret = cs.select(&selectcs, &tmpfd)) == Select::OBJECT) + while ((ret = cs.select(&selectcs)) == Select::OBJECT) { c.pop(kco); EXPECT_TRUE(kfvOp(kco) == "SET"); @@ -336,7 +330,7 @@ TEST(SubscriberStateTable, table_state) /* Verify that all data are read */ { - ret = cs.select(&selectcs, &tmpfd, 1000); + ret = cs.select(&selectcs, 1000); EXPECT_TRUE(ret == Select::TIMEOUT); } } diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp index b46f9fe02..38d4b6d8a 100644 --- a/tests/redis_ut.cpp +++ b/tests/redis_ut.cpp @@ -106,14 +106,13 @@ void consumerWorker(int index) ConsumerTable c(&db, tableName); Select cs; Selectable *selectcs; - int tmpfd; int numberOfKeysSet = 0; int numberOfKeyDeleted = 0; int ret, i = 0; KeyOpFieldsValuesTuple kco; cs.addSelectable(&c); - while ((ret = cs.select(&selectcs, &tmpfd)) == Select::OBJECT) + while ((ret = cs.select(&selectcs)) == Select::OBJECT) { c.pop(kco); if (kfvOp(kco) == "SET") @@ -290,9 +289,8 @@ TEST(DBConnector, multitable) while (1) { Selectable *is; - int fd; - ret = cs.select(&is, &fd); + ret = cs.select(&is); EXPECT_EQ(ret, Select::OBJECT); ((ConsumerTable *)is)->pop(kco); @@ -346,13 +344,13 @@ TEST(DBConnector, notifications) Select s; s.addSelectable(&nc); Selectable *sel; - int fd, value = 1; + int value = 1; clearDB(); thread np(notificationProducer); - int result = s.select(&sel, &fd, 2000); + int result = s.select(&sel, 2000); if (result == Select::OBJECT) { cout << "Got notification from producer" << endl; @@ -382,11 +380,10 @@ void selectableEventThread(Selectable *ev, int *value) Select s; s.addSelectable(ev); Selectable *sel; - int fd; cout << "Starting listening ... " << endl; - int result = s.select(&sel, &fd, 2000); + int result = s.select(&sel, 2000); if (result == Select::OBJECT) { if (sel == ev) @@ -421,30 +418,30 @@ TEST(DBConnector, selectabletimer) Select s; s.addSelectable(&timer); Selectable *sel; - int fd, result; + int result; // Wait a non started timer - result = s.select(&sel, &fd, 2000); + result = s.select(&sel, 2000); ASSERT_EQ(result, Select::TIMEOUT); // Wait long enough so we got timer notification first timer.start(); - result = s.select(&sel, &fd, 2000); + result = s.select(&sel, 2000); ASSERT_EQ(result, Select::OBJECT); ASSERT_EQ(sel, &timer); // Wait short so we got select timeout first - result = s.select(&sel, &fd, 10); + result = s.select(&sel, 10); ASSERT_EQ(result, Select::TIMEOUT); // Wait long enough so we got timer notification first - result = s.select(&sel, &fd, 10000); + result = s.select(&sel, 10000); ASSERT_EQ(result, Select::OBJECT); ASSERT_EQ(sel, &timer); // Reset and wait long enough so we got timer notification first timer.reset(); - result = s.select(&sel, &fd, 10000); + result = s.select(&sel, 10000); ASSERT_EQ(result, Select::OBJECT); ASSERT_EQ(sel, &timer); } diff --git a/tests/selectable_priority.cpp b/tests/selectable_priority.cpp index 2e68dda92..39865027b 100644 --- a/tests/selectable_priority.cpp +++ b/tests/selectable_priority.cpp @@ -89,17 +89,16 @@ TEST(Prioriry, priority_select_1) s2.notify(); s3.notify(); - int tmpfd; int ret; - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_EQ(selectcs, &s3); - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_EQ(selectcs, &s2); - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_EQ(selectcs, &s1); } @@ -121,17 +120,16 @@ TEST(Prioriry, priority_select_2) cs.addSelectable(&s2); cs.addSelectable(&s3); - int tmpfd; int ret; - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_EQ(selectcs, &s3); - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_EQ(selectcs, &s2); - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_EQ(selectcs, &s1); } @@ -153,17 +151,16 @@ TEST(Prioriry, priority_select_3) s2.notify(); s3.notify(); - int tmpfd; int ret; - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_EQ(selectcs, &s3); - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_TRUE(selectcs==&s1 || selectcs==&s2); - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_TRUE(selectcs==&s1 || selectcs==&s2); } @@ -185,17 +182,16 @@ TEST(Prioriry, priority_select_4) s2.notify(); s3.notify(); - int tmpfd; int ret; - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_TRUE(selectcs==&s2 || selectcs==&s3); - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_TRUE(selectcs==&s2 || selectcs==&s3); - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_EQ(selectcs, &s1); } @@ -214,14 +210,13 @@ TEST(Prioriry, priority_select_5) s1.notify(); s2.notify(); - int tmpfd; int ret; - ret = cs.select(&selectcs, &tmpfd); + ret = cs.select(&selectcs); EXPECT_EQ(ret, Select::OBJECT); EXPECT_EQ(selectcs, &s2); cs.removeSelectable(&s1); - ret = cs.select(&selectcs, &tmpfd, 1000); + ret = cs.select(&selectcs, 1000); EXPECT_EQ(ret, Select::TIMEOUT); } diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py index 3cc36b130..3367ca838 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -34,7 +34,7 @@ def test_SubscriberStateTable(): sel.addSelectable(cst) fvs = swsscommon.FieldValuePairs([('a','b')]) t.set("aaa", fvs) - (state, c, fd) = sel.select() + (state, c) = sel.select() assert state == swsscommon.Select.OBJECT (key, op, cfvs) = cst.pop() assert key == "aaa" @@ -50,7 +50,7 @@ def test_Notification(): fvs = swsscommon.FieldValuePairs([('a','b')]) ntfp = swsscommon.NotificationProducer(db, "testntf") ntfp.send("aaa", "bbb", fvs) - (state, c, fd) = sel.select() + (state, c) = sel.select() assert state == swsscommon.Select.OBJECT (op, data, cfvs) = ntfc.pop() assert op == "aaa" From bac891b47e143b1903b8c516042c4fc0410cba12 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Tue, 3 Apr 2018 13:30:27 -0700 Subject: [PATCH 03/13] Use references as loop variables --- common/ipaddresses.cpp | 4 ++-- common/json.cpp | 2 +- common/producerstatetable.cpp | 2 +- common/producertable.cpp | 2 +- common/rediscommand.cpp | 2 +- common/select.cpp | 2 +- common/subscriberstatetable.cpp | 4 ++-- common/table.cpp | 2 +- 8 files changed, 10 insertions(+), 10 deletions(-) diff --git a/common/ipaddresses.cpp b/common/ipaddresses.cpp index 8346c3ee9..042b18d68 100644 --- a/common/ipaddresses.cpp +++ b/common/ipaddresses.cpp @@ -11,7 +11,7 @@ using namespace swss; IpAddresses::IpAddresses(const string &ipsStr) { auto ips = tokenize(ipsStr, IP_DELIMITER); - for (auto ip : ips) + for (const auto& ip : ips) m_ips.insert(ip); } @@ -39,7 +39,7 @@ bool IpAddresses::contains(const IpAddress &ip) const bool IpAddresses::contains(const IpAddresses &ips) const { - for (auto ip : ips.getIpAddresses()) + for (const auto& ip : ips.getIpAddresses()) { if (!contains(ip)) { diff --git a/common/json.cpp b/common/json.cpp index e618a2e73..aaea64d17 100644 --- a/common/json.cpp +++ b/common/json.cpp @@ -13,7 +13,7 @@ string JSon::buildJson(const vector &fv) nlohmann::json j = nlohmann::json::array(); // we use array to save order - for (auto &i : fv) + for (const auto& i : fv) { j.push_back(fvField(i)); j.push_back(fvValue(i)); diff --git a/common/producerstatetable.cpp b/common/producerstatetable.cpp index 9eee190d9..347d1fbca 100644 --- a/common/producerstatetable.cpp +++ b/common/producerstatetable.cpp @@ -69,7 +69,7 @@ void ProducerStateTable::set(string key, vector &values, args.push_back("G"); args.push_back(key); - for (auto& iv: values) + for (const auto& iv: values) { args.push_back(fvField(iv)); args.push_back(fvValue(iv)); diff --git a/common/producertable.cpp b/common/producertable.cpp index b06402d65..afcebf384 100644 --- a/common/producertable.cpp +++ b/common/producertable.cpp @@ -89,7 +89,7 @@ void ProducerTable::set(string key, vector &values, string op, json j; string json_key = getKeyName(key); j[json_key] = json::object(); - for (auto it : values) + for (const auto& it : values) j[json_key][fvField(it)] = fvValue(it); j["OP"] = op; m_dumpFile << j.dump(4); diff --git a/common/rediscommand.cpp b/common/rediscommand.cpp index 10d96c7d1..8791e2abb 100644 --- a/common/rediscommand.cpp +++ b/common/rediscommand.cpp @@ -47,7 +47,7 @@ void RedisCommand::formatHMSET(const std::string &key, std::vector args = { cmd, key.c_str() }; - for (const auto &fvt: values) + for (const auto& fvt: values) { args.push_back(fvField(fvt).c_str()); args.push_back(fvValue(fvt).c_str()); diff --git a/common/select.cpp b/common/select.cpp index 996fa1ac1..18de53628 100644 --- a/common/select.cpp +++ b/common/select.cpp @@ -81,7 +81,7 @@ void Select::removeSelectable(Selectable *selectable) void Select::addSelectables(vector selectables) { - for(auto it : selectables) + for(const auto& it : selectables) { addSelectable(it); } diff --git a/common/subscriberstatetable.cpp b/common/subscriberstatetable.cpp index 78887c001..86276c5d3 100644 --- a/common/subscriberstatetable.cpp +++ b/common/subscriberstatetable.cpp @@ -27,7 +27,7 @@ SubscriberStateTable::SubscriberStateTable(DBConnector *db, string tableName, in vector keys; m_table.getKeys(keys); - for (auto key: keys) + for (const auto& key: keys) { KeyOpFieldsValuesTuple kco; @@ -98,7 +98,7 @@ void SubscriberStateTable::pops(deque &vkco, string /*pr return; } - while (auto event = popEventBuffer()) + while (const auto& event = popEventBuffer()) { KeyOpFieldsValuesTuple kco; /* if the Key-space notification is empty, try next one. */ diff --git a/common/table.cpp b/common/table.cpp index bdc06e52d..485178f73 100644 --- a/common/table.cpp +++ b/common/table.cpp @@ -98,7 +98,7 @@ void TableEntryEnumerable::getContent(vector &tuples) tuples.clear(); - for (auto key: keys) + for (const auto& key: keys) { vector values; string op = ""; From fb613146dc70c8caa1bc47549cb6bbf6ddd511d8 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Tue, 10 Apr 2018 17:37:42 -0700 Subject: [PATCH 04/13] Make select fair for selectables. If we have selectables with equal priority, select will choose a selectable which was chosen before all others. So one selectable can't block others This commit also contains typo fix --- common/select.cpp | 11 ++++++--- common/selectable.h | 35 ++++++++++++++++++++++++----- tests/selectable_priority.cpp | 42 +++++++++++++++++++++++++++++------ 3 files changed, 73 insertions(+), 15 deletions(-) diff --git a/common/select.cpp b/common/select.cpp index 18de53628..140c96e36 100644 --- a/common/select.cpp +++ b/common/select.cpp @@ -116,10 +116,15 @@ int Select::select1(Selectable **c, unsigned int timeout) *c = sel; - if (!sel->hasCachedData()) + m_ready.erase(sel); + // we must update clock only when the selector out of the m_ready + // otherwise we break invariant of the m_ready + sel->updateClock(); + + if (sel->hasCachedData()) { - // remove Selectable from the ready when there're no messages anymore - m_ready.erase(sel); + // reinsert Selectable back to the m_ready set, when there're more messages in the cache + m_ready.insert(sel); } sel->updateAfterRead(); diff --git a/common/selectable.h b/common/selectable.h index fbd28169f..fe9e695ec 100644 --- a/common/selectable.h +++ b/common/selectable.h @@ -4,6 +4,7 @@ #include #include #include +#include #include namespace swss { @@ -11,7 +12,9 @@ namespace swss { class Selectable { public: - Selectable(int pri = 0) : m_priority(pri) {} + Selectable(int pri = 0) : m_priority(pri), + m_last_time_used(std::chrono::steady_clock::now()) {} + virtual ~Selectable() {}; /* return file handler for the Selectable */ @@ -42,19 +45,41 @@ class Selectable return m_priority; } + std::chrono::time_point getLastTimeUsed() const + { + return m_last_time_used; + } + + void updateClock() + { + m_last_time_used = std::chrono::steady_clock::now(); + } + struct cmp { bool operator()(const Selectable* a, const Selectable* b) const { - if (a->getPri() == b->getPri()) - return a > b; - else - return a->getPri() > b->getPri(); + /* Choose Selectable with highest priority first */ + if (a->getPri() > b->getPri()) + return true; + else if (a->getPri() < b->getPri()) + return false; + + /* if the priorities are equal */ + /* use Selectable which was selected later */ + if (a->getLastTimeUsed() < b->getLastTimeUsed()) + return true; + else if (a->getLastTimeUsed() > b->getLastTimeUsed()) + return false; + + /* an impossible case, but prefer a Selectable with lowest address */ + return a < b; } }; private: int m_priority; + std::chrono::time_point m_last_time_used; }; } diff --git a/tests/selectable_priority.cpp b/tests/selectable_priority.cpp index 39865027b..1c33e087d 100644 --- a/tests/selectable_priority.cpp +++ b/tests/selectable_priority.cpp @@ -18,7 +18,7 @@ using namespace swss; #define DEFAULT_POP_BATCH_SIZE (10) -TEST(Prioriry, default_pri_values) +TEST(Priority, default_pri_values) { std::string tableName = "tableName"; @@ -45,7 +45,7 @@ TEST(Prioriry, default_pri_values) EXPECT_EQ(sst.getPri(), 0); } -TEST(Prioriry, set_pri_values) +TEST(Priority, set_pri_values) { std::string tableName = "tableName"; @@ -72,7 +72,7 @@ TEST(Prioriry, set_pri_values) EXPECT_EQ(sst.getPri(), 108); } -TEST(Prioriry, priority_select_1) +TEST(Priority, priority_select_1) { Select cs; Selectable *selectcs; @@ -103,7 +103,7 @@ TEST(Prioriry, priority_select_1) EXPECT_EQ(selectcs, &s1); } -TEST(Prioriry, priority_select_2) +TEST(Priority, priority_select_2) { Select cs; Selectable *selectcs; @@ -134,7 +134,7 @@ TEST(Prioriry, priority_select_2) EXPECT_EQ(selectcs, &s1); } -TEST(Prioriry, priority_select_3) +TEST(Priority, priority_select_3) { Select cs; Selectable *selectcs; @@ -165,7 +165,7 @@ TEST(Prioriry, priority_select_3) EXPECT_TRUE(selectcs==&s1 || selectcs==&s2); } -TEST(Prioriry, priority_select_4) +TEST(Priority, priority_select_4) { Select cs; Selectable *selectcs; @@ -196,7 +196,7 @@ TEST(Prioriry, priority_select_4) EXPECT_EQ(selectcs, &s1); } -TEST(Prioriry, priority_select_5) +TEST(Priority, priority_select_5) { Select cs; Selectable *selectcs; @@ -220,3 +220,31 @@ TEST(Prioriry, priority_select_5) ret = cs.select(&selectcs, 1000); EXPECT_EQ(ret, Select::TIMEOUT); } + +TEST(Priority, priority_select_6) +{ + Select cs; + Selectable *selectcs1; + Selectable *selectcs2; + + SelectableEvent s1(1000); + SelectableEvent s2(1000); + + cs.addSelectable(&s1); + cs.addSelectable(&s2); + + s1.notify(); + s2.notify(); + + int ret; + ret = cs.select(&selectcs1); + EXPECT_EQ(ret, Select::OBJECT); + + s1.notify(); + s2.notify(); + + ret = cs.select(&selectcs2); + EXPECT_EQ(ret, Select::OBJECT); + // we gave fair scheduler. we've read different selectables on the second read + EXPECT_NE(selectcs1, selectcs2); +} From 8e328aa984e7fbceb1b38691dcc8e4a74366974d Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Wed, 11 Apr 2018 11:29:28 -0700 Subject: [PATCH 05/13] Revert "Use references as loop variables" This reverts commit bac891b47e143b1903b8c516042c4fc0410cba12. --- common/ipaddresses.cpp | 4 ++-- common/json.cpp | 2 +- common/producerstatetable.cpp | 2 +- common/producertable.cpp | 2 +- common/rediscommand.cpp | 2 +- common/select.cpp | 2 +- common/subscriberstatetable.cpp | 4 ++-- common/table.cpp | 2 +- 8 files changed, 10 insertions(+), 10 deletions(-) diff --git a/common/ipaddresses.cpp b/common/ipaddresses.cpp index 042b18d68..8346c3ee9 100644 --- a/common/ipaddresses.cpp +++ b/common/ipaddresses.cpp @@ -11,7 +11,7 @@ using namespace swss; IpAddresses::IpAddresses(const string &ipsStr) { auto ips = tokenize(ipsStr, IP_DELIMITER); - for (const auto& ip : ips) + for (auto ip : ips) m_ips.insert(ip); } @@ -39,7 +39,7 @@ bool IpAddresses::contains(const IpAddress &ip) const bool IpAddresses::contains(const IpAddresses &ips) const { - for (const auto& ip : ips.getIpAddresses()) + for (auto ip : ips.getIpAddresses()) { if (!contains(ip)) { diff --git a/common/json.cpp b/common/json.cpp index aaea64d17..e618a2e73 100644 --- a/common/json.cpp +++ b/common/json.cpp @@ -13,7 +13,7 @@ string JSon::buildJson(const vector &fv) nlohmann::json j = nlohmann::json::array(); // we use array to save order - for (const auto& i : fv) + for (auto &i : fv) { j.push_back(fvField(i)); j.push_back(fvValue(i)); diff --git a/common/producerstatetable.cpp b/common/producerstatetable.cpp index 347d1fbca..9eee190d9 100644 --- a/common/producerstatetable.cpp +++ b/common/producerstatetable.cpp @@ -69,7 +69,7 @@ void ProducerStateTable::set(string key, vector &values, args.push_back("G"); args.push_back(key); - for (const auto& iv: values) + for (auto& iv: values) { args.push_back(fvField(iv)); args.push_back(fvValue(iv)); diff --git a/common/producertable.cpp b/common/producertable.cpp index afcebf384..b06402d65 100644 --- a/common/producertable.cpp +++ b/common/producertable.cpp @@ -89,7 +89,7 @@ void ProducerTable::set(string key, vector &values, string op, json j; string json_key = getKeyName(key); j[json_key] = json::object(); - for (const auto& it : values) + for (auto it : values) j[json_key][fvField(it)] = fvValue(it); j["OP"] = op; m_dumpFile << j.dump(4); diff --git a/common/rediscommand.cpp b/common/rediscommand.cpp index 8791e2abb..10d96c7d1 100644 --- a/common/rediscommand.cpp +++ b/common/rediscommand.cpp @@ -47,7 +47,7 @@ void RedisCommand::formatHMSET(const std::string &key, std::vector args = { cmd, key.c_str() }; - for (const auto& fvt: values) + for (const auto &fvt: values) { args.push_back(fvField(fvt).c_str()); args.push_back(fvValue(fvt).c_str()); diff --git a/common/select.cpp b/common/select.cpp index 140c96e36..b024ecca1 100644 --- a/common/select.cpp +++ b/common/select.cpp @@ -81,7 +81,7 @@ void Select::removeSelectable(Selectable *selectable) void Select::addSelectables(vector selectables) { - for(const auto& it : selectables) + for(auto it : selectables) { addSelectable(it); } diff --git a/common/subscriberstatetable.cpp b/common/subscriberstatetable.cpp index 86276c5d3..78887c001 100644 --- a/common/subscriberstatetable.cpp +++ b/common/subscriberstatetable.cpp @@ -27,7 +27,7 @@ SubscriberStateTable::SubscriberStateTable(DBConnector *db, string tableName, in vector keys; m_table.getKeys(keys); - for (const auto& key: keys) + for (auto key: keys) { KeyOpFieldsValuesTuple kco; @@ -98,7 +98,7 @@ void SubscriberStateTable::pops(deque &vkco, string /*pr return; } - while (const auto& event = popEventBuffer()) + while (auto event = popEventBuffer()) { KeyOpFieldsValuesTuple kco; /* if the Key-space notification is empty, try next one. */ diff --git a/common/table.cpp b/common/table.cpp index 485178f73..bdc06e52d 100644 --- a/common/table.cpp +++ b/common/table.cpp @@ -98,7 +98,7 @@ void TableEntryEnumerable::getContent(vector &tuples) tuples.clear(); - for (const auto& key: keys) + for (auto key: keys) { vector values; string op = ""; From 5b76223f98dc5dd05642ac751ba84b8be3537906 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Wed, 11 Apr 2018 11:39:21 -0700 Subject: [PATCH 06/13] Revert performance changes --- common/ipaddress.h | 2 +- common/ipprefix.cpp | 5 +++-- common/logger.cpp | 6 +++--- common/logger.h | 6 +++--- common/redispipeline.h | 2 +- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/common/ipaddress.h b/common/ipaddress.h index 7ad5295d6..ccb370e4b 100644 --- a/common/ipaddress.h +++ b/common/ipaddress.h @@ -20,7 +20,7 @@ class IpAddress { public: IpAddress() {} - IpAddress(const ip_addr_t &ip) : m_ip(ip) {} + IpAddress(const ip_addr_t &ip) { m_ip = ip; } IpAddress(uint32_t ip); IpAddress(const std::string &ipStr); diff --git a/common/ipprefix.cpp b/common/ipprefix.cpp index 56fe96258..7e06091c8 100644 --- a/common/ipprefix.cpp +++ b/common/ipprefix.cpp @@ -45,9 +45,10 @@ IpPrefix::IpPrefix( } } -IpPrefix::IpPrefix(uint32_t ipPrefix, int mask) : m_ip(ipPrefix), - m_mask(mask) +IpPrefix::IpPrefix(uint32_t ipPrefix, int mask) { + m_ip = IpAddress(ipPrefix); + m_mask = mask; if (!isValid()) { throw std::invalid_argument("Invalid IpPrefix from prefix and mask"); diff --git a/common/logger.cpp b/common/logger.cpp index f24f7cbbd..c2a5329c1 100644 --- a/common/logger.cpp +++ b/common/logger.cpp @@ -70,7 +70,7 @@ void Logger::swssOutputNotify(std::string component, std::string outputStr) } } -void Logger::linkToDbWithOutput(const std::string& dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio, const OutputChangeNotify& outputNotify, const std::string& defOutput) +void Logger::linkToDbWithOutput(const std::string dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio, const OutputChangeNotify& outputNotify, const std::string& defOutput) { auto& logger = getInstance(); @@ -122,12 +122,12 @@ void Logger::linkToDbWithOutput(const std::string& dbName, const PriorityChangeN outputNotify(key, output); } -void Logger::linkToDb(const std::string& dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio) +void Logger::linkToDb(const std::string dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio) { linkToDbWithOutput(dbName, prioNotify, defPrio, swssOutputNotify, "SYSLOG"); } -void Logger::linkToDbNative(const std::string& dbName) +void Logger::linkToDbNative(const std::string dbName) { auto& logger = getInstance(); diff --git a/common/logger.h b/common/logger.h index 8b0933fab..664d24deb 100644 --- a/common/logger.h +++ b/common/logger.h @@ -55,10 +55,10 @@ class Logger static Logger &getInstance(); static void setMinPrio(Priority prio); static Priority getMinPrio(); - static void linkToDbWithOutput(const std::string& dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio, const OutputChangeNotify& outputNotify, const std::string& defOutput); - static void linkToDb(const std::string& dbName, const PriorityChangeNotify& notify, const std::string& defPrio); + static void linkToDbWithOutput(const std::string dbName, const PriorityChangeNotify& prioNotify, const std::string& defPrio, const OutputChangeNotify& outputNotify, const std::string& defOutput); + static void linkToDb(const std::string dbName, const PriorityChangeNotify& notify, const std::string& defPrio); // Must be called after all linkToDb to start select from DB - static void linkToDbNative(const std::string& dbName); + static void linkToDbNative(const std::string dbName); void write(Priority prio, const char *fmt, ...) #ifdef __GNUC__ __attribute__ ((format (printf, 3, 4))) diff --git a/common/redispipeline.h b/common/redispipeline.h index 2cb7753c1..88cfd23f0 100644 --- a/common/redispipeline.h +++ b/common/redispipeline.h @@ -64,7 +64,7 @@ class RedisPipeline { if (m_remaining == 0) return NULL; redisReply *reply; - redisGetReply(m_db->getContext(), reinterpret_cast(&reply)); + redisGetReply(m_db->getContext(), (void**)&reply); RedisReply r(reply); m_remaining--; From 4d3bcea6ff22219b673455c870065124c89e2160 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Wed, 11 Apr 2018 15:43:02 -0700 Subject: [PATCH 07/13] Address comments --- common/select.cpp | 8 ++++---- common/select.h | 4 ++-- common/selectable.h | 3 ++- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/common/select.cpp b/common/select.cpp index b024ecca1..2a418972b 100644 --- a/common/select.cpp +++ b/common/select.cpp @@ -87,7 +87,7 @@ void Select::addSelectables(vector selectables) } } -int Select::select1(Selectable **c, unsigned int timeout) +int Select::check_descriptors(Selectable **c, unsigned int timeout) { int sz_selectables = static_cast(m_objects.size()); std::vector events(sz_selectables); @@ -110,7 +110,7 @@ int Select::select1(Selectable **c, unsigned int timeout) m_ready.insert(sel); } - while (!m_ready.empty()) + if (!m_ready.empty()) { auto sel = *m_ready.begin(); @@ -146,14 +146,14 @@ int Select::select(Selectable **c, unsigned int timeout) timeout = -1; /* check if we have some data */ - ret = select1(c, 0); + ret = check_descriptors(c, 0); /* return if we have data, we have an error or desired timeout was 0 */ if (ret != Select::TIMEOUT || timeout == 0) return ret; /* wait for data */ - ret = select1(c, timeout); + ret = check_descriptors(c, timeout); return ret; diff --git a/common/select.h b/common/select.h index 3b5438e6b..079a2c71b 100644 --- a/common/select.h +++ b/common/select.h @@ -24,7 +24,7 @@ class Select /* Remove object from select */ void removeSelectable(Selectable *selectable); - /* Add multiple messages for select */ + /* Add multiple objects for select */ void addSelectables(std::vector selectables); enum { @@ -36,7 +36,7 @@ class Select int select(Selectable **c, unsigned int timeout = std::numeric_limits::max()); private: - int select1(Selectable **c, unsigned int timeout); + int check_descriptors(Selectable **c, unsigned int timeout); int m_epoll_fd; std::unordered_map m_objects; diff --git a/common/selectable.h b/common/selectable.h index fe9e695ec..1c6c745cd 100644 --- a/common/selectable.h +++ b/common/selectable.h @@ -78,7 +78,8 @@ class Selectable }; private: - int m_priority; + int m_priority; // defines priority of Selectable inside Select + // higher value is higher priority std::chrono::time_point m_last_time_used; }; From e835cac22144763e206697644ec5793f2a10e5d6 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Wed, 11 Apr 2018 16:04:08 -0700 Subject: [PATCH 08/13] Make the comparator comment clearer --- common/selectable.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/selectable.h b/common/selectable.h index 1c6c745cd..6fdd62167 100644 --- a/common/selectable.h +++ b/common/selectable.h @@ -72,8 +72,8 @@ class Selectable else if (a->getLastTimeUsed() > b->getLastTimeUsed()) return false; - /* an impossible case, but prefer a Selectable with lowest address */ - return a < b; + /* when a == b */ + return false; } }; From e428d9c758e5857d8ff724197bd0148e9c6d8ae2 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Thu, 12 Apr 2018 11:31:38 -0700 Subject: [PATCH 09/13] Use right EINTR error code for netlink function --- common/consumertablebase.cpp | 1 - common/netlink.cpp | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/common/consumertablebase.cpp b/common/consumertablebase.cpp index 26412346a..c82b545fb 100644 --- a/common/consumertablebase.cpp +++ b/common/consumertablebase.cpp @@ -2,7 +2,6 @@ namespace swss { - ConsumerTableBase::ConsumerTableBase(DBConnector *db, std::string tableName, int popBatchSize, int pri): TableConsumable(db->getDbId(), tableName, pri), RedisTransactioner(db), diff --git a/common/netlink.cpp b/common/netlink.cpp index 6940de3e2..1db9f3af2 100644 --- a/common/netlink.cpp +++ b/common/netlink.cpp @@ -84,7 +84,7 @@ void NetLink::readData() { err = nl_recvmsgs_default(m_socket); } - while(err == -EINTR); // Retry if the process was interrupted by a signal + while(err == -NLE_INTR); // Retry if the process was interrupted by a signal if (err < 0) { From 842b5b0f7ef4de68a341fb3a51cb30631fc0e7f9 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Fri, 13 Apr 2018 11:11:52 -0700 Subject: [PATCH 10/13] Move Selectable comparator to the Select class as a private functional object, to avoid it using by others --- common/select.h | 24 +++++++++++++++++++++++- common/selectable.h | 22 ---------------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/common/select.h b/common/select.h index 079a2c71b..42ed1f17f 100644 --- a/common/select.h +++ b/common/select.h @@ -36,11 +36,33 @@ class Select int select(Selectable **c, unsigned int timeout = std::numeric_limits::max()); private: + struct cmp + { + bool operator()(const Selectable* a, const Selectable* b) const + { + /* Choose Selectable with highest priority first */ + if (a->getPri() > b->getPri()) + return true; + else if (a->getPri() < b->getPri()) + return false; + + /* if the priorities are equal */ + /* use Selectable which was selected later */ + if (a->getLastTimeUsed() < b->getLastTimeUsed()) + return true; + else if (a->getLastTimeUsed() > b->getLastTimeUsed()) + return false; + + /* when a == b */ + return false; + } + }; + int check_descriptors(Selectable **c, unsigned int timeout); int m_epoll_fd; std::unordered_map m_objects; - std::set m_ready; + std::set m_ready; }; } diff --git a/common/selectable.h b/common/selectable.h index 6fdd62167..2d273572d 100644 --- a/common/selectable.h +++ b/common/selectable.h @@ -55,28 +55,6 @@ class Selectable m_last_time_used = std::chrono::steady_clock::now(); } - struct cmp - { - bool operator()(const Selectable* a, const Selectable* b) const - { - /* Choose Selectable with highest priority first */ - if (a->getPri() > b->getPri()) - return true; - else if (a->getPri() < b->getPri()) - return false; - - /* if the priorities are equal */ - /* use Selectable which was selected later */ - if (a->getLastTimeUsed() < b->getLastTimeUsed()) - return true; - else if (a->getLastTimeUsed() > b->getLastTimeUsed()) - return false; - - /* when a == b */ - return false; - } - }; - private: int m_priority; // defines priority of Selectable inside Select // higher value is higher priority From 4d711bbebb476e4073c83a22d2e36387ab07f629 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Fri, 13 Apr 2018 15:41:51 -0700 Subject: [PATCH 11/13] Hide m_last_used_time from all classes except Select --- common/selectable.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/common/selectable.h b/common/selectable.h index 2d273572d..260f173b4 100644 --- a/common/selectable.h +++ b/common/selectable.h @@ -45,6 +45,12 @@ class Selectable return m_priority; } +private: + + friend class Select; + + // only Select class can access and update m_last_time_used + std::chrono::time_point getLastTimeUsed() const { return m_last_time_used; @@ -55,7 +61,7 @@ class Selectable m_last_time_used = std::chrono::steady_clock::now(); } -private: + int m_priority; // defines priority of Selectable inside Select // higher value is higher priority std::chrono::time_point m_last_time_used; From 21b344507f4d65024225dab963597d02efff8ddb Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Fri, 13 Apr 2018 15:45:52 -0700 Subject: [PATCH 12/13] Rename some class members --- common/select.cpp | 6 +++--- common/select.h | 2 +- common/selectable.h | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/common/select.cpp b/common/select.cpp index 2a418972b..e72d5a22d 100644 --- a/common/select.cpp +++ b/common/select.cpp @@ -87,7 +87,7 @@ void Select::addSelectables(vector selectables) } } -int Select::check_descriptors(Selectable **c, unsigned int timeout) +int Select::poll_descriptors(Selectable **c, unsigned int timeout) { int sz_selectables = static_cast(m_objects.size()); std::vector events(sz_selectables); @@ -146,14 +146,14 @@ int Select::select(Selectable **c, unsigned int timeout) timeout = -1; /* check if we have some data */ - ret = check_descriptors(c, 0); + ret = poll_descriptors(c, 0); /* return if we have data, we have an error or desired timeout was 0 */ if (ret != Select::TIMEOUT || timeout == 0) return ret; /* wait for data */ - ret = check_descriptors(c, timeout); + ret = poll_descriptors(c, timeout); return ret; diff --git a/common/select.h b/common/select.h index 42ed1f17f..e8ceceb3f 100644 --- a/common/select.h +++ b/common/select.h @@ -58,7 +58,7 @@ class Select } }; - int check_descriptors(Selectable **c, unsigned int timeout); + int poll_descriptors(Selectable **c, unsigned int timeout); int m_epoll_fd; std::unordered_map m_objects; diff --git a/common/selectable.h b/common/selectable.h index 260f173b4..ed9bc2e6c 100644 --- a/common/selectable.h +++ b/common/selectable.h @@ -13,7 +13,7 @@ class Selectable { public: Selectable(int pri = 0) : m_priority(pri), - m_last_time_used(std::chrono::steady_clock::now()) {} + m_last_used_time(std::chrono::steady_clock::now()) {} virtual ~Selectable() {}; @@ -49,22 +49,22 @@ class Selectable friend class Select; - // only Select class can access and update m_last_time_used + // only Select class can access and update m_last_used_time std::chrono::time_point getLastTimeUsed() const { - return m_last_time_used; + return m_last_used_time; } void updateClock() { - m_last_time_used = std::chrono::steady_clock::now(); + m_last_used_time = std::chrono::steady_clock::now(); } int m_priority; // defines priority of Selectable inside Select // higher value is higher priority - std::chrono::time_point m_last_time_used; + std::chrono::time_point m_last_used_time; }; } From 73596600d227c710b59b5668dc45e93e86a966a9 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Fri, 13 Apr 2018 15:53:24 -0700 Subject: [PATCH 13/13] Make more consistent naming for LastUsedTime of selectable --- common/select.cpp | 2 +- common/select.h | 4 ++-- common/selectable.h | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/common/select.cpp b/common/select.cpp index e72d5a22d..bcfb5389e 100644 --- a/common/select.cpp +++ b/common/select.cpp @@ -119,7 +119,7 @@ int Select::poll_descriptors(Selectable **c, unsigned int timeout) m_ready.erase(sel); // we must update clock only when the selector out of the m_ready // otherwise we break invariant of the m_ready - sel->updateClock(); + sel->updateLastUsedTime(); if (sel->hasCachedData()) { diff --git a/common/select.h b/common/select.h index e8ceceb3f..ab22e202b 100644 --- a/common/select.h +++ b/common/select.h @@ -48,9 +48,9 @@ class Select /* if the priorities are equal */ /* use Selectable which was selected later */ - if (a->getLastTimeUsed() < b->getLastTimeUsed()) + if (a->getLastUsedTime() < b->getLastUsedTime()) return true; - else if (a->getLastTimeUsed() > b->getLastTimeUsed()) + else if (a->getLastUsedTime() > b->getLastUsedTime()) return false; /* when a == b */ diff --git a/common/selectable.h b/common/selectable.h index ed9bc2e6c..f82d489ca 100644 --- a/common/selectable.h +++ b/common/selectable.h @@ -51,12 +51,12 @@ class Selectable // only Select class can access and update m_last_used_time - std::chrono::time_point getLastTimeUsed() const + std::chrono::time_point getLastUsedTime() const { return m_last_used_time; } - void updateClock() + void updateLastUsedTime() { m_last_used_time = std::chrono::steady_clock::now(); }