MIME-Version: 1.0
Date: Fri, 17 Oct 2025 17:53:43 +0700
From: Oleg Tselebrovskiy <o.tselebrovskiy@postgrespro.ru>
To: Andrew Kim <tenistarkim@gmail.com>
Cc: John Naylor <johncnaylorls@gmail.com>,
 pgsql-hackers@lists.postgresql.org
Subject: Re: Proposal for enabling auto-vectorization for checksum
 calculations
In-Reply-To: 
 <CAK64mneN20+sW5WhV+r7hMVo4Rd0z11B6=3L039rWMt1wK3nPg@mail.gmail.com>
References: <20250911054220.3784-1-root@ip-172-31-36-228.ec2.internal>
 <CANWCAZYZQw-nzTXbx3Bk332VtY9_D7ksDsuMZ0A-iDZ53yG7Ng@mail.gmail.com>
 <CAK64mnfeWLBRbMfnOsag0vGTDnT84KJzpuei40nG0OHyw4SESw@mail.gmail.com>
 <CANWCAZa1b2rcvoK657SmcKwh2P2cgASQ1D-0JPj5d3LbfaAVgA@mail.gmail.com>
 <CAK64mneN20+sW5WhV+r7hMVo4Rd0z11B6=3L039rWMt1wK3nPg@mail.gmail.com>
Message-ID: <d023881ae837e0e116d8645c89e319da@postgrespro.ru>
Organization: Postgres Pro
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/d023881ae837e0e116d8645c89e319da%40postgrespro.ru>
Precedence: bulk

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