Optimize str_repeat by divinity76 · Pull Request #23128 · php/php-src

divinity76 · 2026-08-08T06:38:52Z

The memmove got me thinking "can src and dst actually overlap?", and it turns out no:

e always points to populated data + 1
l may reach the end of populated data, but will never exceed it

memcpy is safe here, and faster (unless the compiler already spotted it in an optimization pass and replaced it internally, entirely possible)

The memmove got me thinking "can src and dst actually overlap?", and it turns out no: - e always points to populated data + 1 - l may reach the end of populated data, but will never exceed it memcpy is safe here, and faster (unless the compiler already spotted it in an optimization pass and replaced it internally, entirely possible)

Copilot

Pull request overview

This pull request optimizes PHP_FUNCTION(str_repeat) in ext/standard/string.c by replacing a memmove() with memcpy() in the exponential self-copy loop used to build the repeated string, based on the non-overlap guarantee of the source and destination ranges in that loop.

Changes:

Replace memmove(e, s, l) with memcpy(e, s, l) in the loop that expands the already-copied prefix into the remaining buffer.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

LamentXU123

Uh, from my knowledge the only difference between them is memmove check if s and e is the same, and is slower than memcpy.

In this case, I can't imagine any edge-cases because the two address shouldn't be the same. So this is sensible. But I don't know whether we want minor optimizations, because compilers are smart, and I don't sure if there is more difference between them.

David is knowledgeable about UNIX stuff. Maybe he could decide this. cc @devnexen :)

LamentXU123

From my perspective. But please wait for other's opinion.

devnexen · 2026-08-08T18:21:40Z

difference between them.

David is knowledgeable about UNIX stuff. Maybe he could decide this. cc @devnexen :)

It s nothing unix, just C :)

devnexen · 2026-08-08T18:25:55Z

@divinity76 the change is not wrong but I sense the "optimisation" part might not be remarkable across the board, depending on libc and platforms. Did you benchmark it a bit ?

To summarize: it is probably good to go, but a shift in the wording might be asked.

LamentXU123 · 2026-08-08T18:26:59Z

I assume people familiar with unix is also familiar with this as well (^_^)

@divinity76 the change is not wrong but I sense the "optimisation" part might not be remarkable across the board, depending on libc and platforms. Did you benchmark it a bit ?

I am sure some compilers are smart enough to do this already in this case.

divinity76 · 2026-08-09T10:39:40Z

@devnexen

Did you benchmark it a bit ?

No, In all likelihood It's only saving a couple of asm instructions to the tune of

; check if memcpy is safe
mov eax,src
add eax,len
cmp eax, dst
jl memcpy_is_safe

this execute in nanoseconds on modern computers, and would be very difficult to benchmark.
CPU Branch predictor makes it harder to benchmark
The compiler may catch it at compile-time, in which case it's equally fast and impossible to benchmark.. ! my gcc 13.3.0 with -O2 did not catch it at compile-time, I checked:

- 5205fc: e8 2f 1f ce ff        call   202530 <memcpy@plt>
+ 5205fc: e8 cf 18 ce ff        call   201ed0 <memmove@plt>

To summarize: it is probably good to go, but a shift in the wording might be asked.

Suggestions? Would you prefer str_repeat nitpicking ?

devnexen · 2026-08-09T16:01:50Z

x86-64 (glibc) builds both from the same file, strong_alias (MEMMOVE_SYMBOL (__memmove, unaligned), MEMCPY_SYMBOL (__memcpy, unaligned)), so memcpy is the very same address, nothing saved. aarch64 shares the small-copy code up to 128 bytes, no overlap test at all, and beyond that memmove does sub / cbz / cmp / b.hs L(copy_long) into memcpy s own long path, so 4 instructions. musl just forwards, if ((uintptr_t)s-(uintptr_t)d-n <= -2*n) return memcpy(d, s, n);. And where memmove is a plainer loop instead, it is not a fixed branch anymore, it grows with l. And I m just talking about Linux here..

this is what I was trying to say with the "optimisation" part might not be remarkable across the board. Again not against at all even though I would say, let is give it time to someone to chime in just in case there is some context we re missing.

iliaal

Looks fine to me, although I doubt there is any performance to be gained here, more of a correctness refactor I'd say

divinity76 · 2026-09-17T07:02:01Z

I doubt there is any performance to be gained here, more of a correctness refactor I'd say

Lets compare the glibc 2.42 implementation of memmove:

