Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Frame: decode payload length to return early #31

Merged
merged 4 commits into from
May 10, 2024
Merged

Conversation

678098
Copy link
Contributor

@678098 678098 commented Nov 30, 2023

A small optimization for partial Frames.
I have a feeling though that it makes things a bit more complicated, especially addressing buffer via exact offsets like buffer[3].
Might be good to add const size_t constants for these offsets.

Replacement for PR with Verified status

Signed-off-by: Evgeny Malygin <[email protected]>
@678098 678098 changed the title Update rmqamqpt_frame.cpp Frame: decode payload length to return early Nov 30, 2023
@willhoy
Copy link
Contributor

willhoy commented Jan 12, 2024

looking at this again, I'm not 100% sure we don't rely on the type, channel being populated on a partially returned frame - @adamncasey do you have any thoughts here.

Copy link
Contributor

@adamncasey adamncasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wasn't sure on this change but your question made me look in a few places which convinced me:

https://github.com/bloomberg/rmqcpp/blob/main/src/rmq/rmqio/rmqio_decoder.cpp#L55

If we return PARTIAL we never look at the (possibly) populated frame object.

https://github.com/bloomberg/rmqcpp/blob/main/src/tests/rmqio/rmqio_decoder.t.cpp#L71 this test covers this behaviour

@678098
Copy link
Contributor Author

678098 commented Jan 12, 2024

I believe I reviewed Frame usage when I was making this change, at least rmqcpp itself doesn't rely on fields in a partial Frame.

Another thing, we have a partial Frame early return on line 66 anyway:

Frame::ReturnCode Frame::decode(Frame* frame,
bsl::size_t* readBytes,
bsl::size_t* missingBytes,
const uint8_t* buffer,
bsl::size_t bufferLen)
{
if (bufferLen < frameOverhead()) {
return Frame::PARTIAL;
}

when no fields are updated. This means, that the old version of the code has a strange situation: PARTIAL means that some fields might be or might be not initialized. And we still don't have a way to know which fields are initialized in this case.
That's the reason why not initializing fields for partial Frame in all situation makes it more clear.

@willhoy willhoy merged commit 32d8fb6 into bloomberg:main May 10, 2024
5 checks passed
@678098 678098 deleted the patch-1 branch May 10, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants