Optimize PHP html_entity_decode function #18092
Conversation
…mize scanning for '&' and ';' using memchr Use memcpy instead of character-by-character copying language
ArtUkrainskiy
force-pushed
the
html_entity_decode/improve-memchr
branch
from
d166abe to
66f5709
Compare
ext/standard/html.c
Outdated
| char *output_ptr = ZSTR_VAL(output); | ||
| int doctype = flags & ENT_HTML_DOC_TYPE_MASK; | ||
|
|
||
| assert(*input_end == '\0'); |
There was a problem hiding this comment.
If you change the input parameter to a zend_string* you'd be guaranteed this. Moreover, please use ZEND_ASSERT() instead.
ext/standard/html.c
Outdated
|
|
||
| unsigned code = 0, code2 = 0; | ||
| const char *entity_end_ptr = NULL; | ||
| int valid_entity = 1; |
There was a problem hiding this comment.
| int valid_entity = 1; | |
| bool valid_entity = true; |
|
@Girgias are you going to review the logic as well? Just checking if I should look into this or if you are happy to handle it all? |
Please do review the logic, I only had a cursory glance :) |
|
Ok I will check it out next week if no one is quicker. |
fix logic
ArtUkrainskiy
force-pushed
the
html_entity_decode/improve-memchr
branch
from
9b3e96d to
f093c30
Compare
|
Nice idea @ArtUkrainskiy :-) 👍 |
There was a problem hiding this comment.
I think it looks reasonable except that introduction of valid_entity boolean and checking that everywhere which doesn't look like performance optimization to me. I understand that it was probably meant to make code more readable but not sure if it's worth it in this case. Might be worth to check if it has any impact.
ext/standard/html.c
Outdated
| } else { | ||
| const size_t name_len = semi_colon_ptr - name_start; | ||
| if (name_len == 0) { | ||
| valid_entity = false; |
There was a problem hiding this comment.
I think those extra stores and extra check of valid_entity might be actually slower. Could you try to put beck to goto and measure it.
ext/standard/html.c
Outdated
| } | ||
| } | ||
| } | ||
|
|
||
| assert(*next == ';'); | ||
| /* If entity_end_ptr is not found or does not point to ';', consider the entity invalid */ | ||
| if (!valid_entity || entity_end_ptr == NULL || *entity_end_ptr != ';') { |
There was a problem hiding this comment.
it cannot be ; so that check is pointless - this is checked in resolve_named_entity_html as well as in process_numeric_entity.
As I mentioned above, I think we should keep the goto.
There was a problem hiding this comment.
Hi, @bukka ! Thanks for the code review! I removed the invalid_entity flag, and that actually resulted in a measurable performance gain - you can see the comparison below. I also optimized the condition you mentioned.
Commit: 5f8363b
------------------------------------------------------------------------------
| Test | flag avg(ns) | goto avg(ns) | diff(%) |
------------------------------------------------------------------------------
| 4k & | 21295 | 21148 | 0.27% |
------------------------------------------------------------------------------
| only entities | 10298 | 10032 | 2.80% |
------------------------------------------------------------------------------
| 400 valid entities | 5407 | 5183 | 2.57% |
------------------------------------------------------------------------------
| 200 valid entities | 3075 | 2729 | 5.15% |
------------------------------------------------------------------------------
| 200 invalid entity | 3181 | 2895 | 10.66% |
------------------------------------------------------------------------------
| 200 ampersand | 1190 | 1180 | 0.18% |
------------------------------------------------------------------------------
| 100 valid entities | 1628 | 1533 | 5.54% |
------------------------------------------------------------------------------
| 50 valid entities | 1008 | 881 | 4.68% |
------------------------------------------------------------------------------
| String ends with & | 174 | 174 | 0.00% |
------------------------------------------------------------------------------
Improvements affect the C function
traverse_for_entities:memchrto search for '&' instead of scanning character by character.memchrto locate ';' to determine potential entity boundaries instead ofprocess_named_entity_html, avoiding unnecessary per-character validations.memcpyinstead of character-by-character copying.Benchmark branch - https://github.com/ArtUkrainskiy/php-src/tree/html_entity_decode/benchmark
As you can see, the speedup depends on the number of entities and
&characters in the string — the fewer there are, the more noticeable the performance improvement.In edge cases, where the string consists entirely of
&characters or valid HTML entities, performance actually worsens. However, I don't think this is a common scenario.Either way, I plan to continue optimizing and implement
&scanning using SIMD instructions, which should significantly improve performance even in high-entity-density cases.