Repository navigation
Deserialize subscription payloads in place (backport #1074) - #1085
Open
mergify[bot] wants to merge 1 commit into
Open
mergify[bot] wants to merge 1 commit into
mergify[bot] wants to merge 1 commit into
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 #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> (cherry picked from commit 678931f)
Contributor
|
Pulls: #1085
|
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.
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.
This is an automatic backport of pull request #1074 done by [Mergify](https://mergify.com).