From ef5f2322a767d84d31dc67d0be9a751054cf8b19 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 7 Sep 2023 13:28:58 -0700 Subject: [PATCH] IPC: Remove the "small" object optimization in QNativeIpcKey The original design was supposed to be small, at a single pointer, but that never made it through, with the "QString key" member. So the anonymous union for typeAndFlags overlapping with the extension pointer was just unnecessary headache. So separate the two. This means QNativeIpcKey's size increases from 4 pointers to 40 bytes on 64-bit systems and to 24 bytes on 32-bit systems (so we have 6 unused bytes on both architectures). Fixes: QTBUG-116821 Pick-to: 6.6.0 Change-Id: I512648fd617741199e67fffd1782b7d5ba5ddd12 Reviewed-by: Volker Hilsheimer (cherry picked from commit eb5c5a76cf369dd5e40289c1013f043c7e82ff2e) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/ipc/qtipccommon.cpp | 37 +++++------------ src/corelib/ipc/qtipccommon.h | 74 +++++++++++++++------------------ src/corelib/ipc/qtipccommon_p.h | 18 -------- 3 files changed, 45 insertions(+), 84 deletions(-) diff --git a/src/corelib/ipc/qtipccommon.cpp b/src/corelib/ipc/qtipccommon.cpp index f5b576962f3..06c6932f509 100644 --- a/src/corelib/ipc/qtipccommon.cpp +++ b/src/corelib/ipc/qtipccommon.cpp @@ -375,8 +375,7 @@ QNativeIpcKey::Type QNativeIpcKey::defaultTypeForOs_internal() noexcept */ void QNativeIpcKey::copy_internal(const QNativeIpcKey &other) { - auto copy = new QNativeIpcKeyPrivate(*other.d_func()); - d = quintptr(copy) & 1; + d = new QNativeIpcKeyPrivate(*other.d); } void QNativeIpcKey::move_internal(QNativeIpcKey &&) noexcept @@ -386,21 +385,13 @@ void QNativeIpcKey::move_internal(QNativeIpcKey &&) noexcept QNativeIpcKey &QNativeIpcKey::assign_internal(const QNativeIpcKey &other) { - QNativeIpcKeyPrivate *us = (d & 1) ? d_func() : nullptr; - const QNativeIpcKeyPrivate *them = (other.d & 1) ? other.d_func() : nullptr; - if (us && !them) { - // don't need the extra info, reset to skinny object - typeAndFlags = {}; - typeAndFlags.type = us->type; - delete us; - } else { - // do need the extra info, so create if necessary - if (us) - *us = *them; - else - us = new QNativeIpcKeyPrivate(*them); - d = quintptr(us) | 1; - } + Q_ASSERT(d || other.d); // only 3 cases to handle + if (d && !other.d) + *d = {}; + else if (d) + *d = *other.d; + else + d = new QNativeIpcKeyPrivate(*other.d); return *this; } @@ -411,7 +402,6 @@ QNativeIpcKey &QNativeIpcKey::assign_internal(const QNativeIpcKey &other) */ void QNativeIpcKey::destroy_internal() noexcept { - Q_D(QNativeIpcKey); delete d; } @@ -458,11 +448,6 @@ void QNativeIpcKey::destroy_internal() noexcept \sa nativeKey(), setType() */ -QNativeIpcKey::Type QNativeIpcKey::type_internal() const noexcept -{ - Q_D(const QNativeIpcKey); - return d->type; -} /*! \fn QNativeIpcKey::setType(Type type) @@ -473,8 +458,7 @@ QNativeIpcKey::Type QNativeIpcKey::type_internal() const noexcept */ void QNativeIpcKey::setType_internal(Type type) { - Q_D(QNativeIpcKey); - d->type = type; + Q_UNUSED(type); } /*! @@ -520,7 +504,8 @@ size_t qHash(const QNativeIpcKey &ipcKey, size_t seed) noexcept */ int QNativeIpcKey::compare_internal(const QNativeIpcKey &lhs, const QNativeIpcKey &rhs) noexcept { - return *lhs.d_func() == *rhs.d_func() ? 0 : 1; + Q_UNUSED(lhs); Q_UNUSED(rhs); + return 0; } /*! diff --git a/src/corelib/ipc/qtipccommon.h b/src/corelib/ipc/qtipccommon.h index ba26681787a..bf0936cb42a 100644 --- a/src/corelib/ipc/qtipccommon.h +++ b/src/corelib/ipc/qtipccommon.h @@ -18,7 +18,7 @@ class QNativeIpcKey { Q_GADGET_EXPORT(Q_CORE_EXPORT) public: - enum class Type : quintptr { + enum class Type : quint16 { // 0 is reserved for the invalid type // keep 1 through 0xff free, except for SystemV SystemV = 0x51, // 'Q' @@ -37,31 +37,28 @@ public: ; static Type legacyDefaultTypeForOs() noexcept; - constexpr QNativeIpcKey() noexcept - : QNativeIpcKey(DefaultTypeForOs) - {} + constexpr QNativeIpcKey() noexcept = default; explicit constexpr QNativeIpcKey(Type type) noexcept - : d() + : typeAndFlags{type} { - typeAndFlags.type = type; } Q_IMPLICIT QNativeIpcKey(const QString &k, Type type = DefaultTypeForOs) - : d(), key(k) + : key(k), typeAndFlags{type} { - typeAndFlags.type = type; } QNativeIpcKey(const QNativeIpcKey &other) - : d(other.d), key(other.key) + : d(other.d), key(other.key), typeAndFlags(other.typeAndFlags) { if (isSlowPath()) copy_internal(other); } QNativeIpcKey(QNativeIpcKey &&other) noexcept - : d(std::exchange(other.d, 0)), key(std::move(other.key)) + : d(std::exchange(other.d, nullptr)), key(std::move(other.key)), + typeAndFlags(std::move(other.typeAndFlags)) { if (isSlowPath()) move_internal(std::move(other)); @@ -75,10 +72,11 @@ public: QNativeIpcKey &operator=(const QNativeIpcKey &other) { + typeAndFlags = other.typeAndFlags; key = other.key; if (isSlowPath() || other.isSlowPath()) return assign_internal(other); - d = other.d; + Q_ASSERT(!d); return *this; } @@ -87,6 +85,7 @@ public: { std::swap(d, other.d); key.swap(other.key); + typeAndFlags.swap(other.typeAndFlags); } bool isEmpty() const noexcept @@ -101,8 +100,6 @@ public: constexpr Type type() const noexcept { - if (isSlowPath()) - return type_internal(); return typeAndFlags.type; } @@ -114,7 +111,9 @@ public: } QString nativeKey() const noexcept - { return key; } + { + return key; + } void setNativeKey(const QString &newKey) { key = newKey; @@ -127,36 +126,32 @@ public: private: struct TypeAndFlags { -#if Q_BYTE_ORDER == Q_LITTLE_ENDIAN - // this is the LSB - quintptr isExtended : 1; - Type type : 15; -#endif + Type type = DefaultTypeForOs; + quint16 reserved1 = {}; + quint32 reserved2 = {}; - quintptr reserved : sizeof(quintptr) * 8 - 16; + void swap(TypeAndFlags &other) noexcept + { + std::swap(type, other.type); + std::swap(reserved1, other.reserved1); + std::swap(reserved2, other.reserved2); + } -#if Q_BYTE_ORDER == Q_BIG_ENDIAN - Type type : 15; - quint16 isExtended : 1; - // this was the LSB -#endif - }; - - // Bit 0: if set, holds a pointer (with the LSB set); if clear, holds the - // the TypeAndFlags structure. - union { - quintptr d = 0; - TypeAndFlags typeAndFlags; - static_assert(sizeof(typeAndFlags) == sizeof(d)); + friend constexpr bool operator==(const TypeAndFlags &lhs, const TypeAndFlags &rhs) noexcept + { + return lhs.type == rhs.type && + lhs.reserved1 == rhs.reserved1 && + lhs.reserved2 == rhs.reserved2; + } }; + QNativeIpcKeyPrivate *d = nullptr; QString key; + TypeAndFlags typeAndFlags; friend class QNativeIpcKeyPrivate; - QNativeIpcKeyPrivate *d_func(); - const QNativeIpcKeyPrivate *d_func() const; constexpr bool isSlowPath() const noexcept - { return Q_UNLIKELY(typeAndFlags.isExtended); } + { return Q_UNLIKELY(d); } friend Q_CORE_EXPORT size_t qHash(const QNativeIpcKey &ipcKey, size_t seed) noexcept; friend size_t qHash(const QNativeIpcKey &ipcKey) noexcept @@ -164,13 +159,13 @@ private: friend bool operator==(const QNativeIpcKey &lhs, const QNativeIpcKey &rhs) noexcept { + if (!(lhs.typeAndFlags == rhs.typeAndFlags)) + return false; if (lhs.key != rhs.key) return false; if (lhs.d == rhs.d) return true; - if (lhs.isSlowPath() && rhs.isSlowPath()) - return compare_internal(lhs, rhs) == 0; - return false; + return compare_internal(lhs, rhs) == 0; } friend bool operator!=(const QNativeIpcKey &lhs, const QNativeIpcKey &rhs) noexcept { @@ -181,7 +176,6 @@ private: Q_CORE_EXPORT void move_internal(QNativeIpcKey &&other) noexcept; Q_CORE_EXPORT QNativeIpcKey &assign_internal(const QNativeIpcKey &other); Q_CORE_EXPORT void destroy_internal() noexcept; - Q_DECL_PURE_FUNCTION Q_CORE_EXPORT Type type_internal() const noexcept; Q_CORE_EXPORT void setType_internal(Type); Q_CORE_EXPORT void setNativeKey_internal(const QString &); Q_DECL_PURE_FUNCTION Q_CORE_EXPORT static int diff --git a/src/corelib/ipc/qtipccommon_p.h b/src/corelib/ipc/qtipccommon_p.h index b2cb3d02e21..3d6ad0903b8 100644 --- a/src/corelib/ipc/qtipccommon_p.h +++ b/src/corelib/ipc/qtipccommon_p.h @@ -31,26 +31,8 @@ QT_BEGIN_NAMESPACE class QNativeIpcKeyPrivate { public: - QNativeIpcKey::Type type = {}; - - friend bool operator==(const QNativeIpcKeyPrivate &lhs, const QNativeIpcKeyPrivate &rhs) - { - return lhs.type == rhs.type; - } }; -inline QNativeIpcKeyPrivate *QNativeIpcKey::d_func() -{ - Q_ASSERT(d & 1); // Q_ASSERT(isSlowPath) but without the unlikely - return reinterpret_cast(d & ~1); -} - -inline const QNativeIpcKeyPrivate *QNativeIpcKey::d_func() const -{ - Q_ASSERT(d & 1); // Q_ASSERT(isSlowPath) but without the unlikely - return reinterpret_cast(d & ~1); -} - namespace QtIpcCommon { enum class IpcType { SharedMemory,