C API: improve performance by not memcpy-ing string DB values across …

…ffi boundary (#935) This introduces a couple of new FFI types, representing owned C++ strings and arrays. We use those owned strings for database values, so they do not need to be memcpy'd across ffi boundaries. This is in response to Riff's concerns that database values which hamgrd will be reading may be large and expensive to memcpy. Partner pr to sonic-net/sonic-dash-ha#11 Co-authored-by: erer1243 <[email protected]>
sonic-net · Nov 14, 2024 · 378e828 · 378e828
1 parent 11e4055
commit 378e828
Show file tree

Hide file tree

Showing 13 changed files with 251 additions and 143 deletions.
diff --git a/common/c-api/consumerstatetable.cpp b/common/c-api/consumerstatetable.cpp
@@ -1,3 +1,4 @@
+#include <boost/numeric/conversion/cast.hpp>
 #include <cstdlib>
 #include <cstring>
 #include <deque>
@@ -10,6 +11,7 @@
 
 using namespace swss;
 using namespace std;
+using boost::numeric_cast;
 
 SWSSConsumerStateTable SWSSConsumerStateTable_new(SWSSDBConnector db, const char *tableName,
                                                   const int32_t *p_popBatchSize,

diff --git a/common/c-api/dbconnector.cpp b/common/c-api/dbconnector.cpp
@@ -1,5 +1,6 @@
 #include <cstring>
 #include <string>
+#include <utility>
 
 #include "../dbconnector.h"
 #include "dbconnector.h"
@@ -37,14 +38,14 @@ int8_t SWSSDBConnector_del(SWSSDBConnector db, const char *key) {
     SWSSTry(return ((DBConnector *)db)->del(string(key)) ? 1 : 0);
 }
 
-void SWSSDBConnector_set(SWSSDBConnector db, const char *key, const char *value) {
-    SWSSTry(((DBConnector *)db)->set(string(key), string(value)));
+void SWSSDBConnector_set(SWSSDBConnector db, const char *key, SWSSStrRef value) {
+    SWSSTry(((DBConnector *)db)->set(string(key), takeStrRef(value)));
 }
 
-char *SWSSDBConnector_get(SWSSDBConnector db, const char *key) {
+SWSSString SWSSDBConnector_get(SWSSDBConnector db, const char *key) {
     SWSSTry({
         shared_ptr<string> s = ((DBConnector *)db)->get(string(key));
-        return s ? strdup(s->c_str()) : nullptr;
+        return s ? makeString(move(*s)) : nullptr;
     });
 }
 
@@ -57,21 +58,29 @@ int8_t SWSSDBConnector_hdel(SWSSDBConnector db, const char *key, const char *fie
 }
 
 void SWSSDBConnector_hset(SWSSDBConnector db, const char *key, const char *field,
-                          const char *value) {
-    SWSSTry(((DBConnector *)db)->hset(string(key), string(field), string(value)));
+                          SWSSStrRef value) {
+    SWSSTry(((DBConnector *)db)->hset(string(key), string(field), takeStrRef(value)));
 }
 
-char *SWSSDBConnector_hget(SWSSDBConnector db, const char *key, const char *field) {
+SWSSString SWSSDBConnector_hget(SWSSDBConnector db, const char *key, const char *field) {
     SWSSTry({
         shared_ptr<string> s = ((DBConnector *)db)->hget(string(key), string(field));
-        return s ? strdup(s->c_str()) : nullptr;
+        return s ? makeString(move(*s)) : nullptr;
     });
 }
 
 SWSSFieldValueArray SWSSDBConnector_hgetall(SWSSDBConnector db, const char *key) {
     SWSSTry({
-        auto map = ((DBConnector *)db)->hgetall(key);
-        return makeFieldValueArray(map);
+        auto map = ((DBConnector *)db)->hgetall(string(key));
+
+        // We can't move keys out of the map, we have to copy them, until C++17 map::extract so we
+        // copy them here into a vector to avoid needing an overload on makeFieldValueArray
+        vector<pair<string, string>> pairs;
+        pairs.reserve(map.size());
+        for (auto &pair : map)
+            pairs.push_back(make_pair(pair.first, move(pair.second)));
+
+        return makeFieldValueArray(std::move(pairs));
     });
 }
 

diff --git a/common/c-api/dbconnector.h b/common/c-api/dbconnector.h
@@ -29,71 +29,34 @@ void SWSSDBConnector_free(SWSSDBConnector db);
 // Returns 0 when key doesn't exist, 1 when key was deleted
 int8_t SWSSDBConnector_del(SWSSDBConnector db, const char *key);
 
-void SWSSDBConnector_set(SWSSDBConnector db, const char *key, const char *value);
+void SWSSDBConnector_set(SWSSDBConnector db, const char *key, SWSSStrRef value);
 
-// Returns NULL if key doesn't exist.
-// Result must be freed using free()
-char *SWSSDBConnector_get(SWSSDBConnector db, const char *key);
+// Returns NULL if key doesn't exist
+// Result must be freed using SWSSString_free()
+SWSSString SWSSDBConnector_get(SWSSDBConnector db, const char *key);
 
 // Returns 0 for false, 1 for true
 int8_t SWSSDBConnector_exists(SWSSDBConnector db, const char *key);
 
 // Returns 0 when key or field doesn't exist, 1 when field was deleted
 int8_t SWSSDBConnector_hdel(SWSSDBConnector db, const char *key, const char *field);
 
-void SWSSDBConnector_hset(SWSSDBConnector db, const char *key, const char *field,
-                          const char *value);
+void SWSSDBConnector_hset(SWSSDBConnector db, const char *key, const char *field, SWSSStrRef value);
 
-// Returns NULL if key or field doesn't exist.
-// Result must be freed using free()
-char *SWSSDBConnector_hget(SWSSDBConnector db, const char *key, const char *field);
+// Returns NULL if key or field doesn't exist
+// Result must be freed using SWSSString_free()
+SWSSString SWSSDBConnector_hget(SWSSDBConnector db, const char *key, const char *field);
 
-// Returns an empty map when the key doesn't exist.
-// Result array and all of its elements must be freed using free()
+// Returns an empty map when the key doesn't exist
+// Result array and all of its elements must be freed using appropriate free functions
 SWSSFieldValueArray SWSSDBConnector_hgetall(SWSSDBConnector db, const char *key);
 
 // Returns 0 when key or field doesn't exist, 1 when field exists
 int8_t SWSSDBConnector_hexists(SWSSDBConnector db, const char *key, const char *field);
 
-// std::vector<std::string> keys(const std::string &key);
-
-// std::pair<int, std::vector<std::string>> scan(int cursor = 0, const char
-// *match = "", uint32_t count = 10);
-
-// template<typename InputIterator>
-// void hmset(const std::string &key, InputIterator start, InputIterator stop);
-
-// void hmset(const std::unordered_map<std::string,
-// std::vector<std::pair<std::string, std::string>>>& multiHash);
-
-// std::shared_ptr<std::string> get(const std::string &key);
-
-// std::shared_ptr<std::string> hget(const std::string &key, const std::string
-// &field);
-
-// int64_t incr(const std::string &key);
-
-// int64_t decr(const std::string &key);
-
-// int64_t rpush(const std::string &list, const std::string &item);
-
-// std::shared_ptr<std::string> blpop(const std::string &list, int timeout);
-
-// void subscribe(const std::string &pattern);
-
-// void psubscribe(const std::string &pattern);
-
-// void punsubscribe(const std::string &pattern);
-
-// int64_t publish(const std::string &channel, const std::string &message);
-
-// void config_set(const std::string &key, const std::string &value);
-
 // Returns 1 on success, 0 on failure
 int8_t SWSSDBConnector_flushdb(SWSSDBConnector db);
 
-// std::map<std::string, std::map<std::string, std::map<std::string,
-// std::string>>> getall();
 #ifdef __cplusplus
 }
 #endif

diff --git a/common/c-api/producerstatetable.cpp b/common/c-api/producerstatetable.cpp
@@ -25,7 +25,7 @@ void SWSSProducerStateTable_setBuffered(SWSSProducerStateTable tbl, uint8_t buff
 
 void SWSSProducerStateTable_set(SWSSProducerStateTable tbl, const char *key,
                                 SWSSFieldValueArray values) {
-    SWSSTry(((ProducerStateTable *)tbl)->set(string(key), takeFieldValueArray(values)));
+    SWSSTry(((ProducerStateTable *)tbl)->set(string(key), takeFieldValueArray(std::move(values))));
 }
 
 void SWSSProducerStateTable_del(SWSSProducerStateTable tbl, const char *key) {

diff --git a/common/c-api/producerstatetable.h b/common/c-api/producerstatetable.h
@@ -22,11 +22,6 @@ void SWSSProducerStateTable_set(SWSSProducerStateTable tbl, const char *key, SWS
 
 void SWSSProducerStateTable_del(SWSSProducerStateTable tbl, const char *key);
 
-// Batched version of set() and del().
-// virtual void set(const std::vector<KeyOpFieldsValuesTuple>& values);
-
-// virtual void del(const std::vector<std::string>& keys);
-
 void SWSSProducerStateTable_flush(SWSSProducerStateTable tbl);
 
 int64_t SWSSProducerStateTable_count(SWSSProducerStateTable tbl);

diff --git a/common/c-api/subscriberstatetable.cpp b/common/c-api/subscriberstatetable.cpp
@@ -1,3 +1,4 @@
+#include <boost/numeric/conversion/cast.hpp>
 #include <cstdlib>
 #include <cstring>
 #include <deque>
@@ -11,6 +12,7 @@
 
 using namespace swss;
 using namespace std;
+using boost::numeric_cast;
 
 SWSSSubscriberStateTable SWSSSubscriberStateTable_new(SWSSDBConnector db, const char *tableName,
                                                       const int32_t *p_popBatchSize,

diff --git a/common/c-api/util.cpp b/common/c-api/util.cpp
@@ -1,3 +1,33 @@
 #include "util.h"
 
+using namespace swss;
+
 bool swss::cApiTestingDisableAbort = false;
+
+SWSSString SWSSString_new(const char *data, uint64_t length) {
+    SWSSTry(return makeString(std::string(data, numeric_cast<std::string::size_type>(length))));
+}
+
+SWSSString SWSSString_new_c_str(const char *c_str) {
+    SWSSTry(return makeString(std::string(c_str)));
+}
+
+const char *SWSSStrRef_c_str(SWSSStrRef s) {
+    SWSSTry(return ((std::string *)s)->c_str());
+}
+
+uint64_t SWSSStrRef_length(SWSSStrRef s) {
+    SWSSTry(return ((std::string *)s)->length());
+}
+
+void SWSSString_free(SWSSString s) {
+    SWSSTry(delete (std::string *)s);
+}
+
+void SWSSFieldValueArray_free(SWSSFieldValueArray arr) {
+    SWSSTry(delete[] arr.data);
+}
+
+void SWSSKeyOpFieldValuesArray_free(SWSSKeyOpFieldValuesArray kfvs) {
+    SWSSTry(delete[] kfvs.data);
+}