Optimizations for atomic #8764
Optimizations for atomic #8764
Conversation
twose
commented
Jun 13, 2022
twose
commented
- Remove ZEND_GCC_PREREQ() (We already have ZEND_GCC_VERSION to do such check).
- Prev does not need to be initialized (__atomic_load() returns the contents of *ptr in *ret).
- Make zend_atomic_bool_load() function signature static.
Zend/zend_atomic.h
Outdated
| @@ -82,7 +76,7 @@ static zend_always_inline bool zend_atomic_bool_exchange_ex(zend_atomic_bool *ob | |||
| } | |||
|
|
|||
| static zend_always_inline bool zend_atomic_bool_load_ex(const zend_atomic_bool *obj) { | |||
| return __c11_atomic_load(&obj->value, __ATOMIC_SEQ_CST); | |||
| return __c11_atomic_load(&((zend_atomic_bool *) obj)->value, __ATOMIC_SEQ_CST); | |||
There was a problem hiding this comment.
If we need to cast the const-ness away, why do we declare the argument as const in the first place?
There was a problem hiding this comment.
Edit: this doesn't require a const cast, at least not on clang 13. This intrinsic's exact signature is not documented, so I built it locally on Mac.
There was a problem hiding this comment.
I think load should use const pointer: it won't change data that the pointer pointed to, and this is a clang specific behavior, this is fixed since clang 8.0:
Will PHP drop support of clang 7 and older?
There was a problem hiding this comment.
It's just a workaround, and it should be const.
As some atomic implementations use fetch_add(0) to implement load(), the parameter type of load() is still const to indicate that the value will not be changed.
There was a problem hiding this comment.
I don't see how lying about const correctness is better.
In my opinion, the only bit worth keeping as it stands is removing ZEND_GCC_PREREQ in favor of ZEND_GCC_VERSION, and maybe the .__c11_atomic_load but I'll have to verify that because the exact signature isn't documented for that intrinsic
We already have ZEND_GCC_VERSION to do such check.
__atomic_load() returns the contents of *ptr in *ret.
|
For @morrisonlevi 's change request, I've reverted some changes, #8881 will not be fixed now, and I've recorded it in the UPGRADING. |
| static zend_always_inline bool zend_atomic_bool_load_ex(zend_atomic_bool *obj) { | ||
| static zend_always_inline bool zend_atomic_bool_load_ex(const zend_atomic_bool *obj) { | ||
| /* Or'ing with false won't change the value. */ | ||
| return InterlockedOr8(&obj->value, false); | ||
| return InterlockedOr8(&((zend_atomic_bool *) obj)->value, false); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm still against lying about const correctness here. On this platform it's not const, sorry. I know it's not as clean as nice to have to detect which platforms it is and isn't const on, but that's just reality.
There was a problem hiding this comment.
I also have said something about that in #8881 :
bson-atomic also used type casting to avoid such problems, to be on the safe side, I've added type-casts to all calls.
I think bson has also been proven in production extensively...
| - Core: | ||
| . Drop support of clang 7 and older. | ||
|
|
There was a problem hiding this comment.
Do you know what exactly broke clang 7 and older? The patches attempt to feature detect/guard so this wasn't purposefully done on my side.
There was a problem hiding this comment.
I have referenced this pull requested in #8881, and the issue has been hung for a long time...
There was a problem hiding this comment.
"clang 7" is insufficiently specific because there are two different compilers called "clang": the one published by llvm.org and the one published by Apple Inc. They have different version numbering schemes and changes that appear in a particular version of one of them may appear in a different version of the other one, or not all.
C11 did not have the const qualifier on atomic_load; C17 does. One can either use C11 atomics by casting away the constness, which was proposed and rejected in phpGH-8764, or one can avoid using C11 atomics altogether, instead using C17 atomics if available, which is what this change does. Fixes phpGH-8881
