[PINS] Add ConsumerNotifier for PINS response path #547

donNewtonAlpha · 2021-10-27T22:52:47Z

No description provided.

qiluo-msft · 2021-11-08T17:14:34Z

Please change the PR title to more descriptive. People in the swss-common context do not know Pr3

common/dbconnector.cpp

.bazelrc

qiluo-msft · 2021-11-08T17:28:11Z

common/json.hpp

@@ -5394,7 +5394,7 @@ class basic_json
                {
                    assert(lhs.m_value.array != nullptr);
                    assert(rhs.m_value.array != nullptr);
-                    return *lhs.m_value.array < *rhs.m_value.array;
+                    return (*lhs.m_value.array) < *rhs.m_value.array;


(

Is it a necessary change?

This is a fix from upstream: nlohmann/json#590

Addressed in #545

qiluo-msft · 2021-11-08T17:29:16Z

Please give a link to HLD for this change.

common/schema.h

kcudnik · 2021-11-08T17:45:37Z

this seems to me to be too big change like for a single PR

bocon13 · 2021-11-08T20:09:19Z

BUILD

@@ -0,0 +1,31 @@
+package(default_visibility = ["//visibility:public"])


Integrate Bazel build into PR checker (update the build pipeline) to help catch errors for breaking this in the future

Move to separate PR

mint570 · 2021-11-08T20:42:00Z

@qiluo-msft
About the performance impact we discussed in the meeting, this is only used in P4RT code currently. For existing SONiC codes, they are unchanged (They use the real class instead of the interface. So there shouldn't be performance impact.)

Also, the interface won't change the existing swss-common unit test. They still need to test with the Redis instance. The interface mocking is only helpful for testing codes that use the swss-common libraries. (Such as P4RT, so that P4RT does not need a Redis instance when doing unit test.)

bocon13 · 2021-11-09T04:20:39Z

In modern C++, there is no performance penalty for virtual functions when the compiler can guarantee the runtime type. In our case, the implementation class can be inferred by the compiler, resulting in no additional instructions added by virtual.

The compiler option involved here is: -fdevirtualize which is automatically enabled at -O2.
https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html

The function may be inlined or called directly without a vtable lookup.

Here's a relevant blog post on the topic of virtual functions:
https://devblogs.microsoft.com/cppblog/the-performance-benefits-of-final-classes/

Anyway, I ran an experiment with and without virtual functions, and my implementation class (DBConnector) was NOT final. The result is effectively identical assembly code.

With virtual functions:

        call    std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string<std::allocator<char> >(char const*, std::allocator<char> const&)
        mov     rsi, rsp
        mov     rdi, r12
        call    DBConnector::del(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
        mov     rdi, rsp
        mov     ebp, eax

Without virtual functions:

        call    std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string<std::allocator<char> >(char const*, std::allocator<char> const&)
        mov     rsi, rsp
        mov     rdi, rbp
        call    DBConnector::del(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
        mov     rdi, rsp
        mov     r12d, eax

bocon13 · 2021-11-08T20:13:25Z

common/dbconnectorinterface.h

@@ -0,0 +1,32 @@
+#pragma once


Qi concerned about performance penalty for virtual functions. Can this be done without this interface and virtual functions?

If this is needed for new feature, then it's okay. But we shouldn't incur a performance penalty just for testing.

Update: See comment on main PR conversation regarding virtual functions: #547 (comment)

.bazelrc

bocon13 · 2021-11-09T05:31:10Z

common/json.hpp

@@ -5394,7 +5394,7 @@ class basic_json
                {
                    assert(lhs.m_value.array != nullptr);
                    assert(rhs.m_value.array != nullptr);
-                    return *lhs.m_value.array < *rhs.m_value.array;
+                    return (*lhs.m_value.array) < *rhs.m_value.array;


This is a fix from upstream: nlohmann/json#590

* Add ConsumerNotifier for response path * Introduce interfaces for DBConnector, NotificationConsumer, ConsumerNotifier, NotificationProducer, ProducerStateTable to make it easier to test these classes Submission containing materials of a third party: Copyright Google LLC; Licensed under Apache 2.0 Co-authored-by: Akarsh Gupta <akarshgupta@google.com> Co-authored-by: Ashish Singh <ashishksingh@google.com> Co-authored-by: Jay Hu <jiarenhu@google.com> Co-authored-by: Manali Kumar <manalikumar@google.com> Co-authored-by: Richard Yu <richardyu@google.com> Co-authored-by: Robert J. Halstead <rhalstea@google.com> Co-authored-by: Runming Wu <runmingwu@google.com> Co-authored-by: Srikishen Pondicherry Shanmugam <kishanps@google.com> Co-authored-by: Vivek Ramamoorthy <vivekmoorthy@google.com> Co-authored-by: Yilan Ji <yilanji@google.com> Signed-off-by: Don Newton don@opennetworking.org

donNewtonAlpha · 2021-11-11T20:07:14Z

/azp run

azure-pipelines · 2021-11-11T20:07:19Z

Commenter does not have sufficient privileges for PR 547 in repo Azure/sonic-swss-common

…3-acl-schema

qiluo-msft · 2021-11-12T19:17:38Z

common/producerstatetable.cpp

@@ -358,4 +358,9 @@ void ProducerStateTable::apply_temp_view()
    m_tempViewActive = false;
 }

+std::string ProducerStateTable::get_table_name() const


This function does not add value, can you remove?

We use this in the P4RT server application.

@rhalstea what do you think?

It's useful at various points in P4RT App to know the table name. Particularly around logging. The function was added here to make it available as part of the interface.

It's already defined in TableBase::getTableName(), and TableBase could be treated as an interface.

qiluo-msft · 2021-11-12T19:19:49Z

common/dbconnector.cpp

@@ -750,6 +751,19 @@ int64_t DBConnector::decr(const string &key)
    return r.getContext()->integer;
 }

+#ifndef SWIG
+std::unordered_map<std::string, std::string> DBConnector::hgetall(


hgetall

The template version is a general solution for all STL container types. Why we need to specialize it to unordered_map?

@rhalstea what do you think?

The templates didn't work with the virtual interfaces.

Where this is needed in the P4RT App could be updated to the Table class. We can remove this once that happens, but that change may be difficult with the MVP timelines. However, internally we do want it planned for the next push.

qiluo-msft · 2021-11-12T19:37:30Z

common/consumernotifier.cpp

+            return false;
+        }
+    }
+    notification_consumer_->pop(op, data, values);


This logic is applicable to any Selectable or Select, which contains multiple Selectable. Could you move it to their class?

qiluo-msft · 2021-11-12T19:51:06Z

common/consumernotifierinterface.h

+{
+
+// Wrapper class to use swss::NotificationConsumer.
+class ConsumerNotifierInterface


ConsumerNotifierInterface

I feel the *Interface classes just mark some functions as virtual. Can we remove all the classes?

We can, but the other issue these interface classes address is the base class constructors requiring a redis db to be active. If we remove this do you have any objections to having a protected constructor that doesn't initialize a redis connection?

qiluo-msft · 2021-11-12T19:53:38Z

On the proposal of not adding virtual, I find a discussion https://stackoverflow.com/a/45636400/2514803. Will it help keep production code intact?

In reply to: 963804275

qiluo-msft · 2021-11-12T20:00:37Z

common/producerstatetable.cpp

@@ -358,4 +358,9 @@ void ProducerStateTable::apply_temp_view()
    m_tempViewActive = false;
 }

+std::string ProducerStateTable::get_table_name() const
+{
+  return getTableName();


return

Please follow the code format convention in existing code. Such as 1. use 4-space indentation. 2. m_ prefix for non-public member variables.

bocon13 · 2021-11-12T20:08:51Z

common/dbconnector.h

@@ -199,11 +200,14 @@ class DBConnector : public RedisContext
    ReturnType hgetall(const std::string &key);

 #ifndef SWIG
+    std::unordered_map<std::string, std::string> hgetall(


Qi's suggestion: make this a standalone function in sonic-pins; first parameter is DBConnector; implementation calls the template version.

bocon13 · 2021-11-12T20:18:56Z

common/consumernotifier.h

+
+    bool WaitForNotificationAndPop(std::string &op, std::string &data, std::vector<FieldValueTuple> &values,
+                                   int64_t timeout_ms = 60000LL) override;
+


Robert's proposal

protected: ConsumerNotifier(); // used for testing

Alternative:

Provide a fake/mock DBConnector as a subclass and use that; need to create a testing only constructor there

donNewtonAlpha · 2021-11-16T16:48:46Z

/azpw run

mssonicbld · 2021-11-16T16:48:47Z

/AzurePipelines run

azure-pipelines · 2021-11-16T16:49:01Z

Azure Pipelines successfully started running 1 pipeline(s).

rhalstea · 2021-11-17T17:53:51Z

@bocon13 @qiluo-msft

I've updated sonic-pins to include the fakes/mocks so this pull request shouldn't be needed anymore.

donNewtonAlpha · 2021-11-17T18:28:46Z

Hi Robert can you put a comment on the PR and I can close

…

On Wed, Nov 17, 2021 at 12:54 PM Robert J. Halstead < ***@***.***> wrote: @bocon13 <https://github.com/bocon13> @qiluo-msft <https://github.com/qiluo-msft> I've updated sonic-pins to include the fakes/mocks so this pull request shouldn't be needed anymore. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#547 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJ4SJIVC3JODET2RWAURA3UMPT3XANCNFSM5G3O2YUQ> .

donNewtonAlpha · 2021-11-17T18:52:59Z

Closing PER @rhalstea (Google)

donNewtonAlpha force-pushed the pr3-acl-schema branch from f863211 to a90eee0 Compare October 30, 2021 02:10

qiluo-msft reviewed Nov 8, 2021

View reviewed changes

common/dbconnector.cpp Outdated Show resolved Hide resolved

qiluo-msft reviewed Nov 8, 2021

View reviewed changes

.bazelrc Outdated Show resolved Hide resolved

qiluo-msft reviewed Nov 8, 2021

View reviewed changes

common/schema.h Outdated Show resolved Hide resolved

bocon13 reviewed Nov 8, 2021

View reviewed changes

bocon13 reviewed Nov 9, 2021

View reviewed changes

bocon13 and others added 2 commits November 9, 2021 23:25

Add new tests for notification classes

9eb5bc1

bocon13 force-pushed the pr3-acl-schema branch from a90eee0 to 7bca368 Compare November 10, 2021 07:31

This was referenced Nov 10, 2021

[PINS] Add new tests for notification classes #555

Closed

[PINS] Add AppDB ACL schema values shared between the P4RT app and P4Orch #556

Merged

[PINS] Add Bazel support for external projects #557

Merged

Rename files

cdfe5b7

bocon13 mentioned this pull request Nov 10, 2021

PINS Upstream Tracking for MVP sonic-net/SONiC#841

Open

donNewtonAlpha changed the title ~~Pr3 acl schema~~ [PINS] Add ConsumerNotifier for PINS response path Nov 10, 2021

donNewtonAlpha and others added 2 commits November 11, 2021 12:20

Merge remote-tracking branch 'origin/pr5-response-path-tests' into pr…

eba0bb3

…3-acl-schema

Add libgmock to pipelines test

2e343cb

qiluo-msft reviewed Nov 12, 2021

View reviewed changes

bocon13 reviewed Nov 12, 2021

View reviewed changes

donNewtonAlpha closed this Nov 17, 2021

		@@ -0,0 +1,31 @@
		package(default_visibility = ["//visibility:public"])


		bool WaitForNotificationAndPop(std::string &op, std::string &data, std::vector<FieldValueTuple> &values,
		int64_t timeout_ms = 60000LL) override;

[PINS] Add ConsumerNotifier for PINS response path #547

[PINS] Add ConsumerNotifier for PINS response path #547

Conversation

donNewtonAlpha commented Oct 27, 2021

qiluo-msft commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Nov 8, 2021 • edited Loading

kcudnik commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mint570 commented Nov 8, 2021

bocon13 commented Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

bocon13 Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donNewtonAlpha commented Nov 11, 2021

azure-pipelines bot commented Nov 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Nov 12, 2021

Choose a reason for hiding this comment

bocon13 Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donNewtonAlpha commented Nov 16, 2021

mssonicbld commented Nov 16, 2021

azure-pipelines bot commented Nov 16, 2021

rhalstea commented Nov 17, 2021

donNewtonAlpha commented Nov 17, 2021 via email

donNewtonAlpha commented Nov 17, 2021

qiluo-msft commented Nov 8, 2021 •

edited

Loading

bocon13 commented Nov 9, 2021 •

edited

Loading

bocon13 Nov 9, 2021 •

edited

Loading

bocon13 Nov 12, 2021 •

edited

Loading