> On Jun 27, 2021, at 3:41 AM, Andrey Borodin <x4...@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 +1 business?
> /* ----------
> * 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 to be 4 bytes in length. Given that pglz_compare32 is
dissimilar to pglz_compare, perhaps avoid using /pglz_compare/ in its name?
> /*
> * 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 function overhead you refer to.
The second block calls the static inline function pglz_compare32, which
internally calls memcmp(). Superficially, there seems to be a memcmp()
function call either way. The difference is that in the first block'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 mention that
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 shorter than 16 bytes. Is it worth sharting a static inline function
that uses your optimization in other places? How confident are you that your
optimization really helps?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
