Re: Optimizing COPY with SIMD

Thanks for the review and feedback!

On Mon, Jun 3, 2024 at 10:56 AM Nathan Bossart <nathandbossart(at)gmail(dot)com>
wrote:

> > -/*
> > - * Send text representation of one attribute, with conversion and
> escaping
> > - */
> > #define DUMPSOFAR() \
>
> IIUC this comment was meant to describe the CopyAttributeOutText() function
> just below this macro. When the macro was added in commit 0a5fdb0 from
> 2006, the comment became detached from the function. Maybe we should just
> move it back down below the macro.
>

Ah, that makes sense -- done.

> > +/*
> > + * Send text representation of one attribute, with conversion and
> CSV-style
> > + * escaping. This variant uses SIMD instructions to optimize
> processing, but
> > + * we can only use this approach when encoding_embeds_ascii if false.
> > + */
>
> nitpick: Can we add a few words about why using SIMD instructions when
> encoding_embeds_ascii is true is difficult? I don't dispute that it is
> complex and/or not worth the effort, but it's not clear to me why that's
> the case just from reading the patch.
>

Sounds good.

> > +static void
> > +CopyAttributeOutCSVFast(CopyToState cstate, const char *ptr,
> > + bool use_quote)
>
> nitpick: Can we add "vector" or "simd" to the name instead of "fast"? IMHO
> it's better to be more descriptive.
>

Sure, done.

Attached is a revised patch series, that incorporates the feedback above
and makes two additional changes:

* Add some regression tests to cover COPY behavior with octal and hex
escape sequences
* Optimize the COPY TO text (non-CSV) code path (CopyAttributeOutText()).

In CopyAttributeOutText(), I refactored some code into a helper function to
reduce code duplication, on the theory that field delimiters and escape
sequences are rare, so we don't mind taking a function call in those cases.

We could go further and use the same code to handle both the tail of the
string in the vectorized case and the entire string in the non-vectorized
case, but I didn't bother with that -- as written, it would require taking
an unnecessary strlen() of the input string in the non-vectorized case.

Performance for COPY TO in text (non-CSV) mode:

===
master

Benchmark 1: ./psql -f
/Users/neilconway/copy-out-bench-text-long-strings.sql
Time (mean ± σ): 1.240 s ± 0.013 s [User: 0.001 s, System: 0.000
s]
Range (min … max): 1.220 s … 1.256 s 10 runs

Benchmark 1: ./psql -f /Users/neilconway/copy-out-bench-text-short.sql
Time (mean ± σ): 522.3 ms ± 11.3 ms [User: 1.2 ms, System: 0.0 ms]
Range (min … max): 512.0 ms … 544.3 ms 10 runs

master + SIMD patches:

Benchmark 1: ./psql -f
/Users/neilconway/copy-out-bench-text-long-strings.sql
Time (mean ± σ): 867.6 ms ± 12.7 ms [User: 1.2 ms, System: 0.0 ms]
Range (min … max): 842.1 ms … 891.6 ms 10 runs

Benchmark 1: ./psql -f /Users/neilconway/copy-out-bench-text-short.sql
Time (mean ± σ): 536.7 ms ± 10.9 ms [User: 1.2 ms, System: 0.0 ms]
Range (min … max): 530.1 ms … 566.8 ms 10 runs
===

Looks like there is a slight regression for short attribute values, but I
think the tradeoff is a net win.

I'm going to take a look at applying similar ideas to COPY FROM next.

Neil