Refactor Selectables. Add support for selectable priorities by pavel-shirshov · Pull Request #192 · sonic-net/sonic-swss-common

pavel-shirshov · 2018-04-03T20:50:23Z

PLEASE DON'T PUSH THIS CHANGE. IT'LL BREAK BOTH SWSS AND SAIREDIS.

This refactoring introduce support of priorities for Selectables. Selectables with highest priorities will be selected first. If we have two selectables with the same priorities, the selectable which was accessed earliest will be chosen.

The priorities are defined as last argument of type int for following classes:

ConsumerStateTable (default priority is 0)
ConsumerTable (default priority is 0)
Netlink (default priority is 0)
NotificationConsumer (default priority is 100)
RedisSelect (default priority is 0)
SelectableEvent (default priority is 0)
SelectableTimer (default priority is 50)
SubscriberStateTable (default priority is 0)

To support this feature Selectable interface was changed significantly.
Now Selectable has to provide following methods:

getFd() - return file descriptor for the Selectable
readData() - read all data in Selectable. (The data will be stored internally in selectable)
hasCachedData() - return true, if there're data for the next read (don't remove Selectable from the queue is there're the data)
initializedWithData() - return true, if Constructor of selectable read some data inside of it, so we can return it as ready on the next select().
updateAfterRead() - this method will be run when select return this Selectable from itself.

The method Select::select() was changed. int fd parameter was remove as not used anywhere*

I switched from Unix select() to Linux epoll which is more effective and easy to use.

Also I found that SelectableTimer could lose it's events, if reading rate is greater then timer interval. Probably it's a bug.

Also I made some other small changes and tweaks to improve performance of the swss-common. I'm going to create a separate PR with more performance changes for swss-common.

qiluo-msft · 2018-04-03T21:57:39Z

common/ipaddress.h

 public:
    IpAddress() {}
-    IpAddress(const ip_addr_t &ip) { m_ip = ip; }
+    IpAddress(const ip_addr_t &ip) : m_ip(ip) {}


m_ip [](start = 37, length = 4)

Could you separate simple refactoring code like this into a smaller PR? so we could focus on more difficult one. #Closed

I can, but it will take more time to extract all the changes and revert them back. Can you please consider to check them? They're tiny and follows just a few easy patterns:

func(object obj) -> func(const object& obj)

,

for(auto obj: object) -> for (const auto& obj: object)

