On 07/05/2012 04:55 PM, Eric Blake wrote:
> On 07/05/2012 06:51 AM, Orit Wasserman wrote:
>
> This commit message is a bit sparse. I'd document at least the fact
> that our nzrun detection code in xbzrle_encode_buffer borrows
> long-word-at-a-time NUL-detection tricks from strcmp(), as it is not an
> intuitive trick known by all developers.
>
>> Signed-off-by: Benoit Hudzia <benoit.hud...@sap.com>
>> Signed-off-by: Petter Svard <pett...@cs.umu.se>
>> Signed-off-by: Aidan Shribman <aidan.shrib...@sap.com>
>> Signed-off-by: Orit Wasserman <owass...@redhat.com>
>
> I think I touched this one heavily enough that it warrants adding:
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
Of course
Orit
> Other than the commit message, I'm happy with this patch contents now.
> Still some nits, though:
>
>> +
>> + /* not aligned to sizeof(long) */
>> + res = (slen - i) % sizeof(long);
>
> Comment indentation is off.
>
>> + while (res && old_buf[i] == new_buf[i]) {
>> + zrun_len++;
>> + i++;
>> + res--;
>> + }
>> +
>> + if (!res) {
>
> A comment here might help:
>
> /* word at a time for speed */
>
>> + while (i < slen &&
>> + (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {
>
> On re-reading this, I'm worried whether it violates strict C99 aliasing
> rules; I'm hoping the compiler doesn't mis-optimize it based on a
> loophole of us using questionable semantics. In practice, I'm confident
> that we are only doing aligned reads (otherwise I wouldn't have
> suggested this line in the first place), which is why I'm personally
> okay with it even if it technically violates C99. However, I'm open to
> suggestions from anyone else on the list on how to better express the
> notion of intentionally accessing a long at a time from an aligned
> offset into a byte array, if that will help us avoid even theoretical
> problems.
This is a very interesting question ....
Orit
>
>> + nzrun_len++;
>> + res--;
>> + }
>> +
>> + if (!res) {
>> + /* truncation to 32-bit long okay */
>
> Again, a longer comment might help:
>
> /* word at a time for speed, use of 32-bit long okay */
>
>> + long mask = 0x0101010101010101ULL;
>> + while (i < slen) {
>> + xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);
>
> Same potential strict aliasing problems as for zrun detection.
>
