[Spice-devel] [PATCH spice-common 2/3] lz: Optimise SAME_PIXEL for RGB16

[Frediano Ziglio]

> 
> On Wed, 2018-05-09 at 14:10 +0100, Frediano Ziglio wrote:
> > Do not extract all components and compare one by one, can be easily
> > compared together.
> 
> Do you have some data about how performance compares between the two
> implementations? The new implementation clearly looks more efficient,
> but the code is not as straightforward to understand at a glance (at
> least for me). If it's called a lot, it clearly makes sense to optimize
> the implementation, but if not, I'm a bit ambivalent about the change.
> 
> Jonathon
> 

Looked at produced assembly code and is smaller.

A similar macro (but a bit more readable) can be:

#define SAME_PIXEL(p1, p2) (((p1) & 0x7fffu) == ((p2) & 0x7fffu))

A #define RGB16_BITMASK 0x7fffu can be defined to make even more clear, like

#define RGB16_BITMASK 0x7fffu
#define SAME_PIXEL(p1, p2) (((p1) & RGB16_BITMASK) == ((p2) & RGB16_BITMASK))

or something like

#define GET_rgb(pix) ((pix) & 0x7fffu)
#define SAME_PIXEL(p1, p2) (GET_rgb(p1) == GET_rgb(p2))

> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  common/lz_compress_tmpl.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/lz_compress_tmpl.c b/common/lz_compress_tmpl.c
> > index 69e69a6..b778e9d 100644
> > --- a/common/lz_compress_tmpl.c
> > +++ b/common/lz_compress_tmpl.c
> > @@ -105,9 +105,7 @@
> >  #ifdef LZ_RGB16
> >  #define PIXEL rgb16_pixel_t
> >  #define FNAME(name) lz_rgb16_##name
> > -#define GET_r(pix) (((pix) >> 10) & 0x1f)
> > -#define GET_g(pix) (((pix) >> 5) & 0x1f)
> > -#define GET_b(pix) ((pix) & 0x1f)
> > +#define SAME_PIXEL(p1, p2) ((((p1)^(p2)) & 0x7fffu) == 0)
> >  #define ENCODE_PIXEL(e, pix) {encode(e, (pix) >> 8); encode(e, (pix)
> > & 0xff);}
> >  
> >  #define HASH_FUNC(v, p) {                 \
> > @@ -153,7 +151,7 @@
> >      }
> >  #endif
> >  
> > -#if defined(LZ_RGB16) || defined(LZ_RGB24) || defined(LZ_RGB32)
> > +#if defined(LZ_RGB24) || defined(LZ_RGB32)
> >  #define SAME_PIXEL(p1, p2) (GET_r(p1) == GET_r(p2) && GET_g(p1) ==
> > GET_g(p2) && \
> >                              GET_b(p1) == GET_b(p2))
> >  
> 

Frediano