-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
base: master
Are you sure you want to change the base?
Bump flag size #13886
Conversation
8657ee2
to
09af862
Compare
09af862
to
9a153ec
Compare
Benchmark CI job slows a 0.33% increase in instruction count for Symfony. Whether that translates to real-world impact, I haven't checked. 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 Another risk this alternative introduces is that people can get confused which flag belongs to which field. |
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. |
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). |
Packing of
zend_function
may be slightly improved, e.g. by moving upzend_function.T
. I'm not sure that's worthwhile./cc @derickr