Skip to content

Commit

Permalink
Simplify the safeguarding logic in notify()
Browse files Browse the repository at this point in the history 
The logic in notify() was doing quite a bit more work than
it needed to. By inserting a dummy node after the current one instead of
replacing it, we can avoid half of the data shuffling that has been
happening and also don't need a back pointer when executing the
notification.

Also avoid calling a semi expensive destructor of QPropertyObserver.

Reduces the overhead of notify() by ~30%.

Change-Id: I7ce16bcf9cd9c4368c18bf875fc959223452fd4f
Reviewed-by: Fabian Kosmale <[email protected]>
Reviewed-by: Qt CI Bot <[email protected]>
(cherry picked from commit e8ef871)
Reviewed-by: Ulf Hermann <[email protected]>
  • Loading branch information
@laknoll @Inkane
laknoll authored and Inkane committed Nov 30, 2020
1 parent d555425 commit 1f1a322
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 118 deletions.
143 changes: 53 additions & 90 deletions src/corelib/kernel/qproperty.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -398,10 +398,6 @@ void QPropertyObserver::setSource(const QPropertyBindingData &property)

QPropertyObserver::~QPropertyObserver()
{
if (next.tag() == ActivelyExecuting) {
if (nodeState)
*nodeState = nullptr;
}
QPropertyObserverPointer d{this};
d.unlink();
}
Expand All @@ -415,8 +411,6 @@ QPropertyObserver::QPropertyObserver(QPropertyObserver &&other) noexcept
next->prev = &next;
if (prev)
prev.setPointer(this);
if (next.tag() == ActivelyExecuting)
*nodeState = this;
}

QPropertyObserver &QPropertyObserver::operator=(QPropertyObserver &&other) noexcept
Expand All @@ -435,8 +429,6 @@ QPropertyObserver &QPropertyObserver::operator=(QPropertyObserver &&other) noexc
next->prev = &next;
if (prev)
prev.setPointer(this);
if (next.tag() == ActivelyExecuting)
*nodeState = this;

return *this;
}
Expand All @@ -455,21 +447,21 @@ void QPropertyObserverPointer::unlink()

void QPropertyObserverPointer::setChangeHandler(QPropertyObserver::ChangeHandler changeHandler)
{
Q_ASSERT(ptr->next.tag() != QPropertyObserver::ActivelyExecuting);
Q_ASSERT(ptr->next.tag() != QPropertyObserver::ObserverIsPlaceholder);
ptr->changeHandler = changeHandler;
ptr->next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler);
}

void QPropertyObserverPointer::setAliasedProperty(QUntypedPropertyData *property)
{
Q_ASSERT(ptr->next.tag() != QPropertyObserver::ActivelyExecuting);
Q_ASSERT(ptr->next.tag() != QPropertyObserver::ObserverIsPlaceholder);
ptr->aliasedPropertyData = property;
ptr->next.setTag(QPropertyObserver::ObserverNotifiesAlias);
}

