Skip to content

ext/standard: Optimize pack() by avoiding redundant padding in pack()… - #24133

Merged
LamentXU123 merged 2 commits into
php:masterfrom
LamentXU123:opt20261005
Oct 5, 2026
Merged

LamentXU123 merged 2 commits into
php:masterfrom
LamentXU123:opt20261005

Conversation

@LamentXU123

Copy link
Copy Markdown
Member

… string formats

Fill only the trailling padding instead of filling the whole field. This avoids immediate overwrites and is more effective.

Comment thread UPGRADING Outdated
. Improved performance of array_splice() when inserting without removing
elements.
. Improved performance of str_rot13().
. Improved performance of pack() for string formats (a, A and Z).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just benchmarked it, the win is mainly large strings, it s kind of the same otherwise.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it make sense. The improvement should be directly propotional to the length of the string.

Comment thread ext/standard/pack.c
(ZSTR_LEN(str) < arg_cp) ? ZSTR_LEN(str) : arg_cp);
arg_cp = MIN(ZSTR_LEN(str), arg_cp);
if (arg_cp < arg) {
memset(&ZSTR_VAL(output)[outputpos + arg_cp],

@devnexen devnexen Oct 5, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the compiler might be able to optimise the case without the guard above. But that s really nitpick of course, definitely not a blocker.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With gcc -O2

569642: cmp    rcx,rdx             
569645: jae    5696a3               
569647: sub    rdx,rcx             
; ...
569685: call   201490 <memset@plt>
; ...
5696a3: lea    rdi,[r12+r11*1]       
5696a7: lea    rsi,[r10+0x18]
5696ab: mov    rdx,rcx
; ...
5696b8: call   202570 <memcpy@plt>

if we remove the if branch:

569654: cmovbe rax,rdx              
569658: movsxd rdx,r9d              
; ...
569665: sub    rdx,rax              
; ...
56968c: call   201490 <memset@plt>   
; ...
5696a7: call   202570 <memcpy@plt>

So in this case it is useful.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes indeed.

@LamentXU123
LamentXU123 merged commit 8ca3fe5 into php:master Oct 5, 2026
1 check passed
@LamentXU123

Copy link
Copy Markdown
Member Author

Thanks!

@LamentXU123
LamentXU123 deleted the opt20261005 branch October 5, 2026 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants