Re: [RFC] LZO de/compression support - take 4

["Satyam Sharma" <satyam.sharma@gmail.com>]

On 5/25/07, Nitin Gupta <nitingupta910@gmail.com> wrote:
> Hi,
>
> This is kernel port of LZO1X compressor and LZO1X decompressor (safe
> version only).
>
> * Changes since 'take 3' (Full Changelog after this):
> 1) Removed 'unsafe' decompressor - hence also do away with symlinks in
> Makefiles.

Nice :-)

> 2) Rolled back changes where I replaced COPY4 with memcpy() calls.
> This seemed to be causing too much perf. loss as shown by Richard's
> tests. Need perf. testing again to confirm that this patch has perf.
> comparable to original LZO code/Richard's patch.

Hmmm, you've done away with a lot of cleanup in one big swoop under
suspicion of what exactly causes the performance loss ... so I'd rather
wait till you pinpoint the exact cause.

> 3) Some functions were inlined (DX2, DX3 etc.) - this also seemed to
> be one of factors for perf. loss. Changed them back Macros.

Why would an inline function be slower than a macro? If you're
worried that gcc may ignore to inline them (which is unlikely as it is
with -O2), you can use __always_inline ... inline functions do provide
type-checking.

> 4) Added back the 'register' keyword - again seemed to me one of
> factors for perf. loss.

Again, I wonder whether the programmer's opinion to keep something in
the registers is necessarily better than the compiler's. Especially when
gcc would quite likely ignore your register hint in any case, can't think
of too many reasons to use register these days ...

> Once I pinpoint exact reason for bad perf., I will do above cleanups
> again. But this should not be reason for non-inclusion in mainline.
> These are only minor cleanups.

Right, it'd be nice if you do a step-by-step cleanup to find what exactly
caused the performance hit.

> Richard, can you please provide perf. results for this patch also?
> Also, can you please mail back latest version of your LZO patch? In
> meantime, I will try to include benchmarking support to the
> 'compress-test' module.

I wonder if you could try and use crypto/tcrypt.c -- the debugfs test
module involves userspace, the VFS layer etc ... tcrypt otoh is fully
within the kernel so less likely to introduce spurious noise to the numbers
and better suited to performance benchmarking.

> 5) The only code change: (as suggested by Andrey)
> -#if defined(__LITTLE_ENDIAN)
> -                             m_pos = op - 1;
> -                             m_pos -= (*(const unsigned short *)ip) >> 2;
> -#else
> -                             m_pos = op - 1;
> -                             m_pos -= (ip[0] >> 2) + (ip[1] << 6);
> -#endif
>
> + m_pos = op - 1 - (cpu_to_le16(*(const u16 *)ip) >> 2);
>
> (Andrey suggested le16_to_cpu for above but I think it should be cpu_to_le16).
> *** Need testing on big endian machine ***
>
> Similarly:
> -#if defined(__LITTLE_ENDIAN)
> -                             m_pos -= (*(const unsigned short *)ip) >> 2;
> -#else
> -                             m_pos -= (ip[0] >> 2) + (ip[1] << 6);
> -#endif
>
> + m_pos -= cpu_to_le16(*(const u16 *)ip) >> 2;

You could try and rollback this change too. Also, try to get results
on various arch's yourself (if you can lay your hands on some x86-64
/ sparc64 / etc boxen).

Otherwise, if it's still slower (and we're talking >10% here), there's not
much reason why we shouldn't stay with Richard's patches itself. You
could try and do cleanups to that itself (if Richard's fine with it and as
long as you retain performance).

Satyam