Re: pglz performance

Hi Tomas!

Thanks for benchmarking this!

> 26 нояб. 2019 г., в 14:43, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> написал(а):
>
> On Mon, Nov 25, 2019 at 05:29:40PM +0900, Michael Paquier wrote:
>> On Mon, Nov 25, 2019 at 01:21:27PM +0500, Andrey Borodin wrote:
>>> I think status Needs Review describes what is going on better. It's
>>> not like something is awaited from my side.
>>
>> Indeed. You are right so I have moved the patch instead, with "Needs
>> review". The patch status was actually incorrect in the CF app, as it
>> was marked as waiting on author.
>>
>> @Tomas: updated versions of the patches have been sent by Andrey.
>
> I've done benchmarks on the two last patches, using the data sets from
> test_pglz repository [1], but using three simple queries:
>
> 1) prefix - first 100 bytes of the value
>
> SELECT length(substr(value, 0, 100)) FROM t
>
> 2) infix - 100 bytes from the middle
>
> SELECT length(substr(value, test_length/2, 100)) FROM t
>
> 3) suffix - last 100 bytes
>
> SELECT length(substr(value, test_length - 100, 100)) FROM t
>
> See the two attached scripts, implementing this benchmark. The test
> itself did a 60-second pgbench runs (single client) measuring tps on two
> different machines.
>
> patch 1: v4-0001-Use-memcpy-in-pglz-decompression.patch
> patch 2: v4-0001-Use-memcpy-in-pglz-decompression-for-long-matches.patch
>
> The results (compared to master) from the first machine (i5-2500k CPU)
> look like this:
>
> patch 1 | patch 2
> dataset prefix infix suffix | prefix infix suffix
> -------------------------------------------------------------------------
> 000000010000000000000001 99% 134% 161% | 100% 126% 152%
> 000000010000000000000006 99% 260% 287% | 100% 257% 279%
> 000000010000000000000008 100% 100% 100% | 100% 95% 91%
> 16398 100% 168% 221% | 100% 159% 215%
> shakespeare.txt 100% 138% 141% | 100% 116% 117%
> mr 99% 120% 128% | 100% 107% 108%
> dickens 100% 129% 132% | 100% 100% 100%
> mozilla 100% 119% 120% | 100% 102% 104%
> nci 100% 149% 141% | 100% 143% 135%
> ooffice 99% 121% 123% | 100% 97% 98%
> osdb 100% 99% 99% | 100% 100% 99%
> reymont 99% 130% 132% | 100% 106% 107%
> samba 100% 126% 132% | 100% 105% 111%
> sao 100% 100% 99% | 100% 100% 100%
> webster 100% 127% 127% | 100% 106% 106%
> x-ray 99% 99% 99% | 100% 100% 100%
> xml 100% 144% 144% | 100% 130% 128%
>
> and on the other one (xeon e5-2620v4) looks like this:
>
> patch 1 | patch 2
> dataset prefix infix suffix | prefix infix suffix
> ------------------------------------------------------------------------
> 000000010000000000000001 98% 147% 170% | 98% 132% 159%
> 000000010000000000000006 100% 340% 314% | 98% 334% 355%
> 000000010000000000000008 99% 100% 105% | 99% 99% 101%
> 16398 101% 153% 205% | 99% 148% 201%
> shakespeare.txt 100% 147% 149% | 99% 117% 118%
> mr 100% 131% 139% | 99% 112% 108%
> dickens 100% 143% 143% | 99% 103% 102%
> mozilla 100% 122% 122% | 99% 105% 106%
> nci 100% 151% 135% | 100% 135% 125%
> ooffice 99% 127% 129% | 98% 101% 102%
> osdb 102% 100% 101% | 102% 100% 99%
> reymont 101% 142% 143% | 100% 108% 108%
> samba 100% 132% 136% | 99% 109% 112%
> sao 99% 101% 100% | 99% 100% 100%
> webster 100% 132% 129% | 100% 106% 106%
> x-ray 99% 101% 100% | 90% 101% 101%
> xml 100% 147% 148% | 100% 127% 125%
>
> In general, I think the results for both patches seem clearly a win, but
> maybe patch 1 is bit better, especially on the newer (xeon) CPU. So I'd
> probably go with that one.

From my POV there are two interesting new points in your benchmarks:
1. They are more or lesss end-to-end benchmarks with whole system involved.
2. They provide per-payload breakdown

Prefix experiment is mostly related to reading from page cache and not directly connected with decompression. It's a bit strange that we observe 1% degradation in certain experiments, but I believe it's a noise.
Infix and Suffix results are correlated. We observe no impact of the patch on compressed data.

test_pglz also includes slicing by 2Kb and 8Kb. This was done to imitate toasting. But as far as I understand, in your test data payload will be inserted into toast table too, won't it? If so, I agree that patch 1 looks like a better option.

> 27 нояб. 2019 г., в 1:05, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> написал(а):
>
> Code-wise I think the patches are mostly fine, although the comments
> might need some proof-reading.
>
> 1) I wasn't really sure what a "nibble" is, but maybe it's just me and
> it's a well-known term.
I've took the word from pg_lzcompress.c comments
* The offset is in the upper nibble of T1 and in T2.
* The length is in the lower nibble of T1.
>
> 2) First byte use lower -> First byte uses lower
>
> 3) nibble contain upper -> nibble contains upper
>
> 4) to preven possible uncertanity -> to prevent possible uncertainty
>
> 5) I think we should briefly explain why memmove would be incompatible
> with pglz, it's not quite clear to me.
Here's the example
+ * Consider input: 112341234123412341234
+ * At byte 5 here ^ we have match with length 16 and
+ * offset 4. 11234M(len=16, off=4)
If we simply memmove() this 16 bytes we will produce 112341234XXXXXXXXXXXX, where series of X is 12 undefined bytes, that were at bytes [6:18].

>
> 6) I'm pretty sure the comment in the 'while (off < len)' branch will be
> badly mangled by pgindent.
I think I can just write it without line limit and then run pgindent. Will try to do it this evening. Also, I will try to write more about memmove.
>
> 7) The last change moving "copy" to the next line seems unnecessary.

Oh, looks like I had been rewording this comment, and eventually came to the same text..Yes, this change is absolutely unnecessary.

Thanks!

Best regards, Andrey Borodin.