From 0da8af84b8ed6028c84a88ae3c63d642dcbe412c Mon Sep 17 00:00:00 2001 From: Alexey Edelev Date: Wed, 10 Jul 2024 22:14:05 +0200 Subject: [PATCH] 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 --- src/protobuf/qprotobufjsonserializer.cpp | 65 +++++++++++++++---- src/protobuf/qprotobufserializer.cpp | 83 ++++++++++++++++++------ src/protobuf/qprotobufserializer_p.h | 5 ++ 3 files changed, 120 insertions(+), 33 deletions(-) diff --git a/src/protobuf/qprotobufjsonserializer.cpp b/src/protobuf/qprotobufjsonserializer.cpp index 8b23e64f..933bc948 100644 --- a/src/protobuf/qprotobufjsonserializer.cpp +++ b/src/protobuf/qprotobufjsonserializer.cpp @@ -538,7 +538,8 @@ public: if (metaType.flags() & QMetaType::IsPointer) { auto *messageProperty = propertyData.value(); 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(); - newPropertyValue.setValue(deserializeList(activeValue, - ok)); + QMetaType originalMetatype = cachedPropertyValue.metaType(); + cachedPropertyValue + .setValue(deserializeList(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) - message->setProperty(iter->second, newPropertyValue); + if (!ok) { + 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); } diff --git a/src/protobuf/qprotobufserializer.cpp b/src/protobuf/qprotobufserializer.cpp index 9c970afc..5a0cc3a1 100644 --- a/src/protobuf/qprotobufserializer.cpp +++ b/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)) - return false; + if (!d_ptr->deserializeProperty(message)) { + 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)) - return false; + if (!deserializeProperty(message)) { + 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); - QMetaType metaType = newPropertyValue.metaType(); + if (cachedIndex != index) { + 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(); + auto *messageProperty = cachedPropertyValue.value(); Q_ASSERT(messageProperty != nullptr); - if (deserializeObject(messageProperty)) - return message->setProperty(fieldInfo, std::move(newPropertyValue)); - return false; + return deserializeObject(messageProperty); } const auto fieldFlags = fieldInfo.getFieldFlags(); if (fieldFlags.testFlag(QtProtobufPrivate::FieldFlag::Enum)) { if (fieldFlags.testFlag(QtProtobufPrivate::FieldFlag::Repeated)) { - auto intList = newPropertyValue.value>(); + auto intList = cachedPropertyValue.value>(); if (deserializeEnumList(intList)) { - newPropertyValue.setValue(intList); - return message->setProperty(fieldInfo, std::move(newPropertyValue)); + cachedPropertyValue.setValue(intList); + return true; } return false; } else { - if (deserializeBasic(it, newPropertyValue)) - return message->setProperty(fieldInfo, std::move(newPropertyValue)); + if (deserializeBasic(it, cachedPropertyValue)) + 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 diff --git a/src/protobuf/qprotobufserializer_p.h b/src/protobuf/qprotobufserializer_p.h index 4300614a..919ce021 100644 --- a/src/protobuf/qprotobufserializer_p.h +++ b/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;