Обсуждение: Proposal for enabling auto-vectorization for checksum calculations

Поиск

Список

Период

Сортировка

Proposal for enabling auto-vectorization for checksum calculations

От

Matthew Sterrett

Дата:

05 апреля, 01:03:43

Hello,
This patch enables more compiler autovectorization for the checksum
calculations.
This code is particularly well suited for autovectorization, so just
adding pg_attribute_target and some simple dynamic dispatch logic we
can get improved vectorization.
This gives about a 2x speedup in a synthetic benchmark for
pg_checksum, which is also included as a seperate patch file.

Additionally, another 2x performance increase in the synthetic
benchmark with AVX2 can be obtained if N_SUMS was changed to 64.
However, this would change the results of the checksum. This isn't
included in this patch, but I think it is worth considering for the
future

One additional factor, without explicitly passing some optimization
flag like -O2 the makefile build won't autovectorize any of the code.
However, the meson based build does this automatically.

Вложения

Re: Proposal for enabling auto-vectorization for checksum calculations

От

Matthew Sterrett

Дата:

08 мая, 02:57:06

Hello! I'm still trying to figure out those CI failures, I just wanted
to update things.

From my testing, with this patch repeatedly disabling/enabling
checksums is about 12.4% on an approximately 15 GB database.

By the way, I'd love it if anyone could help me figure out how to
replicate a CI failure in the Cirrus CI.
I haven't been able to figure out how to test CI runs locally, does
anyone know a good method to do that?

Re: Proposal for enabling auto-vectorization for checksum calculations

От

Stepan Neretin

Дата:

10 мая, 14:01:06

On Thu, May 8, 2025 at 6:57 AM Matthew Sterrett <matthewsterrett2@gmail.com> wrote:

Hello! I'm still trying to figure out those CI failures, I just wanted
to update things.

From my testing, with this patch repeatedly disabling/enabling
checksums is about 12.4% on an approximately 15 GB database.

By the way, I'd love it if anyone could help me figure out how to
replicate a CI failure in the Cirrus CI.
I haven't been able to figure out how to test CI runs locally, does
anyone know a good method to do that?

Hi Matthew,

Thanks for the patch!

I ran some timing tests:

(without avx2)

Time: 4034.351 ms
SELECT drive_pg_checksum(512);

(with avx2)


Time: 3559.076 ms
SELECT drive_pg_checksum(512);

Also attached two patches that should fix the CI issues.

Best,

Stepan Neretin

Re: Proposal for enabling auto-vectorization for checksum calculations

От

Matthew Sterrett

Дата:

20 мая, 02:54:09

Hello! Thanks for helping me with this.
I'm still trying to figure out what is going on with the Bookworm test
failures. I'm pretty sure this patchset should resolve all the issues
with the macOS build, but I don't think it will help the linux
failures unfortunately.

On Sat, May 10, 2025 at 4:02 AM Stepan Neretin <slpmcf@gmail.com> wrote:
>
>
>
> On Sat, May 10, 2025 at 6:01 PM Stepan Neretin <slpmcf@gmail.com> wrote:
>>
>>
>>
>> On Thu, May 8, 2025 at 6:57 AM Matthew Sterrett <matthewsterrett2@gmail.com> wrote:
>>>
>>> Hello! I'm still trying to figure out those CI failures, I just wanted
>>> to update things.
>>>
>>> From my testing, with this patch repeatedly disabling/enabling
>>> checksums is about 12.4% on an approximately 15 GB database.
>>>
>>> By the way, I'd love it if anyone could help me figure out how to
>>> replicate a CI failure in the Cirrus CI.
>>> I haven't been able to figure out how to test CI runs locally, does
>>> anyone know a good method to do that?
>>>
>>>
>>
>> Hi Matthew,
>>
>> Thanks for the patch!
>>
>> I ran some timing tests:
>>
>> (without avx2)
>>
>> Time: 4034.351 ms
>> SELECT drive_pg_checksum(512);
>>
>> (with avx2)
>>
>> Time: 3559.076 ms
>> SELECT drive_pg_checksum(512);
>>
>> Also attached two patches that should fix the CI issues.
>>
>> Best,
>>
>> Stepan Neretin
>>
>>
>>
>
> Oops, forgot to attach patches :)
>
> Best,
>
> Stepan Neretin
>
>

Вложения

Re: Proposal for enabling auto-vectorization for checksum calculations

От

Nazir Bilal Yavuz

Дата:

20 мая, 17:42:27

Hi,

On Tue, 20 May 2025 at 02:54, Matthew Sterrett
<matthewsterrett2@gmail.com> wrote:
>
> Hello! Thanks for helping me with this.
> I'm still trying to figure out what is going on with the Bookworm test
> failures. I'm pretty sure this patchset should resolve all the issues
> with the macOS build, but I don't think it will help the linux
> failures unfortunately.

You can see the failure at the artifacts ->
'log/tmp_install/log/install.log' file on the CI web page [1].

If you want to replicate that on your local:

$ ./configure --with-llvm CLANG="ccache clang-16"
$ make -s -j8 world-bin
$ make -j8 check-world

should be enough. I was able to replicate it with these commands. I
hope these help.

[1] https://cirrus-ci.com/task/4834162550505472

--
Regards,
Nazir Bilal Yavuz
Microsoft

Re: Proposal for enabling auto-vectorization for checksum calculations

От

Matthew Sterrett

Дата:

23 мая, 00:54:28

> You can see the failure at the artifacts ->
> 'log/tmp_install/log/install.log' file on the CI web page [1].
>
> If you want to replicate that on your local:
>
> $ ./configure --with-llvm CLANG="ccache clang-16"
> $ make -s -j8 world-bin
> $ make -j8 check-world
>
> should be enough. I was able to replicate it with these commands. I
> hope these help.
Thanks so much for helping me figure this out!

Okay, I've determined that versions of LLVM/Clang before 19 crash when
compiling this patch for some reason; it seems that both make
check-world and make install will crash with the affected LLVM
versions.
Unfortunately, what matters seems to be the version of the linker/LTO
optimizer, which I don't think we can check at compile time.
I added a check for Clang>=19 which works at preventing the crash on my system.
I think it's possible some unusual combination of clang/LLVM might
still crash during the build, but I think this is a reasonable
solution

On Tue, Sep 23, 2025 at 11:32 PM John Naylor johncnaylorls@gmail.com wrote:

There is no attachment in this thread, so it's not showing up in the
commitfest entry (which will need to be moved to next open
commitfest), so it's not getting CI testing:

https://commitfest.postgresql.org/patch/5726/

Note that the whole series must be attached in a single email, or it
won't get automated testing.

Thanks, John. I see the issue now — I’ll attach the entire patch
series in a single email so it shows up properly in the commitfest and
gets CI coverage.

Please find attached v6 of the patchset, updated per your feedback.

Best regards,
Andrew Kim


On Tue, Sep 23, 2025 at 11:32 PM John Naylor <johncnaylorls@gmail.com> wrote:
>
> On Thu, Sep 11, 2025 at 1:55 PM root <tenistarkim@gmail.com> wrote:
> > Thanks for the feedback. This is v5 of the patchset, updated following your comments:
> >
> > - Moved the function pointer definitions out of common headers and
> >   into src/port, consistent with existing practice.
>
> There is no attachment in this thread, so it's not showing up in the
> commitfest entry (which will need to be moved to next open
> commitfest), so it's not getting CI testing:
>
> https://commitfest.postgresql.org/patch/5726/
>
> Note that the whole series must be attached in a single email, or it
> won't get automated testing.
>
> --
> John Naylor
> Amazon Web Services

Вложения

Re: Proposal for enabling auto-vectorization for checksum calculations

От

John Naylor

Дата:

02 октября, 08:25:59

