Cache property values when deserializing them

When deserializing protobuf messages, cache the property values in
the serializer. The cached value life-time is limited by field
number change. This improves the performance of the deserializer
when deserializing repeated fields. So before storing the new value
into the message we first collect all values of the repeated field
that we received from the wire and only when we recognize the change
of the field in the byte stream, update the property value.

Fixes: QTBUG-124594
Pick-to: 6.8 6.7
Change-Id: I5ce5d5c81bf0f7ebf071b5ad374a46787b2767e8
Reviewed-by: Dennis Oberst <dennis.oberst@qt.io>

src/protobuf/qprotobufjsonserializer.cpp

@@ -538,7 +538,8 @@ public:
         if (metaType.flags() & QMetaType::IsPointer) {
             auto *messageProperty = propertyData.value<QProtobufMessage *>();
             Q_ASSERT(messageProperty != nullptr);
-            return deserializeObject(messageProperty);
+            ok = deserializeObject(messageProperty);
+            return propertyData;
         }
 
         auto handler = QtProtobufPrivate::findHandler(metaType);
@@ -593,6 +594,15 @@ public:
     {
         Q_ASSERT(message != nullptr);
 
+        auto restoreOnReturn = qScopeGuard([prevCachedPropertyValue = cachedPropertyValue,
+                                            prevCachedIndex = cachedIndex, this]() {
+            cachedPropertyValue = prevCachedPropertyValue;
+            cachedIndex = prevCachedIndex;
+        });
+
+        cachedPropertyValue.clear();
+        cachedIndex = -1;
+
         auto ordering = message->propertyOrdering();
         Q_ASSERT(ordering != nullptr);
 
@@ -626,21 +636,28 @@ public:
                 auto store = activeValue;
                 activeValue = activeObject.value(key);
 
-                QVariant newPropertyValue = message->property(iter->second, true);
+                if (auto index = ordering->indexOfFieldNumber(iter->second.getFieldNumber());
+                    index != cachedIndex) {
+                    if (!storeCachedValue(message))
+                        return false;
+                    cachedPropertyValue = message->property(iter->second, true);
+                    cachedIndex = index;
+                }
+
                 bool ok = false;
 
                 const auto fieldFlags = iter->second.getFieldFlags();
                 if (fieldFlags & QtProtobufPrivate::FieldFlag::Enum) {
                     if (fieldFlags & QtProtobufPrivate::FieldFlag::Repeated) {
-                        QMetaType originalMetatype = newPropertyValue.metaType();
+                        QMetaType originalMetatype = cachedPropertyValue.metaType();
-                        newPropertyValue.setValue(deserializeList<QStringList, QString>(activeValue,
+                        cachedPropertyValue
-                                                                                        ok));
+                            .setValue(deserializeList<QStringList, QString>(activeValue, ok));
                         if (ok)
-                            ok = newPropertyValue.convert(originalMetatype);
+                            ok = cachedPropertyValue.convert(originalMetatype);
                     } else {
-                        const auto metaEnum = getMetaEnum(newPropertyValue.metaType());
+                        const auto metaEnum = getMetaEnum(cachedPropertyValue.metaType());
                         Q_ASSERT(metaEnum.isValid());
-                        newPropertyValue.setValue(deserializeEnum(activeValue, metaEnum, ok));
+                        cachedPropertyValue.setValue(deserializeEnum(activeValue, metaEnum, ok));
                     }
                     if (!ok)
                         setInvalidFormatError();
@@ -651,15 +668,17 @@ public:
                         mapObj.insert("key"_L1, it.key());
                         mapObj.insert("value"_L1, it.value());
                         activeValue = mapObj;
-                        newPropertyValue = deserializeValue(newPropertyValue, ok);
+                        cachedPropertyValue = deserializeValue(cachedPropertyValue, ok);
                     }
                     activeValue = store;
                 } else {
-                    newPropertyValue = deserializeValue(newPropertyValue, ok);
+                    cachedPropertyValue = deserializeValue(cachedPropertyValue, ok);
                     activeValue = store;
                 }
-                if (ok)
+                if (!ok) {
-                    message->setProperty(iter->second, newPropertyValue);
+                    cachedPropertyValue.clear();
+                    cachedIndex = -1;
+                }
             }
         }
 