rettype
inhibit_loop_to_libcall
MEMMOVE (a1const void *a1, a2const void *a2, size_t len)
{
  unsigned long int dstp = (long int) dest;
  unsigned long int srcp = (long int) src;

  /* This test makes the forward copying code be used whenever possible.
     Reduces the working set.  */
  if (dstp - srcp >= len)	/* *Unsigned* compare!  */
    {
      /* Copy from the beginning to the end.  */

#if MEMCPY_OK_FOR_FWD_MEMMOVE
      dest = memcpy (dest, src, len);
#else
      /* If there not too few bytes to copy, use word copy.  */
      if (len >= OP_T_THRES)
	{

with it's memcpy counterpart:

void *
MEMCPY (void *dstpp, const void *srcpp, size_t len)
{
  unsigned long int dstp = (long int) dstpp;
  unsigned long int srcp = (long int) srcpp;

  /* Copy from the beginning to the end.  */

  /* If there not too few bytes to copy, use word copy.  */
  if (len >= OP_T_THRES)
    {

Memcpy saves the CPU from having to do the dstp - srcp >= len calculation of memmove here.
Also saves a conditional branch.

laruence · 2026-09-17T09:38:33Z

I doubt there is any performance to be gained here, more of a correctness refactor I'd say

Lets compare the glibc 2.42 implementation of memmove:

rettype
inhibit_loop_to_libcall
MEMMOVE (a1const void *a1, a2const void *a2, size_t len)
{
  unsigned long int dstp = (long int) dest;
  unsigned long int srcp = (long int) src;

  /* This test makes the forward copying code be used whenever possible.
     Reduces the working set.  */
  if (dstp - srcp >= len)	/* *Unsigned* compare!  */
    {
      /* Copy from the beginning to the end.  */

#if MEMCPY_OK_FOR_FWD_MEMMOVE
      dest = memcpy (dest, src, len);
#else
      /* If there not too few bytes to copy, use word copy.  */
      if (len >= OP_T_THRES)
	{

with it's memcpy counterpart:

void *
MEMCPY (void *dstpp, const void *srcpp, size_t len)
{
  unsigned long int dstp = (long int) dstpp;
  unsigned long int srcp = (long int) srcpp;

  /* Copy from the beginning to the end.  */

  /* If there not too few bytes to copy, use word copy.  */
  if (len >= OP_T_THRES)
    {

Memcpy saves the CPU from having to do the dstp - srcp >= len calculation of memmove here. Also saves a conditional branch.

Actullay， modern x86-64 glibc doesn't actually build them into libc for this code path. The implementation that runs on x86-64 is sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S,

I don't think this change should be called an "Optimization" :) it's really tiny — anyway, the change itself is safe. I have only one suggestion: add a comment pinning the non-overlap invariant, in case of somebody in the future change this behavior, then silently turn this into UB :):

/* src [s, s+l) and dst [e, e+l) cannot overlap: l <= e-s */
memcpy(e, s, l);

divinity76 · 2026-09-17T10:37:07Z

Actullay， modern x86-64 glibc doesn't actually build them into libc for this code path. The implementation that runs on x86-64 is sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S,

Neat, so, as pointed out by both devnexen and laruence , there is no change on x86-64 glibc.

But in ARM64/aarch64 glibc: https://github.com/bminor/glibc/blob/master/sysdeps/aarch64/memcpy.S

in memmove

ENTRY (MEMMOVE)
(...)
L(move_long):
	/* Only use backward copy if there is an overlap.  */
	sub	tmp1, dstin, src
	cbz	tmp1, L(move0)
	cmp	tmp1, count
	b.hs	L(copy_long)

we know it will jump to copy_long every time it reach move_long in our C code, but still, it saves the ARM cpu from having to do

	sub	tmp1, dstin, src
	cbz	tmp1, L(move0)
	cmp	tmp1, count
	b.hs	L(copy_long)

on arm64/aarch64 glibc.

I don't think this change should be called an "Optimization" :)

hmhmhm...

it's really tiny

agreed.

I have only one suggestion: add a comment pinning the non-overlap invariant, in case of somebody in the future change this behavior, then silently turn this into UB :)

That sounds good.

Not important, but can you go to https://github.com/php/php-src/pull/23128/changes and add a comment on the memcpy line including the content

```suggestion
                        /* src [s, s+l) and dst [e, e+l) cannot overlap: l <= e-s */
						memcpy(e, s, l);
``` <github comment codeblock cannot end with ```>

divinity76 · 2026-09-18T13:02:36Z


 		while (e<ee) {
 			l = (e-s) < (ee-e) ? (e-s) : (ee-e);
-			memmove(e, s, l);


can you try using ```suggestion
like the codeblock language is "suggestion" ?

divinity76 · 2026-09-18T17:51:10Z


 		while (e<ee) {
 			l = (e-s) < (ee-e) ? (e-s) : (ee-e);
-			memmove(e, s, l);


very close! the last line got 3 indentation levels too many

Co-authored-by: Xinchen Hui <laruence@gmail.com>

Copilot AI lite review requested due to automatic review settings August 8, 2026 06:38

divinity76 requested a review from bukka as a code owner August 8, 2026 06:38

github-actions Bot added the Extension: standard label Aug 8, 2026

Copilot started reviewing on behalf of divinity76 August 8, 2026 06:39 View session

Copilot AI reviewed Aug 8, 2026

View reviewed changes

LamentXU123 reviewed Aug 8, 2026

View reviewed changes

LamentXU123 approved these changes Aug 8, 2026

View reviewed changes

LamentXU123 requested review from arnaud-lb and iliaal September 12, 2026 14:16

iliaal approved these changes Sep 15, 2026

View reviewed changes

divinity76 commented Sep 18, 2026

View reviewed changes

laruence reviewed Sep 19, 2026

View reviewed changes

Comment thread ext/standard/string.c

comment pinning the non-overlap invariant

5f64b3c

Co-authored-by: Xinchen Hui <laruence@gmail.com>

Conversation

divinity76 commented Aug 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

LamentXU123 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LamentXU123 left a comment

Choose a reason for hiding this comment

Uh oh!

devnexen commented Aug 8, 2026

Uh oh!

devnexen commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LamentXU123 commented Aug 8, 2026

Uh oh!

divinity76 commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devnexen commented Aug 9, 2026

Uh oh!

iliaal left a comment

Choose a reason for hiding this comment

Uh oh!

divinity76 commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laruence commented Sep 17, 2026

Uh oh!

divinity76 commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

divinity76 Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

divinity76 Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LamentXU123 left a comment •

edited

Loading

devnexen commented Aug 8, 2026 •

edited

Loading

divinity76 commented Aug 9, 2026 •

edited

Loading

divinity76 commented Sep 17, 2026 •

edited

Loading

divinity76 commented Sep 17, 2026 •

edited

Loading