'''
Class1::Class1(int var1)
{
m_var_1 = var1;
}
to
Class1::Class1(int var1) : m_var_1(var1) {}

#Closed

I am thinking 'selectable improvement' as an experiment feature (not mature now). And other simple one are definitely good contribution to existing codebase. Considering the possibility of future reverting and/or conflicting, it's good to separate. Large amount of files in a PR always harm.

In reply to: 178991122 [](ancestors = 178991122)

I've removed the other changes. I kept only one bug with the missed va_end()

qiluo-msft · 2018-04-03T22:00:55Z

common/select.h

+    ~Select();
+
    /* Add object for select */
    void addSelectable(Selectable *selectable);


addSelectable [](start = 9, length = 13)

suggest move all priority setting as addSelectable parameter, so no need to modify all the related classes. #Closed

The classes are already changed. These changes won't affect any client code.
Also, I don't like to add information about priority into Select class.
Following the principle of Single Responsibility I want Select class only work for polling, not for something else.
Also, it's not easy to change Select::addSelectables() method which expects vector of Selectables.
Also, is makes Select class more complicated, we need to store and operate on the priority, which is Selectable attribute. #Pending

Got your idea. Let me correct my comment. I suggest move all priority setting as function call to Selectable (not Select), keep original constructor signature, so no need to modify all the related sub classes. Another benefit is that all previous code with default priority will keep previous behavior. If any future user would like to apply different priority assignment, he just is able to do so explicitly.

Since all the sub classes has not used pri at all, keep all the modification in base class.

In reply to: 178991671 [](ancestors = 178991671)

I don't agree with you. As soon as we create separate method to change priority it will not work as expected. For example, You create a Selectable, then add it into Select object and then change its priority. It will broke Select object, because it didn't expect anyone changes Selectable objects in the queue.
Also it's bad from design viewpoint. The priority property is 'CREATE_AND_SET' property in SAI terms. We don't want to change it as it created.

qiluo-msft · 2018-04-03T22:03:33Z

You mentioned "To support this feature Selectable interface was changed significantly."
Could you explain the detail? Is it really needed to refactor Selectable interface? #Closed

pavel-shirshov · 2018-04-03T23:47:45Z

I must change Selectable interface because:
Previous interface was:

  /* 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;

In this interface: readMe() allowed us to read information from Selectable which file descriptor signaled.
readCache() allowed us to read information from the cache a Selectable regardless of the file descriptor.
In the previous implementation we first read all cached from all selectables. As soon as they are empty we start reading from first signaled file descriptors. If the file descriptor populated cache, we read that cache on the next iteration.
If we want to introduce priority we must check the file descriptor on every iteration. We can't read cache value until we have it. We can have higher priority information in the sockets awaiting for us.

Also I changed the previous Selectable interface because it leaked Select class methods into Selectables.
Selectables should know nothing about how Select is implemented. So isMe() and addFd() is leaked abstractions. Also the old interface had no way to remove selectable from the queue.

qiluo-msft · 2018-04-06T21:33:39Z

common/select.h


-    /* Add file-descriptor for select  */
-    void addFd(int fd);
+    /* Add multiple messages for select */


messages [](start = 20, length = 8)

objects #Closed

qiluo-msft · 2018-04-06T21:39:16Z

common/selectable.h

-    /* Read a message from the socket */
-    virtual void readMe() = 0;
+private:
+    int m_priority;


m_priority [](start = 8, length = 10)

Add some comment about the definition. The larger, the higher priority? #Closed

qiluo-msft · 2018-04-06T21:41:59Z

common/selectable.h

+        bool operator()(const Selectable* a, const Selectable* b) const
+        {
+            if (a->getPri() == b->getPri())
+                return a > b;


a > b [](start = 23, length = 5)

Why define the order this way? The intuitive understanding is they are equal. #Closed

Because std::set requires a functional object which defines order. By default it's std::less.
But I've defined my own order
http://en.cppreference.com/w/cpp/container/set

qiluo-msft · 2018-04-06T21:56:25Z

common/select.cpp

+    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:: [](start = 8, length = 5)

No need to add namespace #Pending

I'm used to use prefixes to avoid ambiguity when reading.
string is so common name, so std::string looks much better.
Anyway we have ~800 usage of std in swss-common
$ grep 'std::' * | wc -l
804

qiluo-msft · 2018-04-06T22:33:51Z

common/select.cpp


-    /* Checking caching from reader */
-    for (Selectable *i : m_objects)
+    while (!m_ready.empty())


while [](start = 4, length = 5)

Since you return in the block, why not use 'if'? #Closed

qiluo-msft · 2018-04-06T23:56:16Z

common/select.cpp

+        timeout = -1;
+
+    /* check if we have some data */
+    ret = select1(c, 0);


select1 [](start = 10, length = 7)

select1 is a weird name. #Closed

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

This reverts commit bac891b.

pavel-shirshov · 2018-04-11T22:24:28Z

Current master has broken unit tests. As soon as they are fixed, I'll resolve conflicts and make some requested changes.

lguohan · 2018-04-12T08:27:40Z

resolve conflict first?

pavel-shirshov · 2018-04-12T18:09:26Z

I've resolved the conflicts, but I have to check everything, until I'm sure everything is alright.

lguohan · 2018-04-12T18:16:28Z

retest this please

pavel-shirshov · 2018-04-13T00:11:02Z

All tests are done. The PR is ready for the final review

qiluo-msft · 2018-04-13T02:16:36Z

common/selectable.h

+
+            /* if the priorities are equal */
+            /* use Selectable which was selected later */
+            if (a->getLastTimeUsed() < b->getLastTimeUsed())


getLastTimeUsed [](start = 19, length = 15)

The order is dynamic, I am afraid it will break the assumption of std::set

…l object, to avoid it using by others

pavel-shirshov · 2018-04-13T22:56:05Z

Qi,

I share your concern about the Comparator using the mutable state. But in this case I don't have any other simple solution. So what I did:

Move the comparator into private section of the Select class. Only this class using this comparator.
The set object which uses the Comparator is already in the private section of the Select class.
Put updateClock() method into private section of Selectable, nobody could run this methong except friend class Select.

Also I have a comment about danger of using Selectalbe->updateLastUsedTime() on the Selectable which is inside of the set.

qiluo-msft

I have no more comments. Please check with other reviewers.

pavel-shirshov added 3 commits April 2, 2018 18:40

Refactor select and selectable. Now they support priority

63bf401

Remove fd parameter from Select.select(). This parameter was never used

cfee528

Use references as loop variables

bac891b

pavel-shirshov requested a review from lguohan April 3, 2018 20:50

pavel-shirshov self-assigned this Apr 3, 2018

pavel-shirshov added Bug Enhancement labels Apr 3, 2018

qiluo-msft reviewed Apr 3, 2018

View reviewed changes

qiluo-msft reviewed Apr 6, 2018

View reviewed changes

pavel-shirshov added 3 commits April 10, 2018 17:37

Make select fair for selectables.

fb61314

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

Revert "Use references as loop variables"

8e328aa

This reverts commit bac891b.

Revert performance changes

5b76223

This was referenced Apr 11, 2018

Use the refactored version of Selectables sonic-net/sonic-sairedis#313

Merged

Use the refactored version of Selectables sonic-net/sonic-swss#470

Merged

pavel-shirshov added 2 commits April 11, 2018 15:43

Address comments

4d3bcea

Make the comparator comment clearer

e835cac

Merge branch 'master' into pavelsh/pri

28dcdad

Use right EINTR error code for netlink function

e428d9c

Merge branch 'master' into pavelsh/pri

88dd5fa

qiluo-msft reviewed Apr 13, 2018

View reviewed changes

pavel-shirshov added 5 commits April 13, 2018 11:11

Move Selectable comparator to the Select class as a private functiona…

842b5b0

…l object, to avoid it using by others

Merge branch 'master' into pavelsh/pri

7bfc1ef

Hide m_last_used_time from all classes except Select

4d711bb

Rename some class members

21b3445

Make more consistent naming for LastUsedTime of selectable

7359660

qiluo-msft approved these changes Apr 14, 2018

View reviewed changes

lguohan approved these changes Apr 16, 2018

View reviewed changes

lguohan merged commit 1ef337a into sonic-net:master Apr 16, 2018

Conversation

pavel-shirshov commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiluo-msft Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Apr 3, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Apr 3, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

qiluo-msft commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavel-shirshov commented Apr 3, 2018

Uh oh!

qiluo-msft Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov commented Apr 11, 2018

Uh oh!

lguohan commented Apr 12, 2018

Uh oh!

pavel-shirshov commented Apr 12, 2018

Uh oh!

lguohan commented Apr 12, 2018

Uh oh!

pavel-shirshov commented Apr 13, 2018

Uh oh!

qiluo-msft Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov commented Apr 3, 2018 •

edited

Loading

qiluo-msft Apr 3, 2018 •

edited

Loading

pavel-shirshov Apr 3, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Apr 3, 2018 •

edited

Loading

pavel-shirshov Apr 3, 2018 •

edited by qiluo-msft

Loading

qiluo-msft commented Apr 3, 2018 •

edited

Loading

qiluo-msft Apr 6, 2018 •

edited

Loading

qiluo-msft Apr 6, 2018 •

edited

Loading

qiluo-msft Apr 6, 2018 •

edited

Loading

qiluo-msft Apr 6, 2018 •

edited

Loading

qiluo-msft Apr 6, 2018 •

edited

Loading

qiluo-msft Apr 6, 2018 •

edited

Loading

pavel-shirshov commented Apr 13, 2018 •

edited

Loading