Re: Improve CRC32C performance on SSE4.2

On Wed, Feb 12, 2025 at 4:34 AM Devulapalli, Raghuveer
<raghuveer(dot)devulapalli(at)intel(dot)com> wrote:

> > On my machine that still regresses compared to master in that range (although by
> > not as much) so I still think 128 bytes is the right threshold.
>
> On my TGL, buffer sizes as small as 64 bytes see performance benefits.

Yeah, I'm guessing it's because newer chips will have better IPC. We
need to take care not to regress for hardware that's only 5-10 years
old.

Attached is v4:

0001: Same perf test, just in case
0002-4: I redid adding the implementation (without the single-block
loop and with 128-byte threshold), but split out the steps for
reference, as a model for possible ARM support in the future. These
will all be squashed on commit. The upstream code has very long lines,
even after running pgindent, so some may find that objectionable. We
could easily turn some commas into semicolons, and then it'll wrap
more nicely. I just wanted to change as little as possible for now. (I
also need to check if I need to put more license text here..)
0005: This has a fleshed-out draft commit message, but otherwise is
just the same configure/choose support as v3.

Some review comments:

1. Some of the comments that only mention SSE 4.2 in the compile- and
run-time checks need to be updated.

> > Okay, Nehalem is 17 years old, and the additional cpuid check would still work on
> > hardware 14-15 years old, so I think it's fine to bump the requirement for runtime
> > hardware support.
>
> Sounds good. I updated the runtime check to include PCLMULQDQ. New algorithm will run only on Westmere and newer CPU.

2. Unfortunately, there is another wrinkle that I failed to consider: If you
search the web for "VirtualBox pclmulqdq" you can see a few reports from not
very long ago that some hypervisors don't enable the CPUID for pclmul. I
don't know how big a problem that is in practice today, but it seems we
should actually have separate checks, with fallback. Sorry I didn't
think of this earlier.

3. Note: I left out the new test file from v3-0001. We should have
tests, but note we already have some CRC tests in
src/test/regress/sql/strings.sql -- let's put new ones there. Also,
for the longer strings we want to test, it's easier to read/verify to
use something like

SELECT crc32c(repeat('A', 128)::bytea);

Maybe it's sufficient to have 127, 128, 129 for lengths, and maybe a
couple more.

--
John Naylor
Amazon Web Services