From bb44c18b673e2955592ee86774eb7cc25d390c17 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Thu, 4 Feb 2021 17:20:33 +0100 Subject: [PATCH] Don't emit change notifications more often than required When a property value changes, first update all dependent bindings to their new value. Only once that is done send out all the notifications and changed signals. This way, if a property depends on multiple other properties, which all get changed, there will only be one notification; and (potentially invalid) intermediate values will not be observed. Fixes: QTBUG-89844 Change-Id: I086077934aee6dc940705f08a87bf8448708881f Reviewed-by: Lars Knoll Reviewed-by: Fabian Kosmale Reviewed-by: Andreas Buhr Reviewed-by: Andrei Golubev --- src/corelib/kernel/qproperty.cpp | 70 ++++++++++++++++--- src/corelib/kernel/qproperty_p.h | 10 ++- .../kernel/qproperty/tst_qproperty.cpp | 31 ++++++++ 3 files changed, 99 insertions(+), 12 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index 1c642562a84..60f21d5b573 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -98,7 +98,7 @@ void QPropertyBindingPrivate::unlinkAndDeref() destroyAndFreeMemory(this); } -void QPropertyBindingPrivate::evaluate() +void QPropertyBindingPrivate::evaluateRecursive() { if (updating) { error = QPropertyBindingError(QPropertyBindingError::BindingLoop); @@ -121,23 +121,35 @@ void QPropertyBindingPrivate::evaluate() BindingEvaluationState evaluationFrame(this); - bool changed; auto bindingFunctor = reinterpret_cast(this) + QPropertyBindingPrivate::getSizeEnsuringAlignment(); if (hasBindingWrapper) { - changed = staticBindingWrapper(metaType, propertyDataPtr, + pendingNotify = staticBindingWrapper(metaType, propertyDataPtr, {vtable, bindingFunctor}); } else { - changed = vtable->call(metaType, propertyDataPtr, bindingFunctor); + pendingNotify = vtable->call(metaType, propertyDataPtr, bindingFunctor); } - if (!changed) + if (!pendingNotify || !firstObserver) return; - // notify dependent bindings and emit changed signal - if (firstObserver) + firstObserver.noSelfDependencies(this); + firstObserver.evaluateBindings(); +} + +void QPropertyBindingPrivate::notifyRecursive() +{ + if (!pendingNotify) + return; + pendingNotify = false; + Q_ASSERT(!updating); + updating = true; + if (firstObserver) { + firstObserver.noSelfDependencies(this); firstObserver.notify(propertyDataPtr); + } if (hasStaticObserver) staticObserverCallback(propertyDataPtr); + updating = false; } QUntypedPropertyBinding::QUntypedPropertyBinding() = default; @@ -248,7 +260,8 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB newBindingRaw->prependObserver(observer); newBindingRaw->setStaticObserver(staticObserverCallback, guardCallback); - newBindingRaw->evaluate(); + newBindingRaw->evaluateRecursive(); + newBindingRaw->notifyRecursive(); } else if (observer) { d.setObservers(observer.ptr); } else { @@ -343,8 +356,10 @@ void QPropertyBindingData::registerWithCurrentlyEvaluatingBinding_helper(Binding void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr) const { QPropertyBindingDataPointer d{this}; - if (QPropertyObserverPointer observer = d.firstObserver()) + if (QPropertyObserverPointer observer = d.firstObserver()) { + observer.evaluateBindings(); observer.notify(propertyDataPtr); + } } int QPropertyBindingDataPointer::observerCount() const @@ -518,9 +533,9 @@ void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr) } case QPropertyObserver::ObserverNotifiesBinding: { - auto bindingToMarkDirty = observer->binding; + auto bindingToNotify = observer->binding; QPropertyObserverNodeProtector protector(observer); - bindingToMarkDirty->evaluate(); + bindingToNotify->notifyRecursive(); next = protector.next(); break; } @@ -534,6 +549,39 @@ void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr) } } +#ifndef QT_NO_DEBUG +void QPropertyObserverPointer::noSelfDependencies(QPropertyBindingPrivate *binding) +{ + auto observer = const_cast(ptr); + // See also comment in notify() + while (observer) { + if (QPropertyObserver::ObserverTag(observer->next.tag()) == QPropertyObserver::ObserverNotifiesBinding) + Q_ASSERT(observer->binding != binding); + + observer = observer->next.data(); + } + +} +#endif + +void QPropertyObserverPointer::evaluateBindings() +{ + auto observer = const_cast(ptr); + // See also comment in notify() + while (observer) { + QPropertyObserver *next = observer->next.data(); + + if (QPropertyObserver::ObserverTag(observer->next.tag()) == QPropertyObserver::ObserverNotifiesBinding) { + auto bindingToEvaluate = observer->binding; + QPropertyObserverNodeProtector protector(observer); + bindingToEvaluate->evaluateRecursive(); + next = protector.next(); + } + + observer = next; + } +} + void QPropertyObserverPointer::observeProperty(QPropertyBindingDataPointer property) { if (ptr->prev) diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index 7ac02cb43d3..3535d7e843b 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -109,6 +109,12 @@ struct QPropertyObserverPointer void setAliasedProperty(QUntypedPropertyData *propertyPtr); void notify(QUntypedPropertyData *propertyDataPtr); +#ifndef QT_NO_DEBUG + void noSelfDependencies(QPropertyBindingPrivate *binding); +#else + void noSelfDependencies(QPropertyBindingPrivate *) {} +#endif + void evaluateBindings(); void observeProperty(QPropertyBindingDataPointer property); explicit operator bool() const { return ptr != nullptr; } @@ -175,6 +181,7 @@ private: // used to detect binding loops for lazy evaluated properties bool updating = false; bool hasStaticObserver = false; + bool pendingNotify = false; bool hasBindingWrapper:1; // used to detect binding loops for eagerly evaluated properties bool isQQmlPropertyBinding:1; @@ -306,7 +313,8 @@ public: void unlinkAndDeref(); - void evaluate(); + void evaluateRecursive(); + void notifyRecursive(); static QPropertyBindingPrivate *get(const QUntypedPropertyBinding &binding) { return static_cast(binding.d.data()); } diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index 12fd124b18c..8cc4fe486a6 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -95,6 +95,7 @@ private slots: void noFakeDependencies(); void bindablePropertyWithInitialization(); + void noDoubleNotification(); }; void tst_QProperty::functorBinding() @@ -1606,6 +1607,36 @@ void tst_QProperty::bindablePropertyWithInitialization() QCOMPARE(tester.prop3().anotherValue, 20); } +void tst_QProperty::noDoubleNotification() +{ + /* dependency graph for this test + x --> y means y depends on x + a-->b-->d + \ ^ + \->c--/ + */ + QProperty a(0); + QProperty b; + b.setBinding([&](){ return a.value(); }); + QProperty c; + c.setBinding([&](){ return a.value(); }); + QProperty d; + d.setBinding([&](){ return b.value() + c.value(); }); + int nNotifications = 0; + int expected = 0; + auto connection = d.subscribe([&](){ + ++nNotifications; + QCOMPARE(d.value(), expected); + }); + QCOMPARE(nNotifications, 1); + expected = 2; + a = 1; + QCOMPARE(nNotifications, 2); + expected = 4; + a = 2; + QCOMPARE(nNotifications, 3); +} + QTEST_MAIN(tst_QProperty); #include "tst_qproperty.moc"