On Thu, Sep 25, 2025 at 4:50 AM Andrew Kim <tenistarkim@gmail.com> wrote:
>
> Thanks, John. I see the issue now — I’ll attach the entire patch
> series in a single email so it shows up properly in the commitfest and
> gets CI coverage.

It's still picking up v4, and the archive link doesn't show any
further replies. Something must have happened with the email
threading, since you weren't on the thread at first.  Please create an
account and edit the entry to point to a more recent message ID:

https://commitfest.postgresql.org/patch/5726/

> Please find attached v6 of the patchset, updated per your feedback.

Thanks. (BTW, we discourage top-posting and prefer to cut to size and
use inline responses)

This is not a complete review, but some architectural thoughts and
some things I've noticed.

The top of the checksum_impl.h has this:

 * This file exists for the benefit of external programs that may wish to
 * check Postgres page checksums.  They can #include this to get the code
 * referenced by storage/checksum.h.  (Note: you may need to redefine
 * Assert() as empty to compile this successfully externally.)

It's going to be a bit tricky to preserve this ability while allowing
the core server and client programs to dispatch to a specialized
implementation, but we should at least try. That means keeping
pg_checksum_block() and pg_checksum_page() where they live now.

I think a good first refactoring patch would be to move
src/backend/storage/checksum.c (which your patch doesn't even touch)
to src/port (and src/include/storage/checksum.h to src/include/port)
and have all callers use that. With that, I imagine only that
checksum.c file would include checksum_impl.h.

If that poses a problem, let us know -- we may have to further juggle
things. If that works without issue, we can proceed with the
specialization. On that, just a few things to note here, although the
next patch doesn't need to worry about any of this yet:

+    #if defined(__has_attribute) && __has_attribute (target)
+    __attribute__((target("avx2")))
+    #endif
+    static int avx2_test(void)
+    {
+      const char buf@<:@sizeof(__m256i)@:>@;
+      __m256i accum = _mm256_loadu_si256((const __m256i *) buf);
+   accum = _mm256_add_epi32(accum, accum);
+      int result = _mm256_extract_epi32(accum, 0);
+      return (int) result;
+    }],

If we're just testing if the target works, we can just use an empty
function, right?

+#define PG_DECLARE_CHECKSUM_ISA(ISANAME) \
+uint32 \
+pg_checksum_block_##ISANAME(const PGChecksummablePage *page);
+
+#define PG_DEFINE_CHECKSUM_ISA(ISANAME) \
+pg_attribute_target(#ISANAME) \
+uint32 pg_checksum_block_##ISANAME(const PGChecksummablePage *page) \

I find this hard to read compared to just using the actual name.

+avx2_available(void)
+{
+#if defined (USE_AVX2_WITH_RUNTIME_CHECK) && defined(__x86_64__)

Why guard on __x86_64__?

+PG_DEFINE_CHECKSUM_ISA(default)
+{
+ uint32 sums[N_SUMS], result = 0;
+ uint32 i, j;
[...]

+#ifdef USE_AVX2_WITH_RUNTIME_CHECK
+PG_DEFINE_CHECKSUM_ISA(avx2)
+{
+ uint32 sums[N_SUMS], result = 0;
+ uint32 i, j;
[...]

With the single src/port file idea above, these would just do "return
pg_checksum_block()" (or pg_checksum_page, whichever makes more
sense).

+ if (avx2_available())
+ {
+ /* optional: patch pointer so next call goes directly */
+ pg_checksum_block = pg_checksum_block_avx2;
+ return pg_checksum_block_avx2(page);
+ }

Not sure what your referring to here by "patching" the pointer, but it
sounds dangerous. Besides, the cost of indirection is basically zero
for multi-kilobyte inputs, so there is not even any motivation to
consider doing differently.

--
John Naylor
Amazon Web Services

Вход в личный кабинет

Восстановление пароля

Подтверждение аккаунта

Изменение пароля

Обсуждение: Proposal for enabling auto-vectorization for checksum calculations

Вложения

Вложения

Вложения

Вложения