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

[Jonathon Jongsma]

On Thu, 2018-05-24 at 07:38 -0400, Frediano Ziglio wrote:
> > 
> > On Thu, 2018-05-10 at 13:03 -0400, Frediano Ziglio wrote:
> > > > 
> > > > 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.
> > 
> > I'm sure that's true. I was more curious about whether you can
> > actually
> > see any practical performance impact in real-world scenarios.
> > 
> 
> Did some measurement, not strictly real-world scenario.
> For 16 bit pixels is about a 10% difference.
> How much these 16 bit images are used I don't know.

Thanks. It's hard to know how that affects real-world scenarios, but
10% seems useful.


> 
> > > 
> > > 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))
> > 
> > Yes, this seems a bit more readable to me.
> > 
> 
> Oh... I prefer the next one, but they have the same result.

No strong opinion, I find both to be a bit more readable than the
original proposal.

Jonathon

> 
> > > 
> > > or something like
> > > 
> > > #define GET_rgb(pix) ((pix) & 0x7fffu)
> > > #define SAME_PIXEL(p1, p2) (GET_rgb(p1) == GET_rgb(p2))
> > 
> > 
> > Not sure this is necessary.
> > 
> > > 
> > > > 
> > > > > 
> > > > > 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