Skip to content

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
@krakjoe
Copy link
Member

krakjoe commented Aug 31, 2025

I'm closing this on the basis that it's stale (and conflicting), and the conversation left questions open that were not addressed.

I can easily be wrong, if you feel very strongly this is worthwhile, please open a fresh PR against the master branch.

@krakjoe krakjoe closed this Aug 31, 2025
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.

5 participants