[NotificationProducer] add pipeline support #708

stepanblyschak · 2022-11-11T13:37:57Z

Added a pipeline support for NotificationProducer. NotificationProducer created with RedisPipeline will buffer published notifications untill a flush is called on RedisPipeline or RedisPipeline's command buffer is filled.

The motivation for this change is to optimize redis usage when using ResponsePublisher from sonic-swss for a lot of entries.

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>

… into not_ch_pipeline

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>

stepanblyschak · 2022-12-06T09:56:39Z

/azpw run

mssonicbld · 2022-12-06T09:56:41Z

/AzurePipelines run

azure-pipelines · 2022-12-06T09:56:52Z

Azure Pipelines successfully started running 1 pipeline(s).

stepanblyschak · 2022-12-06T10:47:27Z

/azpw run

mssonicbld · 2022-12-06T10:47:29Z

/AzurePipelines run

azure-pipelines · 2022-12-06T10:47:39Z

Azure Pipelines successfully started running 1 pipeline(s).

stepanblyschak · 2022-12-06T20:14:01Z

/azpw run

mssonicbld · 2022-12-06T20:14:03Z

/AzurePipelines run

azure-pipelines · 2022-12-06T20:14:16Z

Azure Pipelines successfully started running 1 pipeline(s).

stepanblyschak · 2022-12-07T08:31:15Z

/azpw run

mssonicbld · 2022-12-07T08:31:17Z

/AzurePipelines run

azure-pipelines · 2022-12-07T08:31:32Z

Azure Pipelines successfully started running 1 pipeline(s).

stepanblyschak · 2022-12-07T13:49:12Z

Fix for sairedis tests - sonic-net/sonic-sairedis#1169

stepanblyschak · 2022-12-08T08:30:12Z

/azpw run

mssonicbld · 2022-12-08T08:30:14Z

/AzurePipelines run

azure-pipelines · 2022-12-08T08:30:28Z

Azure Pipelines successfully started running 1 pipeline(s).

liat-grozovik · 2022-12-15T07:50:33Z

@marian-pritsak @prsunny could you please help to review?

qiluo-msft · 2023-01-14T00:03:25Z

BUILD

@@ -10,6 +10,7 @@ cc_library(
        "common/*.hpp",
    ]),
    copts = [
+        "-std=c++14",


c++14

Other places are using c++11. Is it a concern?
Like: pyext/py3/Makefile.am
And sairedis using c++11 depends on swss-common. #Closed

@qiluo-msft I don't see a problem. Everything builds successfully. I don't think other projects need to stay with c++11. c++14 has been around for a long time and adopted by compiler. As long as the headers are c++11 compatible and ABI does not change - everything works.

Is it a blocking issue?
If the change is really needed, I prefer changing the c++NN option in a consistent way, and change dependent libraries/applications first (swss->sairedis->swss-common). And change pyext/py[23]/Makefiles.am at the same time.

sairedis is c++14 - https://github.com/sonic-net/sonic-sairedis/blob/master/configure.ac#L121
swss is c++14 - https://github.com/sonic-net/sonic-swss/blob/master/configure.ac#L57
I believe not every app using swss-common is now c++14, not to mention app extensions.
My point is that swss-common may use c++-14 in .cpp but must have c++-11 compatible headers for those apps that might not be able to migrate yet. Python swss-common bindings compile a wrapper cpp based of the headers, that's why py[23]/Makefile.am aren't changed and stay c++-11.

qiluo-msft · 2023-01-14T00:17:05Z

common/notificationproducer.cpp

 swss::NotificationProducer::NotificationProducer(swss::DBConnector *db, const std::string &channel):
-    m_db(db), m_channel(channel)
+    m_ownedpipe(std::make_unique<swss::RedisPipeline>(db, NON_BUFFERED_COMMAND_BUFFER_SIZE)), m_pipe(m_ownedpipe.get()), m_channel(channel), m_buffered(false)


m_ownedpipe

You are assuming m_pipe initialized after m_ownedpipe initialized. The sequence is not well known knowledge. Suggest you move m_pipe to function body for simplicity purpose. #Closed

@qiluo-msft The order of member initialization is defined by the standard and is well known. Member variables are initialized in the order they are defined in the class definition. If by any chance members are reordered in the definition any modern compiler will raise a warning (-Wreorder in gcc) and many of them should be treated as errors. Since we use -Wall + -Werror it is going to be catched.

qiluo-msft · 2023-01-14T00:19:43Z

common/notificationproducer.cpp

+
+    if (m_buffered)
+    {
+        m_pipe->push(command, REDIS_REPLY_INTEGER);


m_pipe

You may use m_ownedpipe directly. Are you defining two member variables for better performance? If yes, I prefer one variable for simplicity.
Then you can change var name to m_pipe. #Closed

@qiluo-msft NotificationProducer has two invariants 1) Created from DBConnector and having it's own RedisPipeline in m_ownedpipe smart pointer and m_pipe pointing to it 2) NotificationProducer created from RedisPipeline which is not owned - m_ownedpipe is nullptr, m_pipe is a non-owning raw pointer to RedisPipeline object.
So, m_pipe is always pointing to RedisPipeline that needs to be used regardless of how NotificationProducer has been created.

I see the same pattern beeing used for Table class (https://github.com/sonic-net/sonic-swss-common/blob/master/common/table.cpp#L38) and it was a motivation to follow the same desing.

However, the table class uses raw pointer and manual memory allocation/deallocation + a flag to indicate wether we own the RedisPipeline object. So I came up with a bit simpler approach.

qiluo-msft · 2023-01-14T00:26:48Z

common/notificationproducer.cpp

+
+        // if operating in buffered mode return -1 as we can't know the number
+        // of client's that have read the message immediately
+        return -1;


-1

Is it better to use 1? Client will think it send it successsfully. #Closed

@qiluo-msft As stated by the comment on send() method the return code does not indicate the success or failure but the number of clients that read the message. In buffered mode we can't imidatelly tell how many clients read the message, so returning 1 might fool the user of this API that his consumer has already read message.

qiluo-msft · 2023-01-14T00:58:03Z

@bocon13 Could you also help review?

stepanblyschak added 5 commits October 17, 2022 09:29

[notifications] use redis pipeline

e6fa8cc

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>

remove schema changes

f00881e

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>

style fixes

f455362

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>

Merge branch 'master' of https://github.com/sonic-net/sonic-swss-common…

c39b841

… into not_ch_pipeline

make a define for buffer size

fa1ca24

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>

This was referenced Nov 11, 2022

[ResponsePublisher] add pipeline support sonic-net/sonic-swss#2511

Merged

FIB Suppress Announcements of Routes Not Installed in HW sonic-net/SONiC#1103

Merged

prsunny requested a review from qiluo-msft November 11, 2022 20:23

liat-grozovik requested review from marian-pritsak and prsunny November 28, 2022 13:54

use 14 standard in BUILD

102acb6

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>

qiluo-msft reviewed Jan 14, 2023

View reviewed changes

qiluo-msft requested a review from kcudnik January 14, 2023 00:22

qiluo-msft reviewed Jan 14, 2023

View reviewed changes

qiluo-msft approved these changes Jan 27, 2023

View reviewed changes

qiluo-msft merged commit 6b6842a into sonic-net:master Jan 30, 2023

dprital mentioned this pull request Jan 30, 2023

[submodule] Advance sonic-swss-common pointer sonic-net/sonic-buildimage#13556

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NotificationProducer] add pipeline support #708

[NotificationProducer] add pipeline support #708

stepanblyschak commented Nov 11, 2022

stepanblyschak commented Dec 6, 2022

mssonicbld commented Dec 6, 2022

azure-pipelines bot commented Dec 6, 2022

stepanblyschak commented Dec 6, 2022

mssonicbld commented Dec 6, 2022

azure-pipelines bot commented Dec 6, 2022

stepanblyschak commented Dec 6, 2022

mssonicbld commented Dec 6, 2022

azure-pipelines bot commented Dec 6, 2022

stepanblyschak commented Dec 7, 2022

mssonicbld commented Dec 7, 2022

azure-pipelines bot commented Dec 7, 2022

stepanblyschak commented Dec 7, 2022

stepanblyschak commented Dec 8, 2022

mssonicbld commented Dec 8, 2022

azure-pipelines bot commented Dec 8, 2022

liat-grozovik commented Dec 15, 2022

qiluo-msft Jan 14, 2023 •

edited

Loading

stepanblyschak Jan 14, 2023

qiluo-msft Jan 23, 2023

stepanblyschak Jan 24, 2023

qiluo-msft Jan 14, 2023 •

edited

Loading

stepanblyschak Jan 14, 2023 •

edited

Loading

qiluo-msft Jan 14, 2023 •

edited

Loading

stepanblyschak Jan 14, 2023

qiluo-msft Jan 14, 2023 •

edited

Loading

stepanblyschak Jan 14, 2023

qiluo-msft commented Jan 14, 2023

[NotificationProducer] add pipeline support #708

[NotificationProducer] add pipeline support #708

Conversation

stepanblyschak commented Nov 11, 2022

stepanblyschak commented Dec 6, 2022

mssonicbld commented Dec 6, 2022

azure-pipelines bot commented Dec 6, 2022

stepanblyschak commented Dec 6, 2022

mssonicbld commented Dec 6, 2022

azure-pipelines bot commented Dec 6, 2022

stepanblyschak commented Dec 6, 2022

mssonicbld commented Dec 6, 2022

azure-pipelines bot commented Dec 6, 2022

stepanblyschak commented Dec 7, 2022

mssonicbld commented Dec 7, 2022

azure-pipelines bot commented Dec 7, 2022

stepanblyschak commented Dec 7, 2022

stepanblyschak commented Dec 8, 2022

mssonicbld commented Dec 8, 2022

azure-pipelines bot commented Dec 8, 2022

liat-grozovik commented Dec 15, 2022

qiluo-msft Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

stepanblyschak Jan 14, 2023

Choose a reason for hiding this comment

qiluo-msft Jan 23, 2023

Choose a reason for hiding this comment

stepanblyschak Jan 24, 2023

Choose a reason for hiding this comment

qiluo-msft Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

stepanblyschak Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

stepanblyschak Jan 14, 2023

Choose a reason for hiding this comment

qiluo-msft Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

stepanblyschak Jan 14, 2023

Choose a reason for hiding this comment

qiluo-msft commented Jan 14, 2023

qiluo-msft Jan 14, 2023 •

edited

Loading

qiluo-msft Jan 14, 2023 •

edited

Loading

stepanblyschak Jan 14, 2023 •

edited

Loading

qiluo-msft Jan 14, 2023 •

edited

Loading

qiluo-msft Jan 14, 2023 •

edited

Loading