Zend: Add zend_check_method_accessible() to DRY method visibility checks
#18995
Conversation
TimWolla
force-pushed
the
dry-method-visibility-checks
branch
2 times, most recently
from
270af52 to
9c89298
Compare
Zend/zend_objects_API.h
Outdated
| static inline bool zend_check_method_accessible(const zend_function *fn, const zend_class_entry *scope) | ||
| { | ||
| if ( | ||
| !(fn->common.fn_flags & ZEND_ACC_PUBLIC) |
There was a problem hiding this comment.
I'd prefer making this an assert, as many of the call-sites have already accounted for this. Some of them are quite hot too, and there's no guarantee the compiler will omit the repeated check.
There was a problem hiding this comment.
Symfony instruction count went up by 0.01%. Seems to confirm this is not omitted, or the function isn't inlined.
There was a problem hiding this comment.
Indeed. For zend_std_get_constructor it appears the flag is rechecked. I hate this, the point of the method was that I don't need to think about this 😒
There was a problem hiding this comment.
Note though that the increase of 0.01% could also be noise. It's within the noise range that I often see on PRs. Looking at the assembly would confirm whether inlining happened or not.
|
I'm also not sure if the |
TimWolla
force-pushed
the
dry-method-visibility-checks
branch
from
9c89298 to
98dc4f0
Compare
| if (!(fn->common.fn_flags & ZEND_ACC_PUBLIC) | ||
| && fn->common.scope != scope | ||
| && (UNEXPECTED(fn->common.fn_flags & ZEND_ACC_PRIVATE) | ||
| || UNEXPECTED(!zend_check_protected(zend_get_function_root_class(fn), scope)))) { |
There was a problem hiding this comment.
Typically the style is to indent subconditions with spaces. One space for every sublevel. I.e.
| if (!(fn->common.fn_flags & ZEND_ACC_PUBLIC) | |
| && fn->common.scope != scope | |
| && (UNEXPECTED(fn->common.fn_flags & ZEND_ACC_PRIVATE) | |
| || UNEXPECTED(!zend_check_protected(zend_get_function_root_class(fn), scope)))) { | |
| if (!(fn->common.fn_flags & ZEND_ACC_PUBLIC) | |
| && fn->common.scope != scope | |
| && (UNEXPECTED(fn->common.fn_flags & ZEND_ACC_PRIVATE) | |
| || UNEXPECTED(!zend_check_protected(zend_get_function_root_class(fn), scope)))) { |
| ZVAL_UNDEF(EX_VAR(opline->result.var)); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
| if (!zend_check_method_accessible(clone, scope)) { |
There was a problem hiding this comment.
If inlining is not guaranteed then the extra check on the public flag is not eliminated. always inlining might be better.
Zend/zend_objects_API.h
Outdated
| static inline bool zend_check_method_accessible(const zend_function *fn, const zend_class_entry *scope) | ||
| { | ||
| if ( | ||
| !(fn->common.fn_flags & ZEND_ACC_PUBLIC) |
There was a problem hiding this comment.
Note though that the increase of 0.01% could also be noise. It's within the noise range that I often see on PRs. Looking at the assembly would confirm whether inlining happened or not.
TimWolla
force-pushed
the
dry-method-visibility-checks
branch
from
98dc4f0 to
db4b7ee
Compare
| zend_object_store_ctor_failed(zobj); | ||
| constructor = NULL; | ||
| } | ||
| if (!zend_check_method_accessible(constructor, scope)) { |
There was a problem hiding this comment.
To solve the fn_flags reload problem, just insert an ZEND_ASSERT/ZEND_ASSUME here, right before the call to zend_check_method_accessible (preferably an assertion).
There was a problem hiding this comment.
Ah, good idea. That would keep zend_check_method_accessible() easy to use compared to asserting within zend_check_method_accessible() itself. I'll check this tomorrow.
There was a problem hiding this comment.
Now done. The CI Benchmark still sees a 0.01% increase, which now might actually be noise then?
There was a problem hiding this comment.
Can't check now. You didn't specify always_inline though. I also see 0.02 on symfony
To (partly) follow up on #18919 (comment) and #18919 (comment). Best reviewed with whitespace changes ignored.