@@ -667,7 +686,7 @@ public:
         // to deserialize
         activeValue = {};
 
-        return true;
+        return storeCachedValue(message);
     }
 
     void setDeserializationError(QAbstractProtobufSerializer::DeserializationError error,
@@ -701,10 +720,28 @@ public:
 
     static SerializerRegistry handlers;
 
+    [[nodiscard]] bool storeCachedValue(QProtobufMessage *message);
+
+    QVariant cachedPropertyValue;
+    int cachedIndex = -1;
+
 private:
     QProtobufJsonSerializer *qPtr;
 };
 
+bool QProtobufJsonSerializerPrivate::storeCachedValue(QProtobufMessage *message)
+{
+    bool ok = true;
+    if (cachedIndex >= 0 && !cachedPropertyValue.isNull()) {
+        const auto *ordering = message->propertyOrdering();
+        QProtobufFieldInfo fieldInfo(*ordering, cachedIndex);
+        ok = message->setProperty(fieldInfo, cachedPropertyValue);
+        cachedPropertyValue.clear();
+        cachedIndex = -1;
+    }
+    return ok;
+}
+
 QProtobufJsonSerializerPrivate::SerializerRegistry QProtobufJsonSerializerPrivate::handlers = {};
 
 void QProtobufJsonSerializerPrivate::clearError()
@@ -759,6 +796,8 @@ bool QProtobufJsonSerializer::deserializeMessage(QProtobufMessage *message,
     }
     d_ptr->activeValue = document.object();
 
+    d_ptr->cachedPropertyValue.clear();
+    d_ptr->cachedIndex = -1;
     return d_ptr->deserializeObject(message);
 }
 

src/protobuf/qprotobufserializer.cpp

@@ -251,12 +251,22 @@ void QProtobufSerializerPrivate::clearError()
 
 bool QProtobufSerializer::deserializeMessage(QProtobufMessage *message, QByteArrayView data) const
 {
+    d_ptr->cachedPropertyValue = QVariant();
+    d_ptr->cachedIndex = -1;
     d_ptr->clearError();
     d_ptr->it = QProtobufSelfcheckIterator::fromView(data);
+
+    bool ok = true;
     while (d_ptr->it.isValid() && d_ptr->it != data.end()) {
-        if (!d_ptr->deserializeProperty(message))
+        if (!d_ptr->deserializeProperty(message)) {
-            return false;
+            ok = false;
+            break;
+        }
     }
+
+    if (!d_ptr->storeCachedValue(message) || !ok)
+        return false;
+
     if (!d_ptr->it.isValid())
         d_ptr->setUnexpectedEndOfStreamError();
     return d_ptr->it.isValid();
@@ -287,15 +297,31 @@ bool QProtobufSerializerPrivate::deserializeObject(QProtobufMessage *message)
         setUnexpectedEndOfStreamError();
         return false;
     }
-    auto prevIt = it;
+
-    auto restoreOnReturn = qScopeGuard([&](){ it = prevIt; });
+    auto restoreOnReturn = qScopeGuard([prevIt = it, prevCachedPropertyValue = cachedPropertyValue,
+                                        prevCachedIndex = cachedIndex, this]() {
+        it = prevIt;
+        cachedPropertyValue = prevCachedPropertyValue;
+        cachedIndex = prevCachedIndex;
+    });
+
+    cachedPropertyValue.clear();
+    cachedIndex = -1;
+
     QByteArrayView data = *array;
     clearError();
     it = QProtobufSelfcheckIterator::fromView(data);
+    bool ok = true;
     while (it.isValid() && it != data.end()) {
-        if (!deserializeProperty(message))
+        if (!deserializeProperty(message)) {
-            return false;
+            ok = false;
+            break;
+        }
     }
+
+    if (!storeCachedValue(message) || !ok)
+        return false;
+
     if (!it.isValid())
         setUnexpectedEndOfStreamError();
     return it.isValid();
@@ -550,32 +576,36 @@ bool QProtobufSerializerPrivate::deserializeProperty(QProtobufMessage *message)
     }
 
     QProtobufFieldInfo fieldInfo(*ordering, index);
