Skip to content

Optimizations for atomic #8764

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Optimizations for atomic #8764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0241b3b
Remove ZEND_GCC_PREREQ()
twose Jun 13, 2022
2b8ba99
Prev does not need to be initialized
twose Jun 13, 2022
3c8da57
Make zend_atomic_bool_load() function signature static
twose Jun 13, 2022
878b816
Drop support of clang 7 and older
twose Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions UPGRADING
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ PHP 8.2 UPGRADE NOTES
1. Backward Incompatible Changes
========================================

- Core:
. Drop support of clang 7 and older.

Comment on lines +22 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@twose twose Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have referenced this pull requested in #8881, and the issue has been hung for a long time...

Copy link
Contributor

Choose a reason for hiding this comment

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

"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.

- Date:
. DateTime::createFromImmutable() now has a tentative return type of static,
previously it was DateTime.
Expand Down
7 changes: 0 additions & 7 deletions Zend/zend_atomic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,6 @@ ZEND_API void zend_atomic_bool_store(zend_atomic_bool *obj, bool desired) {
zend_atomic_bool_store_ex(obj, desired);
}

#if ZEND_WIN32 || HAVE_SYNC_ATOMICS
/* On these platforms it is non-const due to underlying APIs. */
ZEND_API bool zend_atomic_bool_load(zend_atomic_bool *obj) {
return zend_atomic_bool_load_ex(obj);
}
#else
ZEND_API bool zend_atomic_bool_load(const zend_atomic_bool *obj) {
return zend_atomic_bool_load_ex(obj);
}
#endif
26 changes: 7 additions & 19 deletions Zend/zend_atomic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,17 @@

#include <stdbool.h>

#define ZEND_GCC_PREREQ(x, y) \
((__GNUC__ == (x) && __GNUC_MINOR__ >= (y)) || (__GNUC__ > (x)))

/* Builtins are used to avoid library linkage */
#if __has_feature(c_atomic)
#define HAVE_C11_ATOMICS 1
#elif ZEND_GCC_PREREQ(4, 7)
#elif ZEND_GCC_VERSION >= 4007
#define HAVE_GNUC_ATOMICS 1
#elif defined(__GNUC__)
#define HAVE_SYNC_ATOMICS 1
#elif !defined(ZEND_WIN32)
#define HAVE_NO_ATOMICS 1
#endif

#undef ZEND_GCC_PREREQ

/* Treat zend_atomic_* types as opaque. They have definitions only for size
* and alignment purposes.
*/
Expand Down Expand Up @@ -63,10 +58,9 @@ static zend_always_inline bool zend_atomic_bool_exchange_ex(zend_atomic_bool *ob
return InterlockedExchange8(&obj->value, desired);
}

/* On this platform it is non-const due to Iterlocked API*/
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);
}

Comment on lines -67 to 65
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@twose twose Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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...

static zend_always_inline void zend_atomic_bool_store_ex(zend_atomic_bool *obj, bool desired) {
Expand Down Expand Up @@ -94,13 +88,13 @@ static zend_always_inline void zend_atomic_bool_store_ex(zend_atomic_bool *obj,
#define ZEND_ATOMIC_BOOL_INIT(obj, desired) ((obj)->value = (desired))

static zend_always_inline bool zend_atomic_bool_exchange_ex(zend_atomic_bool *obj, bool desired) {
bool prev = false;
bool prev;
__atomic_exchange(&obj->value, &desired, &prev, __ATOMIC_SEQ_CST);
return prev;
}

static zend_always_inline bool zend_atomic_bool_load_ex(const zend_atomic_bool *obj) {
bool prev = false;
bool prev;
__atomic_load(&obj->value, &prev, __ATOMIC_SEQ_CST);
return prev;
}
Expand All @@ -123,9 +117,9 @@ static zend_always_inline bool zend_atomic_bool_exchange_ex(zend_atomic_bool *ob
return prev;
}

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 false won't change the value */
return __sync_fetch_and_or(&obj->value, false);
return __sync_fetch_and_or(&((zend_atomic_bool *) obj)->value, false);
}

static zend_always_inline void zend_atomic_bool_store_ex(zend_atomic_bool *obj, bool desired) {
Expand Down Expand Up @@ -159,13 +153,7 @@ static zend_always_inline bool zend_atomic_bool_exchange_ex(zend_atomic_bool *ob
ZEND_API void zend_atomic_bool_init(zend_atomic_bool *obj, bool desired);
ZEND_API bool zend_atomic_bool_exchange(zend_atomic_bool *obj, bool desired);
ZEND_API void zend_atomic_bool_store(zend_atomic_bool *obj, bool desired);

#if ZEND_WIN32 || HAVE_SYNC_ATOMICS
/* On these platforms it is non-const due to underlying APIs. */
ZEND_API bool zend_atomic_bool_load(zend_atomic_bool *obj);
#else
ZEND_API bool zend_atomic_bool_load(const zend_atomic_bool *obj);
#endif

END_EXTERN_C()

Expand Down