Re: [Qemu-devel] [PATCH v5 08/10] xbzrle: check 8 bytes at a time after an concurrency scene

[陈梁 <chenliang0016@icloud.com>]

> * arei.gonglei@huawei.com (arei.gonglei@huawei.com) wrote:
>> From: ChenLiang <chenliang88@huawei.com>
>> 
>> The logic of old code is correct. But Checking byte by byte will
>> consume time after an concurrency scene.
>> 
>> Signed-off-by: ChenLiang <chenliang88@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>> xbzrle.c | 28 ++++++++++++++++++----------
>> 1 file changed, 18 insertions(+), 10 deletions(-)
>> 
>> diff --git a/xbzrle.c b/xbzrle.c
>> index 92cccd7..9d67309 100644
>> --- a/xbzrle.c
>> +++ b/xbzrle.c
>> @@ -51,16 +51,24 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen,
>> 
>>         /* word at a time for speed */
>>         if (!res) {
>> -            while (i < slen &&
>> -                   (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {
>> -                i += sizeof(long);
>> -                zrun_len += sizeof(long);
>> -            }
>> -
>> -            /* go over the rest */
>> -            while (i < slen && old_buf[i] == new_buf[i]) {
>> -                zrun_len++;
>> -                i++;
>> +            while (i < slen) {
>> +                if ((*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {
>> +                    i += sizeof(long);
>> +                    zrun_len += sizeof(long);
>> +                } else {
>> +                    /* go over the rest */
>> +                    for (j = 0; j < sizeof(long); j++) {
>> +                        if (old_buf[i] == new_buf[i]) {
>> +                            i++;
>> +                            zrun_len++;
>> +                        } else {
>> +                            break;
>> +                        }
>> +                    }
>> +                    if (j != sizeof(long)) {
>> +                        break;
> 
> Is it not possible to make this code the same as the other loop, you could xor in the same
> way just change the comparison?
> (What do other people think - I was thinking that would just be better since it would
> be symmetric?)
> 
> Dave
Hi,
yeah, It will be symmetric. They are equivalence like this:
(*(long *)(old_buf + i)) == (*(long *)(new_buf + i))

!(xor = (*(long *)(old_buf + i)) ^ (*(long *)(new_buf + i)))

But the second is not efficient, IMHO. Because xbzrle assumes that most of bytes in the dirty
page is not modified. The result of xor will be not useful mostly. ps: It's just my idea.

Best regards
Chen Liang
>> +                    }
>> +                }
>>             }
>>         }
>> 
>> -- 
>> 1.7.12.4
>> 
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>