Re: [Qemu-devel] [PATCH 2/2] migration/xbzrle: make xbzrle_encode_buffer little easier to read

From: Wei Yang <richardw.yang@linux.intel.com>
To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: quintela@redhat.com, Wei Yang <richardw.yang@linux.intel.com>,
	qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 2/2] migration/xbzrle: make xbzrle_encode_buffer little easier to read
Date: Wed, 12 Jun 2019 08:33:14 +0800	[thread overview]
Message-ID: <20190612003314.GA6627@richard> (raw)
In-Reply-To: <20190611175926.GJ2777@work-vm>

On Tue, Jun 11, 2019 at 06:59:26PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> The encoding process could be described below:
>> 
>>     for [content]
>>         get length of a run
>>         encode this run
>> 
>> By refactoring the code with this logic, it maybe a little easier to
>> understand.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/xbzrle.c | 153 +++++++++++++++++++++------------------------
>>  1 file changed, 73 insertions(+), 80 deletions(-)
>> 
>> diff --git a/migration/xbzrle.c b/migration/xbzrle.c
>> index 1ba482ded9..25c69708ec 100644
>> --- a/migration/xbzrle.c
>> +++ b/migration/xbzrle.c
>> @@ -14,6 +14,59 @@
>>  #include "qemu/cutils.h"
>>  #include "xbzrle.h"
>>  
>> +static int next_run(uint8_t *old_buf, uint8_t *new_buf, int off, int slen,
>> +                    bool zrun)
>> +{
>> +    uint32_t len = 0;
>> +    long res;
>> +
>> +    res = (slen - off) % sizeof(long);
>> +
>> +    /* first unaligned bytes */
>> +    while (res) {
>> +        uint8_t xor = old_buf[off + len] ^ new_buf[off + len];
>> +
>> +        if (!(zrun ^ !!xor)) {
>> +            break;
>> +        }
>> +        len++;
>> +        res--;
>> +    }
>> +
>> +    if (res) {
>> +        return len;
>> +    }
>> +
>> +    /* word at a time for speed, use of 32-bit long okay */
>> +    while (off + len < slen) {
>> +        /* truncation to 32-bit long okay */
>> +        unsigned long mask = (unsigned long)0x0101010101010101ULL;
>> +        long xor = (*(long *)(old_buf + off + len)) ^
>> +                   (*(long *)(new_buf + off + len));
>> +
>> +        if (zrun && !(zrun ^ !!xor)) {
>
>Are lines like this really making it easier to read?
>

Yep, this may took some time to understand. Let me put a chart to explain.

We have two choices for both zrun and xor, so we have 4 combinations:

    xor |  0 (no change) |  !0 (changed) 
 zrun   |                |
 -------+----------------+--------------
  1     |  *             |  x
 -------+----------------+--------------
  0     |  x             |  *

We can see the situation with '*' is the one we are looking for. And those
situations could be expressed with (zrun ^ xor). Since we need to convert xor
to something like boolean, so xor is converted to !!xor.

But yes, you are right. This part is not as easy as original one. In case you
don't like this, we can change it back to the original version.

>Juan: Opinion?
>
>Dave
>
-- 
Wei Yang
Help you, Help me