Protocol message compression support #3287

gregtatcam · 2020-03-06T12:57:43Z

Peers negotiate compression via HTTP Headers X-Offer-Compression:lz4. If the client and the server agree on the compression/algorithm (only lz4 is currently supported) then the protocol messages between the peers are compressed when possible. Messages greater than 70 bytes and MANIFESTS, ENDPOINTS, TRANSACTION, GET_LEDGER, LEDGER_DATA, GET_OBJECT, and VALIDATORLIST messages are compressed. If the compressed message is larger than the uncompressed message then the uncompressed message is sent. Compression flag and the compression algorithm type are included in the payload header.

codecov-io · 2020-03-06T13:56:30Z

Codecov Report

Merging #3287 into develop will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3287   +/-   ##
========================================
  Coverage    70.46%   70.46%           
========================================
  Files          678      678           
  Lines        54462    54462           
========================================
  Hits         38375    38375           
  Misses       16087    16087

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51a0c5c...51a0c5c. Read the comment docs.

seelabs

Nice job on this! I did a pass and left some comments. Mostly nits.

src/ripple/app/consensus/RCLConsensus.cpp

src/ripple/app/misc/ValidatorList.h

src/ripple/basics/CompressionAlgorithms.h

src/ripple/overlay/impl/Message.cpp

src/ripple/overlay/impl/ProtocolMessage.h

src/ripple/app/misc/impl/ValidatorSite.cpp

nbougalis · 2020-03-11T05:27:07Z

src/ripple/basics/CompressionAlgorithms.h

+ */
+template<typename BufferFactory>
+std::pair<void const*, std::size_t>
+lz4fCompress(void const * in,


This is cool, but maybe a little too cool and it requires all sorts of magic further down. I think that it's better to avoid all of this and encode the size in an expanded binary header, and use a much simpler implementation here, similar to what the nodestore uses with lz4_compress and lz4_decompress.

The problem is that the message is sent as one buffer but received into multiple buffers. If we use lz4_compress (block compression) on send then on receive the multiple buffers have to be copied into one buffer to pass to lz4_decompress which is undesirable. lz4 frame compression can compress the whole message and decompress one chunk at a time so the multiple buffers can be passed in as zero copy input stream. The only issue with lz4 frame decompression is that it may not consume all input bytes from a chunk. In this case the remaining bytes have to be copied and concatenated with the next input chunk. I have not seen this happen though in my test on 32 million of protocol messages captured from the mainnet. But this case still has to be coded. On another hand the captured sample data also shows that 1) 69% of the messages are received into one buffer; 2) 29% of the messages are received into two buffers; 3) 98% out of the 29% have the message size less than 500 bytes. So the copying might not be a substantial problem after all. Perhaps more data should be captured before we can decide on what compression mode is best to use - block or frame.

src/ripple/basics/CompressionAlgorithms.h

nbougalis · 2020-03-12T05:04:53Z

src/ripple/overlay/impl/Message.cpp

+            return false;
+        switch(type)
+        {
+            case protocol::mtMANIFESTS:


I understand the intention here (to avoid wasting CPU resources on messages that are unlikely to benefit from compression) but I am concerned that this approach is fragile: anytime we add a message type that can benefit from compression we need to remember to add support support for it here.

Also, consider a message like TMTransaction which can (depending on the transaction) be rather large and might benefit from compression.

I don't know that I have a better solution; just calling this out.

Majority of the messages are not compressible (88% in the sample). The filtering is desirable because both CPU and memory (have to allocate compressed message buffer) will be used. Perhaps we could mitigate maintainability concern by adding compiler warning when a case is missing in the switch?

nbougalis · 2020-03-12T05:09:34Z

src/ripple/overlay/impl/Message.cpp

+                    return (mBufferCompressed.data() + headerBytes);
+                });
+
+        double ratio = 1.0 - (double)compressedSize / (double)messageBytes;


I am allergic to floating-point stuff so just seeing the keyword makes my spidey sense tingle. I get what you're trying to do here: avoid expansion (and potentially enforce a minimum before forcing the other end to do work) however I think that we are better off not doing any of this.

Instead, I'd recommend using LZ4_compress_default which does some of this for you: if the result of the compressed data would have been larger than the uncompressed data, it returns 0 (i.e. "failed") and not worrying about a minimum compression ratio.

LZ4_compress_default may return non-zero and still compress into larger buffer. I changed to checking if the compressed size is less than uncompressed.

nbougalis · 2020-03-12T16:59:08Z

src/ripple/overlay/impl/ConnectAttempt.cpp

@@ -275,6 +275,8 @@ ConnectAttempt::makeRequest (bool crawl) -> request_type
    m.insert ("Connection", "Upgrade");
    m.insert ("Connect-As", "Peer");
    m.insert ("Crawl", crawl ? "public" : "private");
