-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Optimize array_key_exists/in_array for empty array #4925
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
base: master
Are you sure you want to change the base?
Conversation
The tests are failing - I'm not familiar with the SSA implementation, and I don't know how to properly replace the function call with the result in all of the affected uses of the variable - the ZEND_QM_ASSIGN is a workaround to prototype this. |
Once again, I feel like there should be a better place to do this ... possibly as a combination of TI, SCCP and DCE. |
Part 1: We can DCE on array_key_exists whose result is not used: 9e403d6 |
Oops, I scrolled over the important part of the patch ... this does use SCCP. The main problem is the handling of errors -- SCCP has an assumption that any value we can compute, we can compute without triggering a diagnostic. To handle this cleanly, we should be separating out that assumption... |
8882e28
to
3bf4457
Compare
I'm assuming the only diagnostic to be concerned about in php 8.0 is the undefined variable warning - TypeError is now thrown by array_key_exists for invalid keys. I agree that it'd be cleaner and help with eliminating dead code such as |
Not sure how to proceed here, but at least the merge conflicts would need to be resolved. |
3bf4457
to
7f47104
Compare
Make opcache replace the result with false if the array argument is known to be empty. This may be useful when a codebase has placeholders, e.g. `if (!in_array($method, self::ALLOWED_METHODS)) { return; }` In zend_inference.c: In php 8, array_key_exists will throw a TypeError instead of returning null. I didn't see any discussion of this optimization (for/against) after a quick search on github, e.g. phpGH-3360 Potential future optimizations: - convert `in_array($needle, ['only one element'], true)` to `===`? (or `==` for strict=false) - When the number of elements is less than 4, switch to looping instead of hash lookup. (exact threshold for better performance to be determined) Also support looping for `in_array($value, [false, 'str', 2.5], true/false)`
7f47104
to
0989f65
Compare
Make opcache replace the result with false if the array argument is
known to be empty.
This may be useful when a codebase has placeholders,
e.g.
if (!in_array($method, self::ALLOWED_METHODS)) { return; }
In zend_inference.c: In php 8, array_key_exists will throw
a TypeError instead of returning null. I can create a separate PR for this if needed.
I didn't see any discussion of this optimization (for/against)
after a quick search on github, e.g. GH-3360
Potential future optimizations:
convert
in_array($needle, ['only one element'], true)
to===
?(or
==
for strict=false)When the number of elements is less than 4, switch to looping instead of hash
lookup. (exact threshold for better performance to be determined)
Also support looping for
in_array($value, [false, 'str', 2.5], true/false)