refactor(Middleware): return payload by const reference#503
Open
dcavalei wants to merge 1 commit into
Open
Conversation
When sizeof(T) is large, returning by value risks blowing the stack depending on system configuration. readPayload<T>() now returns a T const& directly into the middleware-managed buffer. Callers must consume the reference before the message is deallocated, as the middleware reclaims the buffer immediately after dispatch. - readPayload<T>() now returns T const& instead of T, eliminating unnecessary copies when reading from message payloads - FutureDispatcher callbacks now receive etl::reference_wrapper<ArgumentType const> instead of T by value - Replace allocateTrivialType/allocateNonTrivialType with emplaceExternalPayload (uses placement new for proper construction) and allocAndCopyBytesToExternalPayload (for byte span copies) - Drop AllocationPolicy-based serialization path; T must now be copy-constructible (enforced via static_assert) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors middleware payload handling to avoid large by-value copies by returning payloads by const& and plumbing that change through FutureDispatcher callbacks via etl::reference_wrapper<const T>. It also simplifies payload allocation APIs by removing the prior AllocationPolicy serialization path in favor of constructing/copying directly into middleware-managed buffers.
Changes:
MessagePayloadBuilder::readPayload<T>()now returnsT const&, andFutureDispatcherpropagates results asetl::expected<etl::reference_wrapper<T const>, Future::State>.- Replaces prior trivial/non-trivial allocation split with external emplace/copy helpers and a dedicated byte-span allocation path.
- Updates unit tests/mocks and small API typedefs (
ProxyAttributeBase) to match the new callback/result types.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/bsw/middleware/include/middleware/core/MessagePayloadBuilder.h | Switches payload reads to const&, refactors allocation API, adds external placement-new path. |
| libs/bsw/middleware/src/middleware/core/MessagePayloadBuilder.cpp | Implements new byte-span allocation and keeps external deallocation logic. |
| libs/bsw/middleware/include/middleware/core/FutureDispatcher.h | Changes callback result type to expected<reference_wrapper<const T>, …> and wires cref(readPayload()). |
| libs/bsw/middleware/include/middleware/core/ProxyAttributeBase.h | Reuses FutureDispatcher’s Result/Callback typedefs to match the new signature. |
| libs/bsw/middleware/include/middleware/core/ProxyAttribute.h | Aligns getter typedefs with ProxyAttributeBase’s new GetterResult. |
| libs/bsw/middleware/test/src/core/ReferenceApp.h | Updates mock callback type to use reference_wrapper<const T> in the expected. |
| libs/bsw/middleware/test/src/core/FutureDispatcherTest.cpp | Updates test callbacks/expectations to the new dispatcher callback signature. |
| libs/bsw/middleware/test/src/core/MessagePayloadBuilderTest.cpp | Reworks allocation tests to cover internal/external/object/span flows under the new APIs. |
| libs/bsw/middleware/include/middleware/core/Message.h | Minor comment wording fix (“targeted”). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+57
to
+59
| static_assert(etl::is_copy_constructible_v<T>, "T must be copy-constructible"); | ||
| static_assert(!etl::is_span_v<T>, "Use the span overload to allocate bytes"); | ||
|
|
Comment on lines
244
to
246
| core::HRESULT ret = MessagePayloadBuilder::getInstance().allocate(obj, msg); | ||
| auto const storedObj = MessagePayloadBuilder::getInstance().readPayload<decltype(buffer_)>(msg); | ||
| auto const storedObj = MessagePayloadBuilder::getInstance().readPayload<TypeParam>(msg); | ||
|
|
Comment on lines
273
to
275
| core::HRESULT ret = MessagePayloadBuilder::getInstance().allocate(obj, msg); | ||
| auto const storedObj = MessagePayloadBuilder::getInstance().readPayload<decltype(buffer_)>(msg); | ||
| auto const storedObj = MessagePayloadBuilder::getInstance().readPayload<TypeParam>(msg); | ||
|
|
Comment on lines
+305
to
+309
| EXPECT_TRUE(this->getAllocatorImpl().isAnyPoolFull()); | ||
| auto const storedObj = MessagePayloadBuilder::getInstance().readPayload<TypeParam>(msg); | ||
| EXPECT_EQ(obj, storedObj); | ||
| MessagePayloadBuilder::deallocate(msg); | ||
| } |
This file contains hidden or 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
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.
When sizeof(T) is large, returning by value risks blowing the stack depending on system configuration. readPayload() now returns a T const& directly into the middleware-managed buffer. Callers must consume the reference before the message is deallocated, as the middleware reclaims the buffer immediately after dispatch.
readPayload() now returns T const& instead of T, eliminating unnecessary copies when reading from message payloads
FutureDispatcher callbacks now receive etl::reference_wrapper instead of T by value
Replace allocateTrivialType/allocateNonTrivialType with emplaceExternalPayload (uses placement new for proper construction) and allocAndCopyBytesToExternalPayload (for byte span copies)
Drop AllocationPolicy-based serialization path; T must now be copy-constructible (enforced via static_assert)