Re: pglz compression performance, take two

> On Jun 27, 2021, at 3:41 AM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
> And here's what I've come up with.

I have not tested the patch yet, but here are some quick review comments:


> #define PGLZ_HISTORY_SIZE       0x0fff - 1  /* to avoid compare in iteration */
...
> static PGLZ_HistEntry hist_entries[PGLZ_HISTORY_SIZE + 1];
...
>     if (hist_next == PGLZ_HISTORY_SIZE + 1)

These are the only uses of PGLZ_HISTORY_SIZE.  Perhaps you could just defined the symbol as 0x0fff and skip the -1 and
+1business? 

> /* ----------
>  * pglz_compare -
>  *
>  *      Compares 4 bytes at pointers
>  * ----------
>  */
> static inline bool
> pglz_compare32(const void *ptr1, const void *ptr2)
> {
>     return memcmp(ptr1, ptr2, 4) == 0;
> }

The comment function name differs from the actual function name.

Also, pglz_compare returns an offset into the string, whereas pglz_compare32 returns a boolean.  This is fairly
unintuitive. The "32" part of pglz_compare32 implies doing the same thing as pglz_compare but where the string is known
tobe 4 bytes in length.  Given that pglz_compare32 is dissimilar to pglz_compare, perhaps avoid using /pglz_compare/ in
itsname? 

>         /*
>          * Determine length of match. A better match must be larger than the
>          * best so far. And if we already have a match of 16 or more bytes,
>          * it's worth the call overhead to use memcmp()

This comment is hard to understand, given the code that follows.  The first block calls memcmp(), which seems to be the
functionoverhead you refer to.  The second block calls the static inline function pglz_compare32, which internally
callsmemcmp().  Superficially, there seems to be a memcmp() function call either way.  The difference is that in the
firstblock's call to memcmp(), the length is a runtime value, and in the second block, it is a compile-time known
value. If you are depending on the compiler to notice this distinction and optimize the second call, perhaps you can
mentionthat explicitly?  Otherwise, reading and understanding the comment takes more effort. 

I took a quick look for other places in the code that try to beat the performance of memcmp on short strings.  In
varlena.c,rest_of_char_same() seems to do so.  We also use comparisons on NameData, which frequently contains strings
shorterthan 16 bytes.  Is it worth sharting a static inline function that uses your optimization in other places?  How
confidentare you that your optimization really helps? 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company