Is this also something it could make sense to enable/disable for a connection or flow, rather than message by message?

@mattbenjamin I agree with you. I think a per connection compression flag can save bandwidth a lot when communicating with remote OSD for site backup or something else.

I'm not sure, maybe some messages which can't get great compress effect doesn't need to send with compress enabled. I think whether we compress a message isn't decided by connection, and should be considered by message itself. For this case, I prefer to add flag to message footer flag.

That makes sense, and I agree. I'm trying to think of ways to pull conditions out of the decode path...

Is this also something it could make sense to enable/disable for a connection or flow, rather than message by message?

I can imagine that some messages are more compressible than others...

I was kind of thinking the other way, if we have data compressed at a lower layer, does it make sense to think about how to take it and pass it around without a decompression and re-compression step...

@markhpc Could you please share more information on your question? What aspects do you mean for "take it and pass it around without a decompression and re-compression step..."?

@yuyuyu101 I have already make use of footer.flags to tag compressed messages. We can let upper levels to make the decision and set footer.flags earlier.

in the final version you'll need to drop teh first patch (or squash it in to the next one) so that we don't add the recovery-specific code and then remove it in the next patch

I think a sensible approach would be to add Message methods like

virtual should_compress() { return false; }
virtual should_not_compress() { return false; }

That way we can flag any messages that are good or bad candidates. Then we can take a policy that only compressings things that are explicitly flagged as good candidates, or compress everything that is not flagged as a bad candidate?

We'll also need to add a feature bit for compatibility, but we can do that last...

@liewegas in the final version you'll need to drop teh first patch (or squash it in to the next one) so that we don't add the recovery-specific code and then remove it in the next patch

I have removed recovery-specific code originally coding by @XinzeChi in cchengleo@81d64c5

FAIL: the output of run-make-check.sh on centos-centos7 for 23ec502 is http://paste2.org/_p5OeZhJH

Sent from GH.

@liewegas

I think a sensible approach would be to add Message methods like

virtual should_compress() { return false; }
virtual should_not_compress() { return false; }

That way we can flag any messages that are good or bad candidates. Then we can take a policy that only compressings things that are explicitly flagged as good candidates, or compress everything that is not flagged as a bad candidate?

We'll also need to add a feature bit for compatibility, but we can do that last...

Could you please elaborate this a little bit? Are higher level logics use these API to set flag for compression?

It looks to me like you have the feature bit handled correctly for compatibility.

My main concern is that having a Message::compress() and decompress() method adds another dimension of state to the Message struct, which means that other code may potentially need to cope or understand with a compressed Message (or even just that we need to meaningfully represent a compressed Message in memory). Maybe we want to go down that road.... I'm not sure.

But I think a simpler approach would be what I describe in an earlier comment:

1. the msg header struct has the original crc and length values
2. the msg header has bits set if the over-the-wire payload is compressed
3. the msg/simple/Pipe.cc read_message() and write_message() methods have an alternate path that compresses/decompresses the payload right then in private buffers in the local scope.
4. the over-the-write compressed buffer (whether it is front, middle, or data) has the compressed crc, length, and then compressed payload.

Perhaps in the future if we want to represent the compressed message as a valid Message we can tackle that.. but if we avoid it entirely for now I think it's safer and simpler!

An open question for me is whether we leave the header flags for compression set on the Message that we received. This tells others that the data was once compressed on the wire, which might conceivably be interesting, but is someone confusing since it the buffers are no longer compressed. Similarly, if we are sending a message that has those flags set, does that mean we are requesting or forcing compression? We'll need to make sure they are cleared before sending over the wire if, for example, the other end doesn't support compression.

I don't think we should leak the compressed state outside of the messenger, if that's what handles encode/decode.

A more integrated compression system (that, eg, keeps the data compressed until it's passed into the ObjectStore, or even writes it to disk and transparently handles decompressing on update)) might be interesting but a lot more work that I'm not sure is worth it (the reason to do it would be eg memory bandwidth on a system that needs to copy data between threads or something). Outside of a scenario where the external system can actually change its behavior in meaningful ways based on compression state it should be invisible.

FAIL: the output of run-make-check.sh on centos-centos7 for 60895ad3888faafbde6372255bc14d55637dc751 is http://paste2.org/_zcH0LB9W

Sent from GH.

@liewegas
On read_message, front/middle/data_len are used to get read front/middle/data correctly over-the-wire. These fields represents the real length of the following payloads. It also applies to front/middle/data_crc. That's why I didn't emit any original metadata in the header field.

FAIL: the output of run-make-check.sh on centos-centos7 for d8789c0 is http://paste2.org/_fy0f2saI

Sent from GH.

FAIL: the output of run-make-check.sh on centos-centos7 for d1dda41 is http://paste2.org/_9Mpz1h1L

Sent from GH.

FAIL: the output of run-make-check.sh on centos-centos7 for 18d27db is http://paste2.org/_cVmmcvAm

Sent from GH.

I wanted to compress the journal for large write(from my test, (My cluster setup is five osd w/ on ssd as journal. w/o ec & replica size = 1, all data-zero the performance increase 44%. For random-data the performance increate 37%). But at present, there is no compress func/class.
@cchengleo Can you firstly make the first patch merge into the master. After that, i can don my journal compress/decompress. Thanks!

@majianpeng Okay I will put this piece in my TODO list. And make run-make-check pass. From the design aspect, I think the current one is okay to perform necessary functionality.

@cchengleo feel free to re-open when ready.

I heard someone could benefit network message compress for recovery and some use case like vdi. So I would like to push this again. @cchengleo Would you have a new impl or reopen it again to make complete?

I can follow up this PR. Let me take a look for current network impl and make changes to accommodate. I also would like to make it complete.

Sincerely,
Cheng

On May 24, 2015, at 12:36 PM, Haomai Wang notifications@github.com wrote:

I heard someone could benefit network message compress for recovery and some use case like vdi. So I would like to push this again. @cchengleo Would you have a new impl or reopen it again to make complete?

—
Reply to this email directly or view it on GitHub.

@cchengleo I hope we can get some progress for this :-) I would like to help to complete this PR if you have any problem.

Actually we may like to do compress even in messenger level async. Online compress within messenger thread may affect network bandwidth hugely

@yuyuyu101 Thanks for the help! I am recently get busy with other stuffs. Please go ahead and implement what you proposed. I will try to help also.

I am not quite get why online compressing affect network bandwidth hugely. Can you further elaborate?