| From: | Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com> |
|---|---|
| To: | ChenhuiMo <chenhuimo(dot)mch(at)qq(dot)com> |
| Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: [PATCH] Speed up repeat() for larger counts |
| Date: | 2026-07-27 07:31:37 |
| Message-ID: | CAM2+6=WTyhMFtBc4YJLAuhH8+NFjaieGjcMZFWFMzeansmDOcA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hello ChenhuiMo,
This is a really interesting optimization — doing memcpy() exponentially is
a great idea for larger counts. My main use case for repeat() is generating
large files and padding, where count is usually large, so this change helps
exactly the workload I care about.
That said, I'm not entirely sure how common this usage pattern is in
general production workloads, so I was initially unsure whether the added
complexity is worth it. Setting that aside, the performance gain for short
strings repeated a large number of times is real, so I'm fine with getting
this in.
A few things worth looking at:
1. The magic number 8 is unexplained. The naive-loop fallback threshold
seems quite low — should it be closer to a few dozen, say 64? I benchmarked
a range of thresholds and counts: the doubling approach turns out to beat
the naive loop even at very small counts (as low as 2–3), so 8 already
looks like a reasonable cutoff; raising it to 64 would likely make counts
in the 8–64 range slower, not faster. Regardless of the exact value chosen,
the constant needs a comment explaining why it was picked.
2. Because of the exponential growth, CHECK_FOR_INTERRUPTS() granularity
drops off logarithmically; the later doubling steps can copy hundreds of MB
in a single memcpy that can't be interrupted partway through. Would it help
to cap the chunk size, say at 64–100MB? I tested this: capping adds only a
handful of extra memcpy calls even for very large repeats, and the overhead
was within noise; so it looks like a cheap way to bound worst-case
interrupt latency.
3. The doubling logic itself has no comment. I think a comment there is a
must; the self-overlapping-safe memcpy trick isn't obvious at a glance.
4. It's worth adding tests that exercise count at the boundary values to
guard against regressions if this logic gets refactored later.
5. Cosmetic: add a blank line between the variable declaration and the
first statement in the new block.
Thanks
On Sun, Jul 26, 2026 at 11:00 AM ChenhuiMo <chenhuimo(dot)mch(at)qq(dot)com> wrote:
> Hi all,
>
> This patch optimizes repeat(text, int).
>
> The current implementation constructs the result by copying the source text
> once per repetition. For larger repeat counts, that leads to many small
> memcpy() calls. This patch keeps the existing linear path for small
> repeat counts, and uses a doubling strategy for larger ones by repeatedly
> copying the already-generated prefix of the destination buffer.
>
> In local testing, this reduced total runtime in a small repeat()-focused
> microbenchmark, with the largest gains appearing in medium and large repeat
> count cases. Small-count cases did not show obvious regressions after
> keeping the original path below a small cutoff.
>
> The patch is attached.
>
> Thanks for your time, and any feedback would be appreciated.
>
> Best regards,
> Chenhui Mo
--
*Jeevan Chalke*
*Senior Principal Engineer, Engineering Manager*
*Product Development*
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Michael Paquier | 2026-07-27 07:47:00 | Re: [PATCH v1] Fix propagation of indimmediate flag in index_create_copy |
| Previous Message | Richard Guo | 2026-07-27 07:07:56 | Re: remove_useless_joins vs. bug #19560 |