Skip to content

Bump flag size #13886

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

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Bump flag size #13886

wants to merge 7 commits into from

Conversation

iluuu1994
Copy link
Member

Packing of zend_function may be slightly improved, e.g. by moving up zend_function.T. I'm not sure that's worthwhile.

/cc @derickr

@iluuu1994 iluuu1994 force-pushed the bump-flag-size branch 2 times, most recently from 8657ee2 to 09af862 Compare April 4, 2024 23:39
@nielsdos
Copy link
Member

nielsdos commented Apr 5, 2024

Benchmark CI job slows a 0.33% increase in instruction count for Symfony. Whether that translates to real-world impact, I haven't checked.
Making the flags 64-bit may make a potential slowdown on 32-bit architectures, although the question remains how relevant those are nowadays anyway.

I do have an alternative proposal though, one that may be a bit simpler and has less risk of missing a change in the code. We could have 2 separate 32-bit fields for flags. Depending on what flag it is, it either belongs to one or the other field. Most code will be unchanged therefore, and we don't risk missing a change. It maybe also a zero-performance-impact solution.

This may also fix FIXME: sizeof(zend_long) < sizeof(zend_prop_flags) on 32-bit platforms, because we could group flags that are relevant into the same field. Depending on what flags are accepted, we don't have to change anything there at all. The caveat is that if we have flags sharing the same bit we can't distinguish them. So if we accept any kind of flag, this solution becomes not applicable. However, some flags are internal and should probably not be possible to get queried?

Another risk this alternative introduces is that people can get confused which flag belongs to which field.

@iluuu1994
Copy link
Member Author

I wonder if the symfony demo bench is a statistical outlier, that seems very high for this change. Either way, your suggestion might make more sense, given that it drastically reduces code changes required.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Apr 5, 2024

Another risk this alternative introduces is that people can get confused which flag belongs to which field.

I suppose if we keep the new field target-specific, we could use a bitfield. That would avoid any confusion. (Or is that not C++ compatible? I don't know C++. I'll check when I'm at a computer).

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.

2 participants