Skip to content

Commit

Permalink
QProperty: Notify observers even when dependency is gone
Browse files Browse the repository at this point in the history 
Problem description:
--------------------
Assume we have two properties, P1 and P2. Assume further that we assign
a binding to P2, so that it depends on P1. Let the binding additionally
capture some (non-QProperty) boolean, and only create the dependency to
P1 if the boolean is true.

The state afterwards is
P1:[p1vaue|firstObserver]
                      |
                      |
                      v
                ---[p2binding]
	       /
P2:[p2value|binding]

If the boolean is set to false, and P1 changes its value, we still
correctly re-evaluate the binding and update P2's value. However, during
binding evaluation we will notice that there is no further dependency
from P2 on P1, and remove its observer.

The state afterwards is
P1:[p1vaue|firstObserver=nullptr]

                ---[p2binding]
	       /
P2:[p2value|binding]

Then, during the notify phase, we traverse the observer's again,
starting from P1's firstObserver. Given that it is nullptr now, we never
reach P2's binding, and thus won't send a notification from it.

Fix:
----

We store a list of all visited binding-observers (in a QVarLengthArray,
to avoid allocations as long as possible). After the binding evaluation
phase, we then use that list to send notifications from every binding
that we visited. As we already have a list of all bindings, we no longer
need to recurse on binding-observes during the notification process;
instead, we only need to deal with static callbacks and ChangeHandlers.

The pre-existing notification logic is still kept for the grouped update
case, where we already have a list of all delayed properties, and should
therefore not encounter the same issue. Unifying its codepath with the
existing logic is left as an exercise for a later patch.

Fixes: QTBUG-105204
Task-number: QTBUG-104982
Pick-to: 6.4 6.3 6.2
Change-Id: I2951f7d9597f4da0b8560a64dfb834f7ad86e757
Reviewed-by: Qt CI Bot <[email protected]>
Reviewed-by: Sami Shalayel <[email protected]>
Reviewed-by: Ulf Hermann <[email protected]>
  • Loading branch information
@Inkane
Inkane committed Aug 1, 2022
1 parent ebf733c commit f1b1773
Show file tree
Hide file tree
Showing 4 changed files with 170 additions and 30 deletions.
71 changes: 54 additions & 17 deletions src/corelib/kernel/qproperty.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,12 @@ struct QPropertyDelayedNotifications
Change notifications are sent later with notify (following the logic of separating
binding updates and notifications used in non-deferred updates).
*/
void evaluateBindings(int index, QBindingStatus *status) {
[[nodiscard]] PendingBindingObserverList evaluateBindings(int index, QBindingStatus *status) {
PendingBindingObserverList bindingObservers;
auto *delayed = delayedProperties + index;
auto *bindingData = delayed->originalBindingData;
if (!bindingData)
return;
return bindingObservers;

bindingData->d_ptr = delayed->d_ptr;
Q_ASSERT(!(bindingData->d_ptr & QPropertyBindingData::DelayedNotificationBit));
Expand All @@ -134,7 +135,8 @@ struct QPropertyDelayedNotifications
QPropertyBindingDataPointer bindingDataPointer{bindingData};
QPropertyObserverPointer observer = bindingDataPointer.firstObserver();
if (observer)
observer.evaluateBindings(status);
observer.evaluateBindings(bindingObservers, status);
return bindingObservers;
}

/*!
Expand Down Expand Up @@ -216,8 +218,11 @@ void Qt::endPropertyUpdateGroup()
// update all delayed properties
auto start = data;
while (data) {
for (int i = 0; i < data->used; ++i)
data->evaluateBindings(i, status);
for (int i = 0; i < data->used; ++i) {
PendingBindingObserverList bindingObserves = data->evaluateBindings(i, status);
Q_UNUSED(bindingObserves);
// ### TODO: Use bindingObservers for notify
}
data = data->next;
}
// notify all delayed properties
Expand Down Expand Up @@ -271,11 +276,11 @@ void QPropertyBindingPrivate::unlinkAndDeref()
destroyAndFreeMemory(this);
}

void QPropertyBindingPrivate::evaluateRecursive(QBindingStatus *status)
void QPropertyBindingPrivate::evaluateRecursive(PendingBindingObserverList &bindingObservers, QBindingStatus *status)
{
if (!status)
status = &bindingStatus;
return evaluateRecursive_inline(status);
return evaluateRecursive_inline(bindingObservers, status);
}

void QPropertyBindingPrivate::notifyRecursive()
Expand All @@ -294,6 +299,31 @@ void QPropertyBindingPrivate::notifyRecursive()
updating = false;
}

void QPropertyBindingPrivate::notifyNonRecursive(const PendingBindingObserverList &bindingObservers)
{
notifyNonRecursive();
for (auto &&bindingObserver: bindingObservers) {
bindingObserver.binding()->notifyNonRecursive();
}
}

QPropertyBindingPrivate::NotificationState QPropertyBindingPrivate::notifyNonRecursive()
{
if (!pendingNotify)
return Delayed;
pendingNotify = false;
Q_ASSERT(!updating);
updating = true;
if (firstObserver) {
firstObserver.noSelfDependencies(this);
firstObserver.notifyOnlyChangeHandler(propertyDataPtr);
}
if (hasStaticObserver)
staticObserverCallback(propertyDataPtr);
updating = false;
return Sent;
}

/*!
Constructs a null QUntypedPropertyBinding.
Expand Down Expand Up @@ -461,8 +491,9 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB
newBindingRaw->prependObserver(observer);
newBindingRaw->setStaticObserver(staticObserverCallback, guardCallback);

newBindingRaw->evaluateRecursive();
newBindingRaw->notifyRecursive();
PendingBindingObserverList bindingObservers;
newBindingRaw->evaluateRecursive(bindingObservers);
newBindingRaw->notifyNonRecursive(bindingObservers);
} else if (observer) {
d.setObservers(observer.ptr);
} else {
Expand Down Expand Up @@ -565,18 +596,23 @@ void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr
return;
QPropertyBindingDataPointer d{this};

PendingBindingObserverList bindingObservers;
if (QPropertyObserverPointer observer = d.firstObserver()) {
if (notifyObserver_helper(propertyDataPtr, observer, storage) == Evaluated) {
if (notifyObserver_helper(propertyDataPtr, storage, observer, bindingObservers) == Evaluated) {
// evaluateBindings() can trash the observers. We need to re-fetch here.
if (QPropertyObserverPointer observer = d.firstObserver())
observer.notify(propertyDataPtr);
observer.notifyOnlyChangeHandler(propertyDataPtr);
for (auto &&bindingObserver: bindingObservers)
bindingObserver.binding()->notifyNonRecursive();
}
}
}

QPropertyBindingData::NotificationResult QPropertyBindingData::notifyObserver_helper(
QUntypedPropertyData *propertyDataPtr, QPropertyObserverPointer observer,
QBindingStorage *storage) const
QPropertyBindingData::NotificationResult QPropertyBindingData::notifyObserver_helper
(
QUntypedPropertyData *propertyDataPtr, QBindingStorage *storage,
QPropertyObserverPointer observer,
PendingBindingObserverList &bindingObservers) const
{
#ifdef QT_HAS_FAST_CURRENT_THREAD_ID
QBindingStatus *status = storage ? storage->bindingStatus : nullptr;
Expand All @@ -591,7 +627,7 @@ QPropertyBindingData::NotificationResult QPropertyBindingData::notifyObserver_he
return Delayed;
}

observer.evaluateBindings(status);
observer.evaluateBindings(bindingObservers, status);
return Evaluated;
}

Expand Down Expand Up @@ -724,7 +760,7 @@ void QPropertyObserverPointer::noSelfDependencies(QPropertyBindingPrivate *bindi
}
#endif

void QPropertyObserverPointer::evaluateBindings(QBindingStatus *status)
void QPropertyObserverPointer::evaluateBindings(PendingBindingObserverList &bindingObservers, QBindingStatus *status)
{
Q_ASSERT(status);
auto observer = const_cast<QPropertyObserver*>(ptr);
Expand All @@ -733,9 +769,10 @@ void QPropertyObserverPointer::evaluateBindings(QBindingStatus *status)
QPropertyObserver *next = observer->next.data();

if (QPropertyObserver::ObserverTag(observer->next.tag()) == QPropertyObserver::ObserverNotifiesBinding) {
bindingObservers.push_back(observer);
auto bindingToEvaluate = observer->binding;
QPropertyObserverNodeProtector protector(observer);
bindingToEvaluate->evaluateRecursive_inline(status);
bindingToEvaluate->evaluateRecursive_inline(bindingObservers, status);
next = protector.next();
}

Expand Down
95 changes: 84 additions & 11 deletions src/corelib/kernel/qproperty_p.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <qscopedpointer.h>
#include <qscopedvaluerollback.h>
#include <vector>
#include <QtCore/QVarLengthArray>

QT_BEGIN_NAMESPACE

Expand All @@ -29,6 +30,34 @@ namespace QtPrivate {
struct QBindingStatusAccessToken {};
}


/*!
\internal
Similar to \c QPropertyBindingPrivatePtr, but stores a
\c QPropertyObserver * linking to the QPropertyBindingPrivate*
instead of the QPropertyBindingPrivate* itself
*/
struct QBindingObserverPtr
{
private:
QPropertyObserver *d = nullptr;
public:
QBindingObserverPtr() = default;
Q_DISABLE_COPY(QBindingObserverPtr);
void swap(QBindingObserverPtr &other) noexcept
{ qt_ptr_swap(d, other.d); }
QBindingObserverPtr(QBindingObserverPtr &&other) : d(std::exchange(other.d, nullptr)) {}
QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_MOVE_AND_SWAP(QBindingObserverPtr);


inline QBindingObserverPtr(QPropertyObserver *observer);
inline ~QBindingObserverPtr();
inline QPropertyBindingPrivate *binding() const;
inline QPropertyObserver *operator ->();
};

using PendingBindingObserverList = QVarLengthArray<QBindingObserverPtr>;

// Keep all classes related to QProperty in one compilation unit. Performance of this code is crucial and
// we need to allow the compiler to inline where it makes sense.

Expand Down Expand Up @@ -106,19 +135,29 @@ struct QPropertyObserverPointer
void setBindingToNotify_unsafe(QPropertyBindingPrivate *binding);
void setChangeHandler(QPropertyObserver::ChangeHandler changeHandler);

enum class Notify {Everything, OnlyChangeHandlers};

template<Notify notifyPolicy = Notify::Everything>
void notify(QUntypedPropertyData *propertyDataPtr);
void notifyOnlyChangeHandler(QUntypedPropertyData *propertyDataPtr);
#ifndef QT_NO_DEBUG
void noSelfDependencies(QPropertyBindingPrivate *binding);
#else
void noSelfDependencies(QPropertyBindingPrivate *) {}
#endif
void evaluateBindings(QBindingStatus *status);
void evaluateBindings(PendingBindingObserverList &bindingObservers, QBindingStatus *status);
void observeProperty(QPropertyBindingDataPointer property);

explicit operator bool() const { return ptr != nullptr; }

QPropertyObserverPointer nextObserver() const { return {ptr->next.data()}; }

QPropertyBindingPrivate *binding() const
{
Q_ASSERT(ptr->next.tag() == QPropertyObserver::ObserverNotifiesBinding);
return ptr->binding;
};

private:
void unlink_common()
{
Expand Down Expand Up @@ -321,10 +360,21 @@ class Q_CORE_EXPORT QPropertyBindingPrivate : public QtPrivate::RefCounted

void unlinkAndDeref();

void evaluateRecursive(QBindingStatus *status = nullptr);
void Q_ALWAYS_INLINE evaluateRecursive_inline(QBindingStatus *status);
void evaluateRecursive(PendingBindingObserverList &bindingObservers, QBindingStatus *status = nullptr);

// ### TODO: remove as soon as declarative no longer needs this overload
void evaluateRecursive()
{
PendingBindingObserverList bindingObservers;
evaluateRecursive(bindingObservers);
}

void Q_ALWAYS_INLINE evaluateRecursive_inline(PendingBindingObserverList &bindingObservers, QBindingStatus *status);

void notifyRecursive();
void notifyNonRecursive(const PendingBindingObserverList &bindingObservers);
enum NotificationState : bool { Delayed, Sent };
NotificationState notifyNonRecursive();

static QPropertyBindingPrivate *get(const QUntypedPropertyBinding &binding)
{ return static_cast<QPropertyBindingPrivate *>(binding.d.data()); }
Expand Down Expand Up @@ -566,11 +616,14 @@ class QObjectCompatProperty : public QPropertyData<T>
QPropertyBindingDataPointer d{bd};
if (QPropertyObserverPointer observer = d.firstObserver()) {
if (!inBindingWrapper(storage)) {
if (bd->notifyObserver_helper(this, observer, storage)
PendingBindingObserverList bindingObservers;
if (bd->notifyObserver_helper(this, storage, observer, bindingObservers)
== QtPrivate::QPropertyBindingData::Evaluated) {
// evaluateBindings() can trash the observers. We need to re-fetch here.
if (QPropertyObserverPointer observer = d.firstObserver())
observer.notify(this);
observer.notifyOnlyChangeHandler(this);
for (auto&& bindingObserver: bindingObservers)
bindingObserver.binding()->notifyNonRecursive();
}
}
}
Expand Down Expand Up @@ -727,7 +780,7 @@ struct QUntypedBindablePrivate
}
};

inline void QPropertyBindingPrivate::evaluateRecursive_inline(QBindingStatus *status)
inline void QPropertyBindingPrivate::evaluateRecursive_inline(PendingBindingObserverList &bindingObservers, QBindingStatus *status)
{
if (updating) {
error = QPropertyBindingError(QPropertyBindingError::BindingLoop);
Expand Down Expand Up @@ -766,9 +819,10 @@ inline void QPropertyBindingPrivate::evaluateRecursive_inline(QBindingStatus *st
return;

firstObserver.noSelfDependencies(this);
firstObserver.evaluateBindings(status);
firstObserver.evaluateBindings(bindingObservers, status);
}

template<QPropertyObserverPointer::Notify notifyPolicy>
inline void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr)
{
auto observer = const_cast<QPropertyObserver*>(ptr);
Expand Down Expand Up @@ -808,10 +862,12 @@ inline void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataP
}
case QPropertyObserver::ObserverNotifiesBinding:
{
auto bindingToNotify = observer->binding;
QPropertyObserverNodeProtector protector(observer);
bindingToNotify->notifyRecursive();
next = protector.next();
if constexpr (notifyPolicy == Notify::Everything) {
auto bindingToNotify = observer->binding;
QPropertyObserverNodeProtector protector(observer);
bindingToNotify->notifyRecursive();
next = protector.next();
}
break;
}
case QPropertyObserver::ObserverIsPlaceholder:
Expand All @@ -825,12 +881,29 @@ inline void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataP
}
}

inline void QPropertyObserverPointer::notifyOnlyChangeHandler(QUntypedPropertyData *propertyDataPtr)
{
notify<Notify::OnlyChangeHandlers>(propertyDataPtr);
}

inline QPropertyObserverNodeProtector::~QPropertyObserverNodeProtector()
{
QPropertyObserverPointer d{static_cast<QPropertyObserver *>(&m_placeHolder)};
d.unlink_fast();
}

QBindingObserverPtr::QBindingObserverPtr(QPropertyObserver *observer) : d(observer)
{
Q_ASSERT(d);
QPropertyObserverPointer{d}.binding()->addRef();
}

QBindingObserverPtr::~QBindingObserverPtr() { if (d) QPropertyObserverPointer{d}.binding()->deref(); }

QPropertyBindingPrivate *QBindingObserverPtr::binding() const { return QPropertyObserverPointer{d}.binding(); }

QPropertyObserver *QBindingObserverPtr::operator->() { return d; }

QT_END_NAMESPACE

#endif // QPROPERTY_P_H
10 changes: 8 additions & 2 deletions src/corelib/kernel/qpropertyprivate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <QtCore/qglobal.h>
#include <QtCore/qtaggedpointer.h>
#include <QtCore/qmetatype.h>
#include <QtCore/qcontainerfwd.h>

#include <functional>

Expand All @@ -28,6 +29,9 @@ class QBindingStorage;
template<typename Class, typename T, auto Offset, auto Setter, auto Signal, auto Getter>
class QObjectCompatProperty;

struct QBindingObserverPtr;
using PendingBindingObserverList = QVarLengthArray<QBindingObserverPtr>;

namespace QtPrivate {
// QPropertyBindingPrivatePtr operates on a RefCountingMixin solely so that we can inline
// the constructor and copy constructor
Expand Down Expand Up @@ -115,6 +119,7 @@ class QPropertyBindingPrivatePtr
class QUntypedPropertyBinding;
class QPropertyBindingPrivate;
struct QPropertyBindingDataPointer;
class QPropertyObserver;
struct QPropertyObserverPointer;

class QUntypedPropertyData
Expand Down Expand Up @@ -308,8 +313,9 @@ class Q_CORE_EXPORT QPropertyBindingData

enum NotificationResult { Delayed, Evaluated };
NotificationResult notifyObserver_helper(
QUntypedPropertyData *propertyDataPtr, QPropertyObserverPointer observer,
QBindingStorage *storage) const;
QUntypedPropertyData *propertyDataPtr, QBindingStorage *storage,
QPropertyObserverPointer observer,
PendingBindingObserverList &bindingObservers) const;
};

template <typename T, typename Tag>
Expand Down
Loading

0 comments on commit f1b1773

Please sign in to comment.