-    QVariant newPropertyValue = message->property(fieldInfo, true);
+    if (cachedIndex != index) {
-    QMetaType metaType = newPropertyValue.metaType();
+        if (!storeCachedValue(message))
+            return false;
+
+        cachedPropertyValue = message->property(fieldInfo, true);
+        cachedIndex = index;
+    }
+    QMetaType metaType = cachedPropertyValue.metaType();
 
     qProtoDebug() << "wireType:" << wireType << "metaType:" << metaType.name()
                   << "currentByte:" << QString::number((*it), 16);
 
     if (metaType.flags() & QMetaType::IsPointer) {
-        auto *messageProperty = newPropertyValue.value<QProtobufMessage *>();
+        auto *messageProperty = cachedPropertyValue.value<QProtobufMessage *>();
         Q_ASSERT(messageProperty != nullptr);
-        if (deserializeObject(messageProperty))
+        return deserializeObject(messageProperty);
-            return message->setProperty(fieldInfo, std::move(newPropertyValue));
-        return false;
     }
 
     const auto fieldFlags = fieldInfo.getFieldFlags();
     if (fieldFlags.testFlag(QtProtobufPrivate::FieldFlag::Enum)) {
         if (fieldFlags.testFlag(QtProtobufPrivate::FieldFlag::Repeated)) {
-            auto intList = newPropertyValue.value<QList<QtProtobuf::int64>>();
+            auto intList = cachedPropertyValue.value<QList<QtProtobuf::int64>>();
             if (deserializeEnumList(intList)) {
-                newPropertyValue.setValue(intList);
+                cachedPropertyValue.setValue(intList);
-                return message->setProperty(fieldInfo, std::move(newPropertyValue));
+                return true;
             }
             return false;
         } else {
-            if (deserializeBasic<QtProtobuf::int64>(it, newPropertyValue))
+            if (deserializeBasic<QtProtobuf::int64>(it, cachedPropertyValue))
-                return message->setProperty(fieldInfo, std::move(newPropertyValue));
+                return true;
             return false;
         }
     }
@@ -608,7 +638,7 @@ bool QProtobufSerializerPrivate::deserializeProperty(QProtobufMessage *message)
             }
         }
 
-        if (!basicHandler->deserializer(it, newPropertyValue)) {
+        if (!basicHandler->deserializer(it, cachedPropertyValue)) {
             setUnexpectedEndOfStreamError();
             return false;
         }
@@ -624,10 +654,23 @@ bool QProtobufSerializerPrivate::deserializeProperty(QProtobufMessage *message)
                 QCoreApplication::translate("QtProtobuf", error.toUtf8().data()));
             return false;
         }
-        handler.deserializer(q_ptr, newPropertyValue);
+        handler.deserializer(q_ptr, cachedPropertyValue);
     }
 
-    return message->setProperty(fieldInfo, std::move(newPropertyValue));
+    return true;
+}
+
+bool QProtobufSerializerPrivate::storeCachedValue(QProtobufMessage *message)
+{
+    bool ok = true;
+    if (cachedIndex >= 0 && !cachedPropertyValue.isNull()) {
+        const auto *ordering = message->propertyOrdering();
+        QProtobufFieldInfo fieldInfo(*ordering, cachedIndex);
+        ok = message->setProperty(fieldInfo, cachedPropertyValue);
+        cachedPropertyValue.clear();
+        cachedIndex = -1;
+    }
+    return ok;
 }
 
 QAbstractProtobufSerializer::DeserializationError QProtobufSerializer::deserializationError() const

src/protobuf/qprotobufserializer_p.h

@@ -520,6 +520,8 @@ public:
     void clearError();
     void setUnexpectedEndOfStreamError();
 
+    [[nodiscard]] bool storeCachedValue(QProtobufMessage *message);
+
     QAbstractProtobufSerializer::DeserializationError deserializationError =
             QAbstractProtobufSerializer::NoDeserializerError;
     QString deserializationErrorString;
@@ -531,6 +533,9 @@ public:
 
     static const QtProtobufPrivate::QProtobufFieldInfo mapValueOrdering;
 
+    QVariant cachedPropertyValue;
+    int cachedIndex = -1;
+
 private:
     Q_DISABLE_COPY_MOVE(QProtobufSerializerPrivate)
     QProtobufSerializer *q_ptr;