+    if (compressionEnabled)
+        m.insert("Accept-Encoding", "lz4");


I get why you're using Accept-Encoding but since our handling of this header isn't quite standards-compliant, I'm not sure whether using a different header name is better. For example, X-Offer-Compression (both in the request and the response)?

nbougalis · 2020-03-12T17:15:17Z

src/ripple/basics/CompressionAlgorithms.h

+
+namespace compression_algorithms {
+
+inline void doThrow(const char *message)


This function is invoked from multiple places and I don't know that they're all from within a try/catch block, or that the catch is appropriately scoped. We need to be careful when throwing to ensure that not only will the exception be caught but that continuing after the handler doesn't cause weirdness.

Added try/catch in Compression.h module.

nbougalis · 2020-03-12T17:22:58Z

src/ripple/basics/CompressionAlgorithms.h

+{
+    bool res = in.Next(&compressedChunk, &chunkSize);
+
+    if (!res && buffer.size() == 0)


I think that !buffer.empty() reads better both here and on line 167.

nbougalis · 2020-03-12T17:30:39Z

src/ripple/overlay/Message.h

+        if (compressed == Compressed::Off)
+            return mBuffer;
+
+        if (!mCompressedRequested)


Minor nit, but given the usage, this is probably better named: alreadyCompressed_.

Also I'm concerned about thread-safety here; I'm not sure it's a problem, but we need to triple-check that it's not. I'd almost feel better if we just did the compression during construction of the message, so that it never became an issue.

Changed to alreadyRequested_. The message may or may not be compressed.

nbougalis · 2020-03-12T17:42:41Z

src/test/overlay/compression_test.cpp

@@ -0,0 +1,374 @@
+//------------------------------------------------------------------------------


👍 for the unit tests here.

src/ripple/overlay/Message.h

src/ripple/overlay/impl/Message.cpp

src/ripple/overlay/impl/ProtocolMessage.h

src/ripple/overlay/Compression.h

src/ripple/overlay/Message.h

src/ripple/overlay/impl/Message.cpp

src/ripple/overlay/Compression.h

src/ripple/basics/varint_common.h

HowardHinnant · 2020-03-24T16:11:40Z

src/ripple/app/misc/ValidatorList.h

@@ -239,6 +239,8 @@ class ValidatorList
        @param hashRouter HashRouter object which will determine which
            peers not to send to

+        @param compressionEnabled is true if compression is enabled in rippled.cfg
+


Looks like this documentation got lost.

It's no longer needed. Will delete.

HowardHinnant · 2020-03-24T16:24:16Z

src/ripple/overlay/impl/PeerImp.cpp

@@ -86,6 +86,7 @@ PeerImp::PeerImp (Application& app, id_t id,
    , slot_ (slot)
    , request_(std::move(request))
    , headers_(request_)
+    , compressionEnabled_(headers_["X-Offer-Compression"] == "lz4" ? Compressed::On : Compressed::Off)


Does this need && app_.config().COMPRESSION as was done in the other PeerImp constructor?

This is constructor for a peer making outbound connection. The header is not going to have the X-Offer-Compression header if the compression is not enabled so no need to check for COMPRESSION. The other constructor is for the inbound peer. We are not going to send compressed message to that peer if the compression is not enabled so we check for COMPRESSION.

src/ripple/basics/varint_common.h

src/ripple/overlay/impl/Message.cpp

seelabs

👍 Nice job on this!

gregtatcam · 2020-03-25T15:09:18Z

Thanks for good comments!

…

On Wed, Mar 25, 2020 at 11:04 AM Scott Determan ***@***.***> wrote: ***@***.**** approved this pull request. 👍 Nice job on this! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3287 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZPWKVEKTJM5OSFINCUW73RJIMN7ANCNFSM4LC7U6BQ> .

* Peers negotiate compression via HTTP Header "X-Offer-Compression: lz4" * Messages greater than 70 bytes and protocol type messages MANIFESTS, ENDPOINTS, TRANSACTION, GET_LEDGER, LEDGER_DATA, GET_OBJECT, and VALIDATORLIST are compressed * If the compressed message is larger than the uncompressed message then the uncompressed message is sent * Compression flag and the compression algorithm type are included in the message header * Only LZ4 block compression is currently supported

seelabs

👍

* Peers negotiate compression via HTTP Header "X-Offer-Compression: lz4" * Messages greater than 70 bytes and protocol type messages MANIFESTS, ENDPOINTS, TRANSACTION, GET_LEDGER, LEDGER_DATA, GET_OBJECT, and VALIDATORLIST are compressed * If the compressed message is larger than the uncompressed message then the uncompressed message is sent * Compression flag and the compression algorithm type are included in the message header * Only LZ4 block compression is currently supported

intelliot · 2023-09-19T17:18:12Z

Released in 1.6.0

P2P link compression is a feature added in 1.6.0 by #3287. https://xrpl.org/enable-link-compression.html If the default changes in the future - for example, as currently proposed by #4387 - the comment will be updated at that time. Fix #4656

P2P link compression is a feature added in 1.6.0 by XRPLF#3287. https://xrpl.org/enable-link-compression.html If the default changes in the future - for example, as currently proposed by XRPLF#4387 - the comment will be updated at that time. Fix XRPLF#4656

nbougalis requested review from nbougalis, seelabs and HowardHinnant March 6, 2020 16:24

scottschurr assigned nbougalis, seelabs and HowardHinnant Mar 6, 2020

seelabs reviewed Mar 10, 2020

View reviewed changes

gregtatcam requested a review from seelabs March 11, 2020 17:55

nbougalis suggested changes Mar 12, 2020

View reviewed changes

seelabs reviewed Mar 20, 2020

View reviewed changes

src/ripple/basics/varint_common.h Outdated Show resolved Hide resolved

HowardHinnant reviewed Mar 24, 2020

View reviewed changes

seelabs reviewed Mar 24, 2020

View reviewed changes

src/ripple/basics/varint_common.h Outdated Show resolved Hide resolved

seelabs reviewed Mar 24, 2020

View reviewed changes

src/ripple/overlay/impl/Message.cpp Outdated Show resolved Hide resolved

seelabs previously approved these changes Mar 25, 2020

View reviewed changes

HowardHinnant previously approved these changes Mar 26, 2020

View reviewed changes

gregtatcam dismissed stale reviews from HowardHinnant and seelabs via d6b988c March 26, 2020 20:04

gregtatcam force-pushed the compression branch from 7d58515 to d6b988c Compare March 26, 2020 20:04

gregtatcam force-pushed the compression branch from d6b988c to 4b7991b Compare March 26, 2020 22:06

carlhua added the Documentation README changes, code comments, etc. label Mar 27, 2020

seelabs approved these changes Mar 27, 2020

View reviewed changes

HowardHinnant self-requested a review March 27, 2020 18:58

HowardHinnant approved these changes Mar 27, 2020

View reviewed changes

carlhua requested a review from nbougalis April 3, 2020 13:31

carlhua added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Apr 3, 2020

This was referenced Apr 7, 2020

Proposed 1.6.0-b1 #3334

Closed

Proposed 1.6.0-b1 #3338

Merged

manojsdoshi closed this in #3338 Apr 7, 2020

manojsdoshi mentioned this pull request Apr 8, 2020

Signed 1.6.0-b1 #3340

Merged

mDuo13 mentioned this pull request Aug 3, 2020

Add link compression tutorial XRPLF/xrpl-dev-portal#881

Merged

intelliot mentioned this pull request Jan 18, 2023

Enable by default the compression support #4387

Closed

7 tasks

intelliot mentioned this pull request Aug 15, 2023

Add comment about link compression in rippled-example.cfg #4656

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protocol message compression support #3287

Protocol message compression support #3287

gregtatcam commented Mar 6, 2020 •

edited

Loading

codecov-io commented Mar 6, 2020 •

edited

Loading

seelabs left a comment

nbougalis Mar 11, 2020

gregtatcam Mar 13, 2020

nbougalis Mar 12, 2020

gregtatcam Mar 13, 2020

nbougalis Mar 12, 2020

gregtatcam Mar 13, 2020

nbougalis Mar 12, 2020

gregtatcam Mar 13, 2020

nbougalis Mar 12, 2020

gregtatcam Mar 17, 2020

nbougalis Mar 12, 2020

gregtatcam Mar 13, 2020

nbougalis Mar 12, 2020

gregtatcam Mar 17, 2020

nbougalis Mar 12, 2020

HowardHinnant Mar 24, 2020

gregtatcam Mar 24, 2020

HowardHinnant Mar 24, 2020

gregtatcam Mar 24, 2020

seelabs left a comment

gregtatcam commented Mar 25, 2020 via email

seelabs left a comment

intelliot commented Sep 19, 2023


		namespace compression_algorithms {

		inline void doThrow(const char *message)

		@@ -0,0 +1,374 @@
		//------------------------------------------------------------------------------

Protocol message compression support #3287

Protocol message compression support #3287

Conversation

gregtatcam commented Mar 6, 2020 • edited Loading

codecov-io commented Mar 6, 2020 • edited Loading

Codecov Report

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

gregtatcam commented Mar 25, 2020 via email

seelabs left a comment

Choose a reason for hiding this comment

intelliot commented Sep 19, 2023

gregtatcam commented Mar 6, 2020 •

edited

Loading

codecov-io commented Mar 6, 2020 •

edited

Loading