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

Zend: Use pointer to zend_type for variance checks #18257

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

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Apr 5, 2025

Depends on #18256

Copy link
Member

@DanielEScherzer DanielEScherzer left a comment

Choose a reason for hiding this comment

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

Okay for ext/reflection

@Girgias Girgias force-pushed the variance-checks-use-pointer branch from cb494fd to d8e1e19 Compare April 7, 2025 11:53
@iluuu1994
Copy link
Member

What's the motivation behind this? Does it improve assembly size due to better argument passing? All the type macros also work on pointers, so this becomes a bit unergonomic. But I don't have big objections.

@Girgias
Copy link
Member Author

Girgias commented Apr 8, 2025

Main motivation is for #18260 where I want to store a pointer to the bound zend_type in a HashTable. This might also pave the way for #18189 if we start reusing zend_types instead of copying them. Could definitely use the 32bit of padding of zend_type for a refcount field.

@iluuu1994
Copy link
Member

IMO, the overhead of refcounting to save 8 bytes (8 byte pointer vs zend_type of size 16 bytes) is not worthwhile. Interning the complex type (i.e. the zend_type.ptr field) should be enough, and doesn't require any changes to the type struct. You could make an argument that interning the full type is cleaner, but in practice it's unlikely to make a real difference.

That said, passing types through args as pointers might still make sense, at least if they're not immediately copied to the new call frame. If the intention is to do this on a larger scale, it might make sense to introduce macros that work on pointers. Feel free to ask for other opinions.

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.

3 participants