From 98e0688e9859c5ef51f9f08743f4925df990ac41 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Wed, 8 Jul 2015 17:21:38 -0700 Subject: [PATCH 1/3] Reducing heap allocation in network packet parsing --- libraries/entities/src/EntityItem.cpp | 160 +++++++++++++++++------- libraries/entities/src/EntityItemID.cpp | 7 +- libraries/shared/src/BufferParser.h | 123 ++++++++++++++++++ 3 files changed, 240 insertions(+), 50 deletions(-) create mode 100644 libraries/shared/src/BufferParser.h diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 6c27152a97..1165518097 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -12,9 +12,11 @@ #include "EntityItem.h" #include +#include #include +#include #include #include #include @@ -354,50 +356,78 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef // ~27-35 bytes... const int MINIMUM_HEADER_BYTES = 27; - int bytesRead = 0; if (bytesLeftToRead < MINIMUM_HEADER_BYTES) { return 0; } + int clockSkew = args.sourceNode ? args.sourceNode->getClockSkewUsec() : 0; + + BufferParser parser(data, bytesLeftToRead); + +#ifdef DEBUG +#define VALIDATE_ENTITY_ITEM_PARSER 1 +#endif + +#ifdef VALIDATE_ENTITY_ITEM_PARSER + int bytesRead = 0; int originalLength = bytesLeftToRead; // TODO: figure out a way to avoid the big deep copy below. QByteArray originalDataBuffer((const char*)data, originalLength); // big deep copy! - - int clockSkew = args.sourceNode ? args.sourceNode->getClockSkewUsec() : 0; - const unsigned char* dataAt = data; +#endif // id - QByteArray encodedID = originalDataBuffer.mid(bytesRead, NUM_BYTES_RFC4122_UUID); // maximum possible size - _id = QUuid::fromRfc4122(encodedID); - dataAt += encodedID.size(); - bytesRead += encodedID.size(); - + parser.readUuid(_id); +#ifdef VALIDATE_ENTITY_ITEM_PARSER + { + QByteArray encodedID = originalDataBuffer.mid(bytesRead, NUM_BYTES_RFC4122_UUID); // maximum possible size + QUuid id = QUuid::fromRfc4122(encodedID); + dataAt += encodedID.size(); + bytesRead += encodedID.size(); + Q_ASSERT(id == _id); + Q_ASSERT(parser.offset() == bytesRead); + } +#endif + // type + parser.readCompressedCount((quint32&)_type); +#ifdef VALIDATE_ENTITY_ITEM_PARSER QByteArray encodedType = originalDataBuffer.mid(bytesRead); // maximum possible size ByteCountCoded typeCoder = encodedType; encodedType = typeCoder; // determine true length dataAt += encodedType.size(); bytesRead += encodedType.size(); quint32 type = typeCoder; - _type = (EntityTypes::EntityType)type; + EntityTypes::EntityType oldType = (EntityTypes::EntityType)type; + Q_ASSERT(oldType == _type); + Q_ASSERT(parser.offset() == bytesRead); +#endif bool overwriteLocalData = true; // assume the new content overwrites our local data + quint64 now = usecTimestampNow(); // _created - quint64 createdFromBuffer = 0; - memcpy(&createdFromBuffer, dataAt, sizeof(createdFromBuffer)); - dataAt += sizeof(createdFromBuffer); - bytesRead += sizeof(createdFromBuffer); - - quint64 now = usecTimestampNow(); - if (_created == UNKNOWN_CREATED_TIME) { - // we don't yet have a _created timestamp, so we accept this one - createdFromBuffer -= clockSkew; - if (createdFromBuffer > now || createdFromBuffer == UNKNOWN_CREATED_TIME) { - createdFromBuffer = now; + { + quint64 createdFromBuffer = 0; + parser.readValue(createdFromBuffer); +#ifdef VALIDATE_ENTITY_ITEM_PARSER + { + quint64 createdFromBuffer2 = 0; + memcpy(&createdFromBuffer2, dataAt, sizeof(createdFromBuffer2)); + dataAt += sizeof(createdFromBuffer2); + bytesRead += sizeof(createdFromBuffer2); + Q_ASSERT(createdFromBuffer2 == createdFromBuffer); + Q_ASSERT(parser.offset() == bytesRead); + } +#endif + if (_created == UNKNOWN_CREATED_TIME) { + // we don't yet have a _created timestamp, so we accept this one + createdFromBuffer -= clockSkew; + if (createdFromBuffer > now || createdFromBuffer == UNKNOWN_CREATED_TIME) { + createdFromBuffer = now; + } + _created = createdFromBuffer; } - _created = createdFromBuffer; } #ifdef WANT_DEBUG @@ -417,15 +447,21 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef #endif quint64 lastEditedFromBuffer = 0; - quint64 lastEditedFromBufferAdjusted = 0; // TODO: we could make this encoded as a delta from _created // _lastEdited - memcpy(&lastEditedFromBuffer, dataAt, sizeof(lastEditedFromBuffer)); - dataAt += sizeof(lastEditedFromBuffer); - bytesRead += sizeof(lastEditedFromBuffer); - lastEditedFromBufferAdjusted = lastEditedFromBuffer - clockSkew; - + parser.readValue(lastEditedFromBuffer); +#ifdef VALIDATE_ENTITY_ITEM_PARSER + { + quint64 lastEditedFromBuffer2 = 0; + memcpy(&lastEditedFromBuffer2, dataAt, sizeof(lastEditedFromBuffer2)); + dataAt += sizeof(lastEditedFromBuffer2); + bytesRead += sizeof(lastEditedFromBuffer2); + Q_ASSERT(lastEditedFromBuffer2 == lastEditedFromBuffer); + Q_ASSERT(parser.offset() == bytesRead); + } +#endif + quint64 lastEditedFromBufferAdjusted = lastEditedFromBuffer - clockSkew; if (lastEditedFromBufferAdjusted > now) { lastEditedFromBufferAdjusted = now; } @@ -487,9 +523,21 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef } // last updated is stored as ByteCountCoded delta from lastEdited - QByteArray encodedUpdateDelta = originalDataBuffer.mid(bytesRead); // maximum possible size - ByteCountCoded updateDeltaCoder = encodedUpdateDelta; - quint64 updateDelta = updateDeltaCoder; + quint64 updateDelta; + parser.readCompressedCount(updateDelta); +#ifdef VALIDATE_ENTITY_ITEM_PARSER + { + QByteArray encodedUpdateDelta = originalDataBuffer.mid(bytesRead); // maximum possible size + ByteCountCoded updateDeltaCoder = encodedUpdateDelta; + quint64 updateDelta2 = updateDeltaCoder; + Q_ASSERT(updateDelta == updateDelta2); + encodedUpdateDelta = updateDeltaCoder; // determine true length + dataAt += encodedUpdateDelta.size(); + bytesRead += encodedUpdateDelta.size(); + Q_ASSERT(parser.offset() == bytesRead); + } +#endif + if (overwriteLocalData) { _lastUpdated = lastEditedFromBufferAdjusted + updateDelta; // don't adjust for clock skew since we already did that #ifdef WANT_DEBUG @@ -499,17 +547,25 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef #endif } - encodedUpdateDelta = updateDeltaCoder; // determine true length - dataAt += encodedUpdateDelta.size(); - bytesRead += encodedUpdateDelta.size(); - // Newer bitstreams will have a last simulated and a last updated value quint64 lastSimulatedFromBufferAdjusted = now; if (args.bitstreamVersion >= VERSION_ENTITIES_HAS_LAST_SIMULATED_TIME) { // last simulated is stored as ByteCountCoded delta from lastEdited - QByteArray encodedSimulatedDelta = originalDataBuffer.mid(bytesRead); // maximum possible size - ByteCountCoded simulatedDeltaCoder = encodedSimulatedDelta; - quint64 simulatedDelta = simulatedDeltaCoder; + quint64 simulatedDelta; + parser.readCompressedCount(simulatedDelta); +#ifdef VALIDATE_ENTITY_ITEM_PARSER + { + QByteArray encodedSimulatedDelta = originalDataBuffer.mid(bytesRead); // maximum possible size + ByteCountCoded simulatedDeltaCoder = encodedSimulatedDelta; + quint64 simulatedDelta2 = simulatedDeltaCoder; + Q_ASSERT(simulatedDelta2 == simulatedDelta); + encodedSimulatedDelta = simulatedDeltaCoder; // determine true length + dataAt += encodedSimulatedDelta.size(); + bytesRead += encodedSimulatedDelta.size(); + Q_ASSERT(parser.offset() == bytesRead); + } +#endif + if (overwriteLocalData) { lastSimulatedFromBufferAdjusted = lastEditedFromBufferAdjusted + simulatedDelta; // don't adjust for clock skew since we already did that if (lastSimulatedFromBufferAdjusted > now) { @@ -521,9 +577,6 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef qCDebug(entities) << " lastSimulatedFromBufferAdjusted:" << debugTime(lastSimulatedFromBufferAdjusted, now); #endif } - encodedSimulatedDelta = simulatedDeltaCoder; // determine true length - dataAt += encodedSimulatedDelta.size(); - bytesRead += encodedSimulatedDelta.size(); } #ifdef WANT_DEBUG @@ -537,10 +590,26 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef // Property Flags - QByteArray encodedPropertyFlags = originalDataBuffer.mid(bytesRead); // maximum possible size - EntityPropertyFlags propertyFlags = encodedPropertyFlags; - dataAt += propertyFlags.getEncodedLength(); - bytesRead += propertyFlags.getEncodedLength(); + EntityPropertyFlags propertyFlags; + parser.readFlags(propertyFlags); +#ifdef VALIDATE_ENTITY_ITEM_PARSER + { + QByteArray encodedPropertyFlags = originalDataBuffer.mid(bytesRead); // maximum possible size + EntityPropertyFlags propertyFlags2 = encodedPropertyFlags; + dataAt += propertyFlags.getEncodedLength(); + bytesRead += propertyFlags.getEncodedLength(); + Q_ASSERT(propertyFlags2 == propertyFlags); + Q_ASSERT(parser.offset() == bytesRead); + } +#endif + +#ifdef VALIDATE_ENTITY_ITEM_PARSER + Q_ASSERT(parser.data() + parser.offset() == dataAt); +#else + const unsigned char* dataAt = parser.data() + parser.offset(); + int bytesRead = parser.offset(); +#endif + if (args.bitstreamVersion >= VERSION_ENTITIES_HAVE_SIMULATION_OWNER_AND_ACTIONS_OVER_WIRE) { // pack SimulationOwner and terse update properties near each other @@ -549,6 +618,7 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef // even when we would otherwise ignore the rest of the packet. if (propertyFlags.getHasProperty(PROP_SIMULATION_OWNER)) { + QByteArray simOwnerData; int bytes = OctreePacketData::unpackDataFromBytes(dataAt, simOwnerData); SimulationOwner newSimOwner; diff --git a/libraries/entities/src/EntityItemID.cpp b/libraries/entities/src/EntityItemID.cpp index 4e66f5d156..d882559ee3 100644 --- a/libraries/entities/src/EntityItemID.cpp +++ b/libraries/entities/src/EntityItemID.cpp @@ -11,7 +11,7 @@ #include #include - +#include #include #include "RegisteredMetaTypes.h" @@ -33,11 +33,8 @@ EntityItemID::EntityItemID(const QUuid& id) : QUuid(id) EntityItemID EntityItemID::readEntityItemIDFromBuffer(const unsigned char* data, int bytesLeftToRead) { EntityItemID result; - if (bytesLeftToRead >= NUM_BYTES_RFC4122_UUID) { - // id - QByteArray encodedID((const char*)data, NUM_BYTES_RFC4122_UUID); - result = QUuid::fromRfc4122(encodedID); + BufferParser(data, bytesLeftToRead).readUuid(result); } return result; } diff --git a/libraries/shared/src/BufferParser.h b/libraries/shared/src/BufferParser.h new file mode 100644 index 0000000000..622d59d125 --- /dev/null +++ b/libraries/shared/src/BufferParser.h @@ -0,0 +1,123 @@ +// +// Created by Bradley Austin Davis on 2015/07/08 +// Copyright 2013-2015 High Fidelity, Inc. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +#pragma once +#ifndef hifi_BufferParser_h +#define hifi_BufferParser_h + +#include + +#include +#include + +#include "GLMHelpers.h" +#include "ByteCountCoding.h" +#include "PropertyFlags.h" + +class BufferParser { +public: + BufferParser(const uint8_t* data, size_t size, size_t offset = 0) : + _offset(offset), _data(data), _size(size) { + } + + template + void readValue(T& result) { + Q_ASSERT(remaining() >= sizeof(T)); + memcpy(&result, _data + _offset, sizeof(T)); + _offset += sizeof(T); + } + + template<> + void readValue(quat& result) { + size_t advance = unpackOrientationQuatFromBytes(_data + _offset, result); + _offset += advance; + } + + template<> + void readValue(QString& result) { + uint16_t length; readValue(length); + result = QString((const char*)_data + _offset); + } + + template<> + void readValue(QUuid& result) { + uint16_t length; readValue(length); + Q_ASSERT(16 == length); + readUuid(result); + } + + template<> + void readValue(xColor& result) { + readValue(result.red); + readValue(result.blue); + readValue(result.green); + } + + template<> + void readValue>(QVector& result) { + uint16_t length; readValue(length); + result.resize(length); + memcpy(result.data(), _data + _offset, sizeof(glm::vec3) * length); + _offset += sizeof(glm::vec3) * length; + } + + template<> + void readValue(QByteArray& result) { + uint16_t length; readValue(length); + result = QByteArray((char*)_data + _offset, (int)length); + _offset += length; + } + + void readUuid(QUuid& result) { + readValue(result.data1); + readValue(result.data2); + readValue(result.data3); + readValue(result.data4); + result.data1 = qFromBigEndian(result.data1); + result.data2 = qFromBigEndian(result.data2); + result.data3 = qFromBigEndian(result.data3); + } + + template + void readFlags(PropertyFlags& result) { + // FIXME doing heap allocation + QByteArray encoded((const char*)(_data + _offset), remaining()); + result.decode(encoded); + _offset += result.getEncodedLength(); + } + + template + void readCompressedCount(T& result) { + // FIXME switch to a heapless implementation as soon as Brad provides it. + QByteArray encoded((const char*)(_data + _offset), std::min(sizeof(T) << 1, remaining())); + ByteCountCoded codec = encoded; + result = codec.data; + encoded = codec; + _offset += encoded.size(); + } + + inline size_t remaining() const { + return _size - _offset; + } + + inline size_t offset() const { + return _offset; + } + + inline const uint8_t* data() const { + return _data; + } + +private: + + size_t _offset{ 0 }; + const uint8_t* const _data; + const size_t _size; +}; + +#endif From 600e9cbf52197467d3f02c945e85c6dba42a1185 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Wed, 8 Jul 2015 18:17:03 -0700 Subject: [PATCH 2/3] Fixing template specialization compilation error on gcc/clang --- libraries/shared/src/BufferParser.h | 91 +++++++++++++++-------------- 1 file changed, 46 insertions(+), 45 deletions(-) diff --git a/libraries/shared/src/BufferParser.h b/libraries/shared/src/BufferParser.h index 622d59d125..5d388a6f5f 100644 --- a/libraries/shared/src/BufferParser.h +++ b/libraries/shared/src/BufferParser.h @@ -26,54 +26,13 @@ public: } template - void readValue(T& result) { + inline void readValue(T& result) { Q_ASSERT(remaining() >= sizeof(T)); memcpy(&result, _data + _offset, sizeof(T)); _offset += sizeof(T); } - template<> - void readValue(quat& result) { - size_t advance = unpackOrientationQuatFromBytes(_data + _offset, result); - _offset += advance; - } - - template<> - void readValue(QString& result) { - uint16_t length; readValue(length); - result = QString((const char*)_data + _offset); - } - - template<> - void readValue(QUuid& result) { - uint16_t length; readValue(length); - Q_ASSERT(16 == length); - readUuid(result); - } - - template<> - void readValue(xColor& result) { - readValue(result.red); - readValue(result.blue); - readValue(result.green); - } - - template<> - void readValue>(QVector& result) { - uint16_t length; readValue(length); - result.resize(length); - memcpy(result.data(), _data + _offset, sizeof(glm::vec3) * length); - _offset += sizeof(glm::vec3) * length; - } - - template<> - void readValue(QByteArray& result) { - uint16_t length; readValue(length); - result = QByteArray((char*)_data + _offset, (int)length); - _offset += length; - } - - void readUuid(QUuid& result) { + inline void readUuid(QUuid& result) { readValue(result.data1); readValue(result.data2); readValue(result.data3); @@ -84,7 +43,7 @@ public: } template - void readFlags(PropertyFlags& result) { + inline void readFlags(PropertyFlags& result) { // FIXME doing heap allocation QByteArray encoded((const char*)(_data + _offset), remaining()); result.decode(encoded); @@ -92,7 +51,7 @@ public: } template - void readCompressedCount(T& result) { + inline void readCompressedCount(T& result) { // FIXME switch to a heapless implementation as soon as Brad provides it. QByteArray encoded((const char*)(_data + _offset), std::min(sizeof(T) << 1, remaining())); ByteCountCoded codec = encoded; @@ -120,4 +79,46 @@ private: const size_t _size; }; + +template<> +inline void BufferParser::readValue(quat& result) { + size_t advance = unpackOrientationQuatFromBytes(_data + _offset, result); + _offset += advance; +} + +template<> +inline void BufferParser::readValue(QString& result) { + uint16_t length; readValue(length); + result = QString((const char*)_data + _offset); +} + +template<> +inline void BufferParser::readValue(QUuid& result) { + uint16_t length; readValue(length); + Q_ASSERT(16 == length); + readUuid(result); +} + +template<> +inline void BufferParser::readValue(xColor& result) { + readValue(result.red); + readValue(result.blue); + readValue(result.green); +} + +template<> +inline void BufferParser::readValue(QVector& result) { + uint16_t length; readValue(length); + result.resize(length); + memcpy(result.data(), _data + _offset, sizeof(glm::vec3) * length); + _offset += sizeof(glm::vec3) * length; +} + +template<> +inline void BufferParser::readValue(QByteArray& result) { + uint16_t length; readValue(length); + result = QByteArray((char*)_data + _offset, (int)length); + _offset += length; +} + #endif From f2beb79d232af270d5af3249116d730c7eb8e5e5 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Wed, 8 Jul 2015 18:54:38 -0700 Subject: [PATCH 3/3] Updating buffer parser to use new heapless API --- libraries/shared/src/BufferParser.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/libraries/shared/src/BufferParser.h b/libraries/shared/src/BufferParser.h index 5d388a6f5f..84bde2be31 100644 --- a/libraries/shared/src/BufferParser.h +++ b/libraries/shared/src/BufferParser.h @@ -53,11 +53,9 @@ public: template inline void readCompressedCount(T& result) { // FIXME switch to a heapless implementation as soon as Brad provides it. - QByteArray encoded((const char*)(_data + _offset), std::min(sizeof(T) << 1, remaining())); - ByteCountCoded codec = encoded; + ByteCountCoded codec; + _offset += codec.decode(reinterpret_cast(_data + _offset), remaining()); result = codec.data; - encoded = codec; - _offset += encoded.size(); } inline size_t remaining() const {