-
Notifications
You must be signed in to change notification settings - Fork 8k
Add a 1-char fastpath to implode()
#19276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1-char bench on an i7-4790:
Curiously a large difference with your measurement, did you benchmark a release build? 2-char glue is slightly slower:
|
I checked again and
Given the number of occurrences of 1 char glue as written in the description, I think this fast path would still worth it? Description updated with the non-debug numbers for 1-char glue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for me
9696231
to
19a6b57
Compare
|
||
cptr -= ZSTR_LEN(glue); | ||
memcpy(cptr, ZSTR_VAL(glue), ZSTR_LEN(glue)); | ||
if (ZSTR_LEN(glue) == 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in loop, is ZSTR_LEN(glue)
guaranteed to be evaluated only once?
What about moving the condition outside the loop, ie. "duplicating" the loop body?
Also what about the case when the glue is empty string? (we use that strongly in https://github.com/atk4/ui/blob/6.0.0/src/HtmlTemplate.php#L570 - with 10k calls per webpage request easily)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in loop, is ZSTR_LEN(glue) guaranteed to be evaluated only once?
No. But chances are that it doesn't matter if this is in a variable or not because you'll need a load from a spill slot in the variable case most likely.
I think it's worth adding a 1 char fast path for
implode()
. A quick search on Github reveals the following stats:/implode\(\s*["'][^"']{1}["']\s*,\s*/ language:PHP
reveals 1.4 million occurrences/implode\(\s*["'][^"']{2,}["']\s*,\s*/ language:PHP
reveals 1.1 million occurrencesHere is the benchmark code:
And the results:
No change for multi char glue, but a nice improved performance for single char glue especially one big arrays.