Improve compression support: #3419
Conversation
nbougalis
commented
May 29, 2020
nbougalis
commented
- Optimize parsing of compressed message headers
- Enforce protocol-defined message size maxima
- Update comments
| if (size < hdr.header_size) | ||
| return {}; | ||
| { |
There was a problem hiding this comment.
I prefer a style that checks for errors and returns early as opposed for checking for success. That style tends to reduce nesting indentation levels. Most of the code in this function is written this way, but this block doesn't do that. It's a minor nit, so no change needed, but since I don't have many comments in this PR I thought I'd point it out.
I.e
if ((*iter & 0xFC) != 0)
{
ec = make_error_code(boost::system::errc::no_message);
return boost::none;
}
hdr.header_size = headerBytes;
// ect.
| @@ -84,21 +84,14 @@ class compression_test : public beast::unit_test::suite | |||
| std::shared_ptr<T> proto, | |||
| protocol::MessageType mt, | |||
| uint16_t nbuffers, | |||
| const char* msg, | |||
| bool log = false) | |||
| std::string msg) | |||
|
These changes are non-trivial enough that we should probably assign another reviewer as well. |
| enum class Algorithm : std::uint8_t { None = 0x00, LZ4 = 0x01 }; | ||
| // All values other than 'none' must have the high bit. The low order four bits | ||
| // must be 0. | ||
| enum class Algorithm : std::uint8_t { None = 0x00, LZ4 = 0x90 }; |
There was a problem hiding this comment.
Why the change from 0x01 to 0x90?
There was a problem hiding this comment.
The new framing for compressed messages alters the first byte of the header, and specifically the first four bits: If the high bit is set, it means the message is compressed. The remaining three bits specify the compression algorithm used.
0x90 is 1001000 in binary: high bit set (compression enabled) , low bit of high nibble set (LZ4).
While reading your comment, it occurred to me to check when we set this value, and I noticed that the code there is wrong. It does:
*h |= 0x80 | (static_cast<uint8_t>(comprAlgorithm) << 4);
Obviously not the right thing, this should now be: *h |= static_cast<std::uint8_t>(comprAlgorithm); instead. For the record, this was a bug that I introduced into this PR and not @gregtatcam.
I'll push a [FOLD] commit to address this.
Thanks @MarkusTeufelberger! Sharp eyes!
* Optimize parsing of compressed message headers * Enforce protocol-defined message size maxima * Update comments
There was a problem hiding this comment.
I'm not going to click Request Changes, because I am going on PTO and I don't want to block the merge. But I strongly suggest adding a layer of abstraction for all of the bitwise operations in parseMessageHeader, per my comments.
Sorry for leaving two separate reviews. My comments were originally part of a resolved conversation, so I moved them.
| * @param ec contains more information: | ||
| * - set to `errc::success` if not enough bytes were present | ||
| * - set to `errc::no_message` if a valid header was not present | ||
| */ | ||
| template <class BufferSequence> | ||
| boost::optional<MessageHeader> |
There was a problem hiding this comment.
Can we change this to std::optional? I think we are trying to migrate away from boost::optional
There was a problem hiding this comment.
We can. Shouldn't be a problem.
| template <class BufferSequence> | ||
| boost::optional<MessageHeader> | ||
| parseMessageHeader(BufferSequence const& bufs, std::size_t size) | ||
| parseMessageHeader( |
There was a problem hiding this comment.
What do you think of putting the bitwise comparison inside of a named lambda, where the name describes what is being checked? I don't really like bitwise operations in a conditional directly, because it doesn't read that easily. It always takes me a second to figure out whats going on, and I think sometimes subtle bugs can occur. Remember this: #3417? At the least, I think we should add a comment saying exactly what is being checked.
There was a problem hiding this comment.
Actually, after reading through the whole function, I think we should abstract away these bitwise operations to a class, or a bunch of helper functions. Bitwise operations are pretty low level, and I think it's hard to read the code and easy to mess up. We could compromise on more judicious use of comments (I would put a clear comment before every bitwise operation), but I'd really prefer named functions; it's pretty easy for a developer to change the code and forget to update the comment, but it's harder to change the function implementation and forget to update the function name.
There was a problem hiding this comment.
I hear what you're saying, but I think that adding additional lambas would only add confusion. Better comments would probably help; I'll try and follow-up.
|
Looks good to me |
carlhua
added
the
Passed
