Repository navigation
Deserialize subscription payloads in place - #1074
Conversation
take_one_message() allocated a buffer sized at four times the payload plus 64 KiB, copied the payload into it, and then built the FastBuffer over payload_data.size(). The extra capacity was never addressable by the deserializer: FastCDR is bounded by the size passed to FastBuffer and raises NotEnoughMemoryException on overrun, so the margin could not be used. Construct the FastBuffer over the received payload instead, as the released branches do, and drop the allocation and the full-payload copy. Payload owns its bytes and msg_data stays alive for the whole deserialization, so the slice remains valid for as long as the message is processed. The allocation and the copy were both introduced together in ros2#930. Removing them restores the behaviour of the released branches while keeping the endpoint-aware deserialization path that the same commit added. Signed-off-by: Quchaosheng <quchaosheng000406@163.com>
|
@Quchaosheng thanks for the PR, could you update your PR description to follow the template? |
|
@Yadunund thanks.Updated the description to follow the template, thanks for the pointer. |
|
@Quchaosheng sorry for the delay. Will take a look this week. |
Yadunund
left a comment
Yadunund
left a comment
There was a problem hiding this comment.
LGTM. Let's also backport to lyrical.
@YuanYuYuan could you do a final pass review as well?
Yep. As I stated in the issue, this patch does solve it. Thanks again @Quchaosheng ! |
|
Repos file: https://gist.githubusercontent.com/Yadunund/62882e88df23c7375068cc6bcad1691d/raw/488fe03bb732127a28b3dd372197db3d6615b1a4/ros2.repos
|
|
https://github.com/Mergifyio backport lyrical |
✅ Backports have been createdDetails
|
Description
take_one_message()allocated a buffer ofpayload_data.size() * 4 + 65536bytes and copied the payload into it before deserializing. The kilted, jazzy and humble branches already deserialize straight from the payload withFastBuffer, so this brings rolling in line with them and drops the per-message allocation and copy from the subscription hot path.This is the first option in the issue's suggested handling, and the one the released branches use. Right-sizing by itself is not: as the issue measured, the oversized block is what keeps
M_MMAP_THRESHOLDlifted, and with itM_TRIM_THRESHOLD, so removing the oversized block while still allocating costs 30-40% at 1 MiB because glibc returns the heap withbrkbetween messages and the next one re-faults it. Removing the allocation takes the allocator out of the path instead of trading one policy for another.Payloadowns its bytes andmsg_datastays alive for the whole deserialization, so the slice stays valid. FastCDR is bounded by the size passed toFastBufferand raisesNotEnoughMemoryExceptionon overrun, so the 4x margin was never addressable by the deserializer.Related to #1060
Is this user-facing behavior change?
No. Same deserialization result, minus a per-message allocation and copy.
Did you use Generative AI?
Yes, I used an LLM to review the change and to help write this description. The code change is mine.