Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TysonAndre
Copy link
Contributor

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)

@TysonAndre
Copy link
Contributor Author

001+ In function ExampleInArray::test (after calls):
002+ non-var op1 of 5 (ZEND_QM_ASSIGN) uses or defs an ssa var
003+ op1 use of 13 (CV $x) does not match op -3 of 5 (ZEND_QM_ASSIGN)
004+ non-var op1 of 11 (ZEND_QM_ASSIGN) uses or defs an ssa var
005+ op1 use of 13 (CV $x) does not match op -3 of 11 (ZEND_QM_ASSIGN)
006+ non-var op1 of 83 (ZEND_QM_ASSIGN) uses or defs an ssa var
007+ op1 use of 51 (TMP) does not match op -3 of 83 (ZEND_QM_ASSIGN)
008+ 
009+ In function ExampleInArray::test (after dce):
010+ non-var op1 of 5 (ZEND_QM_ASSIGN) uses or defs an ssa var
011+ op1 use of 13 (CV $x) does not match op -3 of 5 (ZEND_QM_ASSIGN)
001- Warning: Undefined variable: undef in %s on line 11

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.

@nikic
Copy link
Member

nikic commented Dec 5, 2019

Once again, I feel like there should be a better place to do this ... possibly as a combination of TI, SCCP and DCE.

@nikic
Copy link
Member

nikic commented Dec 5, 2019

Part 1: We can DCE on array_key_exists whose result is not used: 9e403d6

@nikic
Copy link
Member

nikic commented Dec 5, 2019

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...

@TysonAndre
Copy link
Contributor Author

TysonAndre commented Dec 20, 2020

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...

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 if (in_array($undef, [true])) { ... } - I'm not familiar enough with SCCP to know how to do that.

@cmb69
Copy link
Member

cmb69 commented Dec 28, 2021

Not sure how to proceed here, but at least the merge conflicts would need to be resolved.

@TysonAndre TysonAndre force-pushed the optimize-array_key_exists branch from 3bf4457 to 7f47104 Compare December 28, 2021 16:43
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)`
@TysonAndre TysonAndre force-pushed the optimize-array_key_exists branch from 7f47104 to 0989f65 Compare December 28, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants