C API: improve performance by not memcpy-ing string DB values across ffi boundary #935
+251
−143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erer1243
force-pushed
the
c-interface-nocopy
branch
from
44cb87c
to
3017a8f
Compare
r12f
pushed a commit
to sonic-net/sonic-dash-ha
that referenced
this pull request
Nov 1, 2024
…FFI (#11) partner PR to sonic-net/sonic-swss-common#935 Do we want to reorganize the files further? lmk --------- Co-authored-by: erer1243 <erer1243@users.noreply.github.com>
erer1243
changed the title
liuh-80
reviewed
Nov 4, 2024
| @@ -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) { | |||
There was a problem hiding this comment.
I can't find the definition of SWSSStrRef, where is the source code?
There was a problem hiding this comment.
Near the top of util.h
sonic-swss-common/common/c-api/util.h
Lines 15 to 19 in 3017a8f
liuh-80
reviewed
Nov 4, 2024
| entry.field = strdup(pair.first.c_str()); | ||
| entry.value = strdup(pair.second.c_str()); | ||
| entry.value = makeString(std::move(pair.second)); |
There was a problem hiding this comment.
Already move inside makeString(), maybe don't need move here?
There was a problem hiding this comment.
You need the move call there to be able to call the function. C++ won't automatically make an rvalue ref
common/c-api/util.h:160:39: error: cannot bind rvalue reference of type 'std::string&&' {aka 'std::__cxx11::basic_string<char>&&'} to lvalue of type 'std::__cxx11::basic_string<char>'
160 | entry.value = makeString(pair.second);
| ~~~~~^~~~~~
common/c-api/util.h:146:51: note: initializing argument 1 of 'SWSSStringOpaque* swss::makeString(std::string&&)'
146 | static inline SWSSString makeString(std::string &&s) {
| ~~~~~~~~~~~~~~^
|
@erer1243 , can you help to resolve the conflict? |
lguohan
previously approved these changes
Nov 9, 2024
erer1243
force-pushed
the
c-interface-nocopy
branch
from
d7056cb
to
3e49737
Compare
|
@lguohan should be good now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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