Improve sub frame decode performance #777
Merged
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
|
PR missing one of the required labels: {'breaking-change', 'new feature', 'bug', 'dependencies', 'enhancement', 'documentation', 'internal'} |
sashacmc
reviewed
Nov 6, 2024
include/zenoh-pico/config.h
Outdated
| @@ -45,6 +45,7 @@ | |||
| #define Z_FEATURE_LOCAL_SUBSCRIBER 0 | |||
| #define Z_FEATURE_PUBLISHER_SESSION_CHECK 1 | |||
| #define Z_FEATURE_BATCHING 1 | |||
| #define Z_FEATURE_MEMOIZATION 0 | |||
There was a problem hiding this comment.
Suggested change
| #define Z_FEATURE_MEMOIZATION 0 | |
| #define Z_FEATURE_MEMORIZATION 0 |
There was a problem hiding this comment.
The technical term is Memoization though, https://en.wikipedia.org/wiki/Memoization. But yeah that might not be transparent to the user, in that case we might as well go to something like Z_FEATURE_RX_CACHE
src/protocol/codec/transport.c
Outdated
| @@ -28,6 +28,10 @@ | |||
| #include "zenoh-pico/utils/logging.h" | |||
| #include "zenoh-pico/utils/result.h" | |||
|
|
|||
| #define _Z_FRAME_VEC_BASE_SIZE 8 // Abritrary small value | |||
| #define _Z_FRAME_VEC_SIZE_FROM_ZBUF_LEN(len) \ | |||
| _Z_FRAME_VEC_BASE_SIZE + (len) / Z_CONFIG_FRAME_AVG_MSG_SIZE // Approximate number of messages in frame | |||
There was a problem hiding this comment.
Suggested change
| _Z_FRAME_VEC_BASE_SIZE + (len) / Z_CONFIG_FRAME_AVG_MSG_SIZE // Approximate number of messages in frame | |
| (_Z_FRAME_VEC_BASE_SIZE + (len) / Z_CONFIG_FRAME_AVG_MSG_SIZE) // Approximate number of messages in frame |
To avoid potential incorrect usage in expressions
sashacmc
approved these changes
Nov 8, 2024
jean-roland
added a commit
to jean-roland/zenoh-pico
that referenced
this pull request
Nov 13, 2024
* feat: alias string decode * fix: alias keyexpr only on rx buffer * feat: add svec release function * feat: remove duplicate message clear * feat: remove unused subscription function * fix: missing const qualifier * feat: remove encoding null value * feat: improve sample create perf * feat: init string in encoding move * feat: replace rc sub list with sub info vec * feat: check before encoding clear * feat: avoid rechecking msg reliability * feat: add sub memoization * fix: alias cache if suffix * feat: move z_bytes instead of copy * feat: add svec move function * feat: add memoization config token * feat: add vec alias function * feat: switch sub cache to session level * feat: add svec expand function * fix: missing function args * fix: don't use _z_noop_move in svec * feat: svec ops return error instead of bool * feat: svec functions * feat: add use elem f in svec * feat: switch frame nmsg to svec * feat: add config value for initial frame size evaluation * feat: remove superfluous init * fix: rename cache token * fix: remove superfluous initialization
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.
The introduction of batching revealed lot of room to improve performance in frame decoding. The will be split in a subscriber focused PR (this one) and a query focused one (TBD).
One of the costliest operation in subscriber triggering was the construction of the key-expression and the callback lookup. Memoizing the results improved significantly performance. Although the current implementation is a bit trivial as it only holds one entry. Probably worth adding a true lru cache implementation in next PR (open to suggestion).
With frames containing 4000+ frames, the use of reference vectors were causing a lot of memory fragmentation and inefficiencies. The use of svec is much better but require to minimize reallocation, thus a configurable
Z_CONFIG_FRAME_AVG_MSG_SIZEhas been introduced to strike a balance between memory consumption and time spent on reallocation.Svec/elem were a bit improved as well (added the elem_move, ability to choose default svec move/copy behaviour with boolean, return result instead of bool...)
There were a few cases of superfluous or redundant type init and clear. Specifically on the network messages and they were removed.