public inbox for [email protected]
help / color / mirror / Atom feedFrom: Oleg Tselebrovskiy <[email protected]>
To: Andrew Kim <[email protected]>
Cc: John Naylor <[email protected]>
Cc: [email protected]
Subject: Re: Proposal for enabling auto-vectorization for checksum calculations
Date: Fri, 17 Oct 2025 17:53:43 +0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAK64mneN20+sW5WhV+r7hMVo4Rd0z11B6=3L039rWMt1wK3nPg@mail.gmail.com>
References: <[email protected]>
<CANWCAZYZQw-nzTXbx3Bk332VtY9_D7ksDsuMZ0A-iDZ53yG7Ng@mail.gmail.com>
<CAK64mnfeWLBRbMfnOsag0vGTDnT84KJzpuei40nG0OHyw4SESw@mail.gmail.com>
<CANWCAZa1b2rcvoK657SmcKwh2P2cgASQ1D-0JPj5d3LbfaAVgA@mail.gmail.com>
<CAK64mneN20+sW5WhV+r7hMVo4Rd0z11B6=3L039rWMt1wK3nPg@mail.gmail.com>
Greetings!
I've also tried to use AVX2 to speedup checksums and I've found your
approach quite interesting
But I see some issues with v6 patch
1) checksum.c was moved to src/port, but special meson rules are left in
src/backend/storage/page/meson.build. As a result, assembly code for
moved src/port/checksum.c doesn't use -funroll-loops and
-ftree-vectorize (latter isn't probably needed now, due to the nature of
the patch). The same is true for src/port/Makefile, there are no
instructions to use CFLAGS_UNROLL_LOOPS and CFLAGS_VECTORIZE
2) checksum.c was moved to src/port, but checksum.h and checksum_impl.h
are left in src/include/storage. I think they both should be moved to
src/include/port, as John Naylor suggested in his review of v5
3) checksum_impl.h now doesn't provide any code, so including it in
external programs won't allow checksum calculation. I think that all
code should be in checksum_impl.h, and external programs could just
define USE_AVX2_WITH_RUNTIME_CHECK (probably using similar checks as we
are) to use AVX2 implementation. If not - then they will default to
default realisation
4) I don't understand why do we need to check for AVX2 intrinsics if we
don't use those in code (at least I don't see them directly)? As in
review of v5, couldn't test functions in configure, config/c-compiler.m4
and ./meson.build just be {return 0;} or {return 1;}?
5) Why do we need both src/backend/storage/page/checksum.c and
src/port/checksum.c?
6)
> +/* Function declarations for ISA-specific implementations */
> +uint32 pg_checksum_block_default(const PGChecksummablePage *page);
> +#ifdef USE_AVX2_WITH_RUNTIME_CHECK
> +uint32 pg_checksum_block_avx2(const PGChecksummablePage *page);
> +#endif
What is "ISA-specific implementations" in this comment? Maybe I'm just
not familiar with the term? Or is it an artifact from macro
implementation?
7) Why remove all comments from code of pg_checksum_block_default? I
could understand if you just removed comments from
pg_checksum_block_avx2, since it just duplicates code (though I
personally would leave all the comments even when duplicating code), but
I don't understand removing comments from pg_checksum_block_default
8) It might be a personal taste, but pg_checksum_block_dispatch looks
more like "choose" function from src/port/pg_crc32c_sse42_choose.c and
alike. "dispatch" from src/include/port/pg_crc32c looks a little
different - we don't choose function pointer once there, we choose
between inlined computation and calling a function with runtime check.
So I'd suggest changing name of pg_checksum_block_dispatch to
pg_checksum_block_choose
Other than those, I think the core of this patch is good
Oleg Tselebrovskiy, PostgresPro
view thread (35+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Proposal for enabling auto-vectorization for checksum calculations
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox