Skip to content

zend_execute: Streamline typechecks in zend_check_type_slow() if an object is given #18277

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.

Sign up for GitHub

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 2 commits into
base: master
Choose a base branch
from
Open

zend_execute: Streamline typechecks in zend_check_type_slow() if an object is given #18277

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e216a2
zend_execute: Improve type checking performance when passing a `Closu…
TimWolla Apr 8, 2025
0cca79a
zend_execute: Streamline typechecks in `zend_check_type_slow()` if an…
TimWolla Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions Zend/zend_execute.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1177,8 +1177,14 @@ static zend_always_inline bool zend_check_type_slow(
}

const uint32_t type_mask = ZEND_TYPE_FULL_MASK(*type);
if ((type_mask & MAY_BE_CALLABLE) &&
zend_is_callable(arg, is_internal ? IS_CALLABLE_SUPPRESS_DEPRECATIONS : 0, NULL)) {
if (
(type_mask & MAY_BE_CALLABLE)
&& (
/* Fast check for closures. */
EXPECTED(Z_OBJCE_P(arg) == zend_ce_closure)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think arg is guaranteed to be an object at this point. Maybe this can be combined with the Z_TYPE_P(arg) == IS_OBJECT) above.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can combine it with the one above as a type declaration that only contains built-in types won't be complex.

Copy link
Member

Choose a reason for hiding this comment

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

I was referring to something like:

if (Z_TYPE_P(arg) == IS_OBJECT) {
    if ((type_mask & MAY_BE_CALLABLE) && EXPECTED(Z_OBJCE_P(arg) == zend_ce_closure)) {
        return true;
    } else if (ZEND_TYPE_IS_COMPLEX(*type)) {
        ...
    }
}

Not sure if that's better

Copy link
Member

@Girgias Girgias Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is reasonable to have if(EXPECTED(Z_TYPE_P(arg) == IS_OBJECT) {} first, as to get to this point you need to cross if (EXPECTED(ZEND_TYPE_CONTAINS_CODE(*type, Z_TYPE_P(arg)))) { from zend_check_type().

Meaning that we are either dealing with an object, a callable, or a scalar type that needs to be coerced which depends on strict_type.

The nice benefit of this approach is that it would mean that the call to zend_value_instanceof_static() could be moved into that new if block and remove the check for if (Z_TYPE_P(zv) != IS_OBJECT) {

Copy link
Member

@iluuu1994 iluuu1994 Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. The may also still check for ZEND_TYPE_IS_COMPLEX(*type) first (before type_mask & MAY_BE_CALLABLE) to avoid any degradation of complex types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted according to Gina's suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

Is static really more common than callable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is static really more common than callable?

I'm seeing static used quite a bit for return types. The closure check is probably cheaper, though. No strong opinion here. Can reorder if desired (ideally after someone else verified the numbers, because I don't really trust the results on my CPU).

Copy link
Member

Choose a reason for hiding this comment

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

Same. My benchmarks are usually all over the place... I have yet to find a setting for me that works well.

Copy link
Member

@Girgias Girgias Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I guess we can have the static type check after the quick Closure check, especially as we could now transform static to the class name for final classes.

|| zend_is_callable(arg, is_internal ? IS_CALLABLE_SUPPRESS_DEPRECATIONS : 0, NULL)
)
) {
return 1;
}
if ((type_mask & MAY_BE_STATIC) && zend_value_instanceof_static(arg)) {
Expand Down
Loading