Skip to content
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

Separate internal and user function extension handles #14252

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented May 16, 2024

This allows us to skip zend_init_internal_run_time_cache() when opcache is enabled. This function can be quite expensive.

This is backwards in compatible with other extensions that use extension handles for internal functions. If they rely on zend_get_op_array_extension_handle() to register cache for internal functions, they might run into segfaults due to NULL pointers, since the cache won't actually be allocated. Even worse, the offset they rely on may clash with that of other, correctly installed internal function handles, and/or overflow the internal function completely, leading to unpredictable behavior. Hence, it might be better to delay this to master.

Benchmark

Tested with Symfony Demo. The time win is constant across applications, so the relative time win will look more favorable for shorter requests.

Before:
3.145369 + 3.146658 + 3.146848 + 3.148367 + 3.148696 + 3.148788 + 3.149109 + 3.149189 + 3.149542 + 3.149608 + 3.150335 + 3.151215 + 3.151918 + 3.151977 + 3.153058 + 3.153311 + 3.158170 + 3.161079 + 3.185014 + 3.249802
Avg: 3.15740265

After:
3.141876 + 3.142185 + 3.142216 + 3.144321 + 3.144610 + 3.145507 + 3.146388 + 3.146561 + 3.146796 + 3.147445 + 3.147642 + 3.147791 + 3.148390 + 3.148433 + 3.149037 + 3.150181 + 3.150525 + 3.151157 + 3.151939 + 3.153810
Avg: 3.1473405

Diff: -0.32%

@iluuu1994 iluuu1994 requested a review from bwoebi May 16, 2024 12:05
@iluuu1994 iluuu1994 changed the title Separate internal and user function call handles Separate internal and user function extension handles May 16, 2024
@iluuu1994
Copy link
Member Author

iluuu1994 commented May 16, 2024

Actually, I'm just seeing that zend_observer_fcall_op_array_extension is exported and used in a few other places. I'll have to take a closer look at those. Edit: Done.

@iluuu1994 iluuu1994 force-pushed the separate-i-and-u-function-extension-handles branch 3 times, most recently from 111e8dc to 25d1b6e Compare May 16, 2024 12:23
@iluuu1994 iluuu1994 marked this pull request as ready for review May 16, 2024 13:11
@iluuu1994 iluuu1994 requested a review from dstogov as a code owner May 16, 2024 13:11
This allows us to skip zend_init_internal_run_time_cache() when opcache is
enabled. This function can be quite expensive.
@iluuu1994 iluuu1994 changed the base branch from PHP-8.2 to master May 16, 2024 13:45
@iluuu1994 iluuu1994 force-pushed the separate-i-and-u-function-extension-handles branch from 25d1b6e to 95e0c97 Compare May 16, 2024 13:45
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

I think it's OK.

@bwoebi why you don't use indirect map_ptrs?
they should be already used for internal classes.
I noticed this function profiling Symfony Demo.
It took ~2% with disabled observer and enabled JIT.

@iluuu1994
Copy link
Member Author

@dstogov I think the downside is that then we have to do the run time check for internal functions too, as well as allocate more arena space. This might still be faster overall, I don't know. We'd have to try.

@iluuu1994
Copy link
Member Author

@dstogov I started working on your idea in master...iluuu1994:php-src:alloc-internal-rtc-at-runtime. I'm very skeptical it will be faster than this solution, because (compared to this one) it only avoids a fbc->type == ZEND_USER_FUNCTION check for user functions. For internal functions, the !RUN_TIME_CACHE(&fbc->op_array) check is likely more expensive.

Anyway, I haven't measured this yet. I will do so tomorrow.

@bwoebi
Copy link
Member

bwoebi commented May 20, 2024

@iluuu1994 The current form of the PR looks pretty reasonable and about zero extra cost to observers for me, so I like it :-)

@iluuu1994 iluuu1994 force-pushed the separate-i-and-u-function-extension-handles branch from 563c465 to 95e0c97 Compare May 20, 2024 17:34
@iluuu1994 iluuu1994 closed this in 62ebe82 May 21, 2024
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.

None yet

3 participants