Add zend_string_init_fast and related macros to speed up string creation #5684
Conversation
…ion for common cases
Zend/zend_API.h
Outdated
| @@ -636,6 +636,20 @@ END_EXTERN_C() | |||
| ZVAL_PSTRINGL(z, "", 0); \ | |||
| } while (0) | |||
|
|
|||
| #define ZVAL_ONE_CHAR_STRING(z, c) do { \ | |||
There was a problem hiding this comment.
what about using ZVAL_ONE_CHAR_INTERNED_STR? So that we wouldn't lose the information that it creates an interned string?
There was a problem hiding this comment.
what about using
ZVAL_ONE_CHAR_INTERNED_STR? So that we wouldn't lose the information that it creates an interned string?
but API named *_STR usually takes zend_string * as a parameter and ZVAL_ONE_CHAR_INTERNED_STRING is too long...
There was a problem hiding this comment.
Length of a macro name is not a valid argument IMHO, readability is more important than just how many characters less you will type once.
There was a problem hiding this comment.
Length of a macro name is not a valid argument IMHO, readability is more important than just how many characters less you will type once.
You're right, I prefer ZVAL_ONE_CHAR_INTERNED_STRING
There was a problem hiding this comment.
@twose Using str vs string has a semantic difference. Please see this here: #5647 (comment) Initially, I also argued against using abbreviations, but "unfortunately" they make sense in this case. :)
There was a problem hiding this comment.
@kocsismate I'm a little confused now, this macro accepts a parameter of type char, not zend_string *, so we named it *_ONE_CHAR_INTERNED_STRING instead of ONE_CHAR_INTERNED_STR, is that right?
There was a problem hiding this comment.
I'd suggest using just ZVAL_CHAR(zv, c), RETVAL_CHAR(c), RETURN_CHAR(c) ... :)
There was a problem hiding this comment.
I'd suggest using just
ZVAL_CHAR(zv, c)... :)
I'm in favor of your proposal, the reason I didn’t do this at first was that there is no char type in PHP
| #define RETVAL_EMPTY_STRING() ZVAL_EMPTY_STRING(return_value) | ||
| #define RETVAL_RES(r) ZVAL_RES(return_value, r) | ||
| #define RETVAL_ARR(r) ZVAL_ARR(return_value, r) | ||
| #define RETVAL_NULL() ZVAL_NULL(return_value) |
There was a problem hiding this comment.
Previously I also tried to do something similar, but it turned out that tabs should only be used for indentation at the beginning of the line. Otherwise, spaces are preferred.
| ZVAL_INTERNED_STR(z, ZSTR_CHAR((zend_uchar) _c)); \ | ||
| } while (0) | ||
|
|
||
| #define ZVAL_STRINGL_FAST(z, s, l) do { \ |
There was a problem hiding this comment.
Can you please add a comment to clarify when the fast implementation should be used? I believe there are some people (including me) who would appreciate a little bit more information about its applicability/use-cases.
There was a problem hiding this comment.
@kocsismate
sorry for my poor English...😢
you can add comments if possible...
|
There seems to be different things going on in this PR, the And like Maté said I don't see when I should be using this over the non fast version as just reading the name of the function I would expect the point of it is to use as much as possible and in that case I would find it more logical to just rename the current And if that's not why it's trying to achieve than I'm having trouble grasping the meaning of "fast" here. |
|
@Girgias I had the same idea as you at the beginning, but there are many |
|
for now, applying this optimization to RETVAL_STRING/RETURN_STRING is relatively easy |
|
This all depends on how generally useful this optimization is. The places where it is used right now are there because zero or one character strings are common in those context. There are of course many cases where they aren't. |
|
I think the cost of the inspection is small but the gain is big when the condition is hit |
There are only two hard things in Computer Science: |
Any better name?
BTW, I removed some spaces mixed in the tabs so the diff looks a bit strange
In theory, we can apply it to
add_next_index_string*/add_assoc_string*...