void QPropertyObserverPointer::setBindingToMarkDirty(QPropertyBindingPrivate *binding)
{
Q_ASSERT(ptr->next.tag() != QPropertyObserver::ActivelyExecuting);
Q_ASSERT(ptr->next.tag() != QPropertyObserver::ObserverIsPlaceholder);
ptr->bindingToMarkDirty = binding;
ptr->next.setTag(QPropertyObserver::ObserverNotifiesBinding);
}
Expand All @@ -479,55 +471,26 @@ void QPropertyObserverPointer::setBindingToMarkDirty(QPropertyBindingPrivate *bi
QPropertyObserverNodeProtector is a RAII wrapper which takes care of the internal switching logic
for QPropertyObserverPointer::notify (described ibidem)
*/
template <QPropertyObserver::ObserverTag tag>
struct [[nodiscard]] QPropertyObserverNodeProtector {
QPropertyObserver m_placeHolder;
QPropertyObserver *&m_observer;
union {
QPropertyBindingPrivate *m_binding;
QPropertyObserver::ChangeHandler m_changeHandler;
};
QPropertyObserverNodeProtector(QPropertyObserver *&observer)
: m_observer(observer)
QPropertyObserverBase m_placeHolder;
QPropertyObserverNodeProtector(QPropertyObserver *observer)
{
static_assert(tag == QPropertyObserver::ObserverNotifiesBinding ||
tag == QPropertyObserver::ObserverNotifiesChangeHandler);
if constexpr (tag == QPropertyObserver::ObserverNotifiesBinding)
m_binding = m_observer->bindingToMarkDirty;
else
m_changeHandler = m_observer->changeHandler;
switchNodes(m_placeHolder, m_observer);
m_observer->nodeState = &m_observer;
m_observer->next.setTag(QPropertyObserver::ActivelyExecuting);
m_placeHolder.next.setTag(QPropertyObserver::ActivelyExecuting);
// insert m_placeholder after observer into the linked list
QPropertyObserver *next = observer->next.data();
m_placeHolder.next = next;
observer->next = static_cast<QPropertyObserver *>(&m_placeHolder);
if (next)
next->prev = &m_placeHolder.next;
m_placeHolder.prev = &observer->next;
m_placeHolder.next.setTag(QPropertyObserver::ObserverIsPlaceholder);
}

QPropertyObserver *next() const { return m_placeHolder.next.data(); }

~QPropertyObserverNodeProtector() {
if (m_observer) {
if constexpr (tag == QPropertyObserver::ObserverNotifiesBinding)
m_observer->bindingToMarkDirty = m_binding;
else
m_observer->changeHandler = m_changeHandler;
switchNodes(*m_observer, &m_placeHolder);
m_observer->next.setTag(tag);
}
// set tag to a safer value where we don't execute anything in the dtor
m_placeHolder.next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler);
QPropertyObserverPointer d{static_cast<QPropertyObserver *>(&m_placeHolder)};
d.unlink();
}

/*!
\internal
replaces a node \a observer in the list with another node \a placeholder which must not be in the list
*/
static void switchNodes(QPropertyObserver &placeHolder, QPropertyObserver *observer) {
placeHolder.next = std::exchange(observer->next, {});
placeHolder.prev = std::exchange(observer->prev, {});
if (placeHolder.next) {
placeHolder.next->prev = &placeHolder.next;
}
if (placeHolder.prev)
placeHolder.prev.setPointer(&placeHolder);
};
};

/*! \internal
Expand All @@ -539,7 +502,7 @@ struct [[nodiscard]] QPropertyObserverNodeProtector {
ObserverNotifiesChangeHandler case would not work. Thus we instead pass the knowledge of
whether the value has changed we obtained when evaluating the binding eagerly along
*/
void QPropertyObserverPointer::notify(QPropertyBindingPrivate *triggeringBinding, QUntypedPropertyData *propertyDataPtr,bool alreadyKnownToHaveChanged)
void QPropertyObserverPointer::notify(QPropertyBindingPrivate *triggeringBinding, QUntypedPropertyData *propertyDataPtr, bool alreadyKnownToHaveChanged)
{
bool knownIfPropertyChanged = alreadyKnownToHaveChanged;
bool propertyChanged = true;
Expand All @@ -551,56 +514,56 @@ void QPropertyObserverPointer::notify(QPropertyBindingPrivate *triggeringBinding
* However, complication arise due to the fact that the triggered operations might modify the list,
* which includes deletion and move of the current and next nodes.
* Therefore, we take a few safety precautions:
* 1. Before executing any action which might modify the list, we replace the actual node in the list with
* a placeholder node. As that one is stack allocated and owned by us, we can rest assured that it is
* 1. Before executing any action which might modify the list, we insert a placeholder node after the current node.
* As that one is stack allocated and owned by us, we can rest assured that it is
* still there after the action has executed, and placeHolder->next points to the actual next node in the list.
* Note that taking next at the beginning of the loop does not work, as the execuated action might either move
* or delete that node.
* 2. To properly handle deletion or moves of the real current node, we store a pointer to a pointer to itself in
* its nodeState. Whenever the node is reallocated and moved, we update that pointer to point to its new
* location. If the node is actually deleted, we set it to nullptr.
* 3. After the triggered action has finished, we can use that information to restore the list to contain the actual
* node again. We either switch the nodes with the real nodes current location, or, if the real node has been
* deleted, we simply unlink the temporary node.
* 2. After the triggered action has finished, we can use the next pointer in the placeholder node as a safe way to
* retrieve the next node.
* 3. Some care needs to be taken to avoid infinite recursion with change handlers, so we add an extra test there, that
* checks whether we're already have the same change handler in our call stack. This can be done by checking whether
* the node after the current one is a placeholder node.
*/
while (observer) {
QPropertyObserver *next = nullptr;
QPropertyObserver *next = observer->next.data();

char preventBug[1] = {'\0'}; // QTBUG-87245
Q_UNUSED(preventBug);
switch (observer->next.tag()) {
switch (QPropertyObserver::ObserverTag(observer->next.tag())) {
case QPropertyObserver::ObserverNotifiesChangeHandler:
if (auto handlerToCall = observer->changeHandler) {
// both evaluateIfDirtyAndReturnTrueIfValueChanged and handlerToCall might modify the list
QPropertyObserverNodeProtector<QPropertyObserver::ObserverNotifiesChangeHandler> protector(observer);
if (!knownIfPropertyChanged && triggeringBinding) {
knownIfPropertyChanged = true;
propertyChanged = triggeringBinding->evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr);
}
if (!propertyChanged)
return;
handlerToCall(observer, propertyDataPtr);
next = protector.m_placeHolder.next.data();
} else {
next = observer->next.data();
{
auto handlerToCall = observer->changeHandler;
// prevent recursion
if (next && next->next.tag() == QPropertyObserver::ObserverIsPlaceholder) {
observer = next->next.data();
continue;
}
// both evaluateIfDirtyAndReturnTrueIfValueChanged and handlerToCall might modify the list
QPropertyObserverNodeProtector protector(observer);
if (!knownIfPropertyChanged && triggeringBinding) {
knownIfPropertyChanged = true;
propertyChanged = triggeringBinding->evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr);
}
if (!propertyChanged)
return;
handlerToCall(observer, propertyDataPtr);
next = protector.next();
break;
}
case QPropertyObserver::ObserverNotifiesBinding:
if (auto bindingToMarkDirty = observer->bindingToMarkDirty) {
QPropertyObserverNodeProtector<QPropertyObserver::ObserverNotifiesBinding> protector(observer);
bindingToMarkDirty->markDirtyAndNotifyObservers();
next = protector.m_placeHolder.next.data();
} else {
next = observer->next.data();
}
{
auto bindingToMarkDirty = observer->bindingToMarkDirty;
QPropertyObserverNodeProtector protector(observer);
bindingToMarkDirty->markDirtyAndNotifyObservers();
next = protector.next();
break;
}
case QPropertyObserver::ObserverNotifiesAlias:
next = observer->next.data();
break;
case QPropertyObserver::ActivelyExecuting:
case QPropertyObserver::ObserverIsPlaceholder:
// recursion is already properly handled somewhere else
return;
default:
Q_UNREACHABLE();
break;
}
observer = next;
}
Expand Down
48 changes: 27 additions & 21 deletions src/corelib/kernel/qproperty.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,19 +205,43 @@ namespace Qt {

struct QPropertyObserverPrivate;
struct QPropertyObserverPointer;
class QPropertyObserver;

class Q_CORE_EXPORT QPropertyObserver
class QPropertyObserverBase
{
public:
// Internal
enum ObserverTag {
ObserverNotifiesBinding, // observer was installed to notify bindings that obsverved property changed
ObserverNotifiesChangeHandler, // observer is a change handler, which runs on every change
ObserverNotifiesAlias, // used for QPropertyAlias
ActivelyExecuting // the observer is currently evaluated in QPropertyObserver::notifyObservers or its
// placeholder. We only can store 4 different values, therefore those two conflate
ObserverIsPlaceholder // the observer before this one is currently evaluated in QPropertyObserver::notifyObservers.
};
protected:
using ChangeHandler = void (*)(QPropertyObserver*, QUntypedPropertyData *);

private:
friend struct QPropertyObserverNodeProtector;
friend class QPropertyObserver;
friend struct QPropertyObserverPointer;
friend struct QPropertyBindingDataPointer;
friend class QPropertyBindingPrivate;

QTaggedPointer<QPropertyObserver, ObserverTag> next;
// prev is a pointer to the "next" element within the previous node, or to the "firstObserverPtr" if it is the
// first node.
QtPrivate::QTagPreservingPointerToPointer<QPropertyObserver, ObserverTag> prev;

union {
QPropertyBindingPrivate *bindingToMarkDirty = nullptr;
ChangeHandler changeHandler;
QUntypedPropertyData *aliasedPropertyData;
};
};

class Q_CORE_EXPORT QPropertyObserver : public QPropertyObserverBase
{
public:
constexpr QPropertyObserver() = default;
QPropertyObserver(QPropertyObserver &&other) noexcept;
QPropertyObserver &operator=(QPropertyObserver &&other) noexcept;
Expand All @@ -229,7 +253,6 @@ class Q_CORE_EXPORT QPropertyObserver
void setSource(const QtPrivate::QPropertyBindingData &property);

protected:
using ChangeHandler = void (*)(QPropertyObserver*, QUntypedPropertyData *);
QPropertyObserver(ChangeHandler changeHandler);
QPropertyObserver(QUntypedPropertyData *aliasedPropertyPtr);

Expand All @@ -240,26 +263,9 @@ class Q_CORE_EXPORT QPropertyObserver

private:

QTaggedPointer<QPropertyObserver, ObserverTag> next;
// prev is a pointer to the "next" element within the previous node, or to the "firstObserverPtr" if it is the
// first node.
QtPrivate::QTagPreservingPointerToPointer<QPropertyObserver, ObserverTag> prev;

union {
QPropertyBindingPrivate *bindingToMarkDirty = nullptr;
ChangeHandler changeHandler;
QUntypedPropertyData *aliasedPropertyData;
QPropertyObserver **nodeState;
};

QPropertyObserver(const QPropertyObserver &) = delete;
QPropertyObserver &operator=(const QPropertyObserver &) = delete;

friend struct QPropertyObserverPointer;
friend struct QPropertyBindingDataPointer;
friend class QPropertyBindingPrivate;
template<ObserverTag>
friend struct QPropertyObserverNodeProtector;
};

template <typename Functor>
Expand Down
7 changes: 0 additions & 7 deletions src/corelib/kernel/qproperty_p.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,13 +245,6 @@ class Q_CORE_EXPORT QPropertyBindingPrivate : public QtPrivate::RefCounted
void clearDependencyObservers() {
for (size_t i = 0; i < qMin(dependencyObserverCount, inlineDependencyObservers.size()); ++i) {
QPropertyObserverPointer p{&inlineDependencyObservers[i]};
if (p.ptr->next.tag() == QPropertyObserver::ActivelyExecuting) {
*(p.ptr->nodeState) = nullptr;
p.ptr->nodeState = nullptr;

// set tag to "safer" value, as we return the same observer pointer from allocateDependencyObserver
p.ptr->next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler);
}
p.unlink();
}
if (heapObservers)
Expand Down

0 comments on commit 1f1a322

Please sign in to comment.