From 2dd7b9597a24ba61e164e369fef341da8a4544e8 Mon Sep 17 00:00:00 2001 From: Dave Streeter Date: Thu, 23 Oct 2025 15:02:21 +0100 Subject: [PATCH 1/5] HPCC-35227 Use peekStringList to improve Binary deserialize Implement peekAttributStringList in binary deserializeSelf Keep peekStringList Add unit test for peekAttributStringList Signed-off-by: Dave Streeter --- system/jlib/jptree.cpp | 45 +++++++++------- system/jlib/jstream.cpp | 85 ++++++++++++++++++++++++++---- system/jlib/jstream.hpp | 1 + testing/unittests/jstreamtests.cpp | 33 ++++++++++++ 4 files changed, 134 insertions(+), 30 deletions(-) diff --git a/system/jlib/jptree.cpp b/system/jlib/jptree.cpp index dc3d37dee7b..23fd7b68f3e 100644 --- a/system/jlib/jptree.cpp +++ b/system/jlib/jptree.cpp @@ -3070,39 +3070,46 @@ void PTree::deserializeFromStream(IBufferedSerialInputStream &src) } } +static std::vector attrStringOffsets = []() +{ + std::vector vec; + // Reserve capacity to reduce allocations + vec.reserve(50); + return vec; +}(); + void PTree::deserializeSelf(IBufferedSerialInputStream &src) { - size32_t len{0}; - const char *name = queryZeroTerminatedString(src, len); - if (len == 0) + size32_t skipLen{0}; + const char *name = queryZeroTerminatedString(src, skipLen); + if (skipLen == 0) throwUnexpectedX("PTree deserialization error: end of stream, expected name"); setName(name); - src.skip(len + 1); // Skip over name and null terminator + src.skip(skipLen + 1); // Skip over name and null terminator read(src, flags); // Read attributes until we encounter a zero byte (attribute list terminator) - for (;;) + attrStringOffsets.clear(); + const char * base = peekAttributeStringList(attrStringOffsets, src, skipLen); + if (base) // there is at least one attribute to process { - NextByteStatus status = isNextByteZero(src); - if (status == NextByteStatus::nextByteIsZero) - { - src.skip(1); // Skip over null terminator. - break; - } - if (status == NextByteStatus::endOfStream) - throwUnexpectedX("PTree deserialization error: end of stream, expected attribute name"); - - // NextByteStatus::nextByteIsNonZero - read the attribute key-value pair - std::pair attrPair = peekKeyValuePair(src, len); - if (attrPair.second == nullptr) + size_t numStringOffsets = attrStringOffsets.size(); + if (numStringOffsets % 2 != 0) throwUnexpectedX("PTree deserialization error: end of stream, expected attribute value"); constexpr bool attributeNameNotEncoded = false; // Deserialized attribute name is in its original unencoded form - setAttribute(attrPair.first, attrPair.second, attributeNameNotEncoded); + for (size_t i = 0; i < numStringOffsets; i += 2) + { + const char *attrName = base + attrStringOffsets[i]; + const char *attrValue = base + attrStringOffsets[i + 1]; + setAttribute(attrName, attrValue, attributeNameNotEncoded); + } - src.skip(len + 1); // +1 to skip over second null terminator. + src.skip(skipLen); // Skip over all attributes and the terminator } + else + throwUnexpectedX("PTree deserialization error: end of stream, expected attribute name"); if (value) delete value; diff --git a/system/jlib/jstream.cpp b/system/jlib/jstream.cpp index f18b7a825b1..db719441187 100644 --- a/system/jlib/jstream.cpp +++ b/system/jlib/jstream.cpp @@ -530,15 +530,40 @@ extern jlib_decl std::pair peekKeyValuePair(IBuffere } } -const char * peekStringList(std::vector & matchOffsets, IBufferedSerialInputStream & in, size32_t & len) +using NextValueValidator = std::function; + +static bool rejectAllValidator(const char * start, size32_t secondCharOffset, size32_t thirdCharOffset, size32_t got, bool expectingValue) +{ + return false; +} + +static bool attributeNameAndValueValidator(const char * start, size32_t secondCharOffset, size32_t thirdCharOffset, size32_t got, bool expectingValue) +{ + if (expectingValue) + { + if (thirdCharOffset < got) + { + char nextChar = start[secondCharOffset]; + return nextChar == '\0' || (nextChar == '@' && isValidXPathStartChr(start[thirdCharOffset])); + } + // else no more data for third character + } + // else empty name is not allowed + + return false; +} + +const char * peekStringList(std::vector & matches, IBufferedSerialInputStream & in, size32_t & len, NextValueValidator isValidNextValue) { size32_t scanned = 0; size32_t startNext = 0; + bool expectingValue = false; + for (;;) { size32_t got; - const char * start = (const char *)in.peek(scanned+1,got); - if (got <= scanned) + const char * start = (const char *)in.peek(scanned+1, got); + if (unlikely(got <= scanned)) { len = scanned; if (startNext == scanned) @@ -549,25 +574,63 @@ const char * peekStringList(std::vector & matchOffsets, IBufferedSeria return nullptr; } - for (size32_t offset = scanned; offset < got; offset++) + const char * searchStart = start + scanned; + size32_t searchLen = got - scanned; + + // null-byte scanning in the newly available portion + const char * nullPos; + while ((nullPos = (const char *)memchr(searchStart, '\0', searchLen)) != nullptr) { - char next = start[offset]; - if (!next) + size32_t offset = nullPos - start; + + if (unlikely(offset == startNext)) { - if (offset == startNext) + // Found a null terminator or an empty string at the start of a field + size32_t secondCharOffset = offset + 1; + size32_t thirdCharOffset = offset + 2; + + if (isValidNextValue(start, secondCharOffset, thirdCharOffset, got, expectingValue)) { - //A zero length string terminates the list - include the empty string in the length - len = offset + 1; - return start; + // Valid empty value + matches.push_back(startNext); + expectingValue = false; + startNext = secondCharOffset; + + searchStart = nullPos + 1; + searchLen = got - secondCharOffset; + continue; } - matchOffsets.push_back(startNext); + + // Either not enough data to decide, or invalid empty field, treat as list terminator + len = offset + 1; + return start; + } + else + { + matches.push_back(startNext); + expectingValue = !expectingValue; startNext = offset + 1; + + searchStart = nullPos + 1; + searchLen = got - startNext; } } + + // No more nulls found in current buffer, need to peek more data scanned = got; } } +const char * peekStringList(std::vector & matches, IBufferedSerialInputStream & in, size32_t & len) +{ + return peekStringList(matches, in, len, rejectAllValidator); +} + +const char * peekAttributeStringList(std::vector & matches, IBufferedSerialInputStream & in, size32_t & len) +{ + return peekStringList(matches, in, len, attributeNameAndValueValidator); +} + //--------------------------------------------------------------------------- class CFileSerialInputStream final : public CInterfaceOf diff --git a/system/jlib/jstream.hpp b/system/jlib/jstream.hpp index ac4431d84a5..f5cf53d18e4 100644 --- a/system/jlib/jstream.hpp +++ b/system/jlib/jstream.hpp @@ -73,6 +73,7 @@ extern jlib_decl std::pair peekKeyValuePair(IBuffere //Return a vector of offsets of the starts of null terminated strings - terminated by a null string or end of file. //Returns a pointer to the base string if valid. extern jlib_decl const char * peekStringList(std::vector & matches, IBufferedSerialInputStream & in, size32_t & len); +extern jlib_decl const char * peekAttributeStringList(std::vector & matches, IBufferedSerialInputStream & in, size32_t & len); interface ISerialOutputStream : extends IInterface diff --git a/testing/unittests/jstreamtests.cpp b/testing/unittests/jstreamtests.cpp index d9da589f13a..4d2ff679c49 100644 --- a/testing/unittests/jstreamtests.cpp +++ b/testing/unittests/jstreamtests.cpp @@ -1018,6 +1018,25 @@ class JlibStreamStressTest : public CppUnit::TestFixture CPPUNIT_ASSERT(in.eos()); } + void testAttributeStringListPeek(IBufferedSerialInputStream & in) + { + offset_t offset = 0; + std::vector matches; + unsigned skipLen = 0; + const char * base = peekAttributeStringList(matches, in, skipLen); + + unsigned delta = 0; + for (size32_t next : matches) + { + unsigned len = check("testAttributeStringListPeek", offset, base + next, delta); + offset += len+1; + delta++; + } + assertex(skipLen == offset); + in.skip(skipLen); + CPPUNIT_ASSERT(in.eos()); + } + void testVarStringBuffer() { size32_t maxStringLength = 0x10000; @@ -1060,6 +1079,13 @@ class JlibStreamStressTest : public CppUnit::TestFixture testStringListPeek(*in); DBGLOG("Buffer:testStringListPeek took %lluus", timer.elapsedNs()/1000); } + + { + CCycleTimer timer; + Owned in = createBufferedSerialInputStream(buffer.reset(0)); + testAttributeStringListPeek(*in); + DBGLOG("Buffer:testAttributeStringListPeek took %lluus", timer.elapsedNs()/1000); + } } void testVarStringFile(size32_t maxStringLength) { @@ -1099,6 +1125,13 @@ class JlibStreamStressTest : public CppUnit::TestFixture DBGLOG("File:testStringListPeek took %lluus", timer.elapsedNs()/1000); } + { + CCycleTimer timer; + Owned in = createInput(filename, bufferSize, nullptr, 0, false); + testAttributeStringListPeek(*in); + DBGLOG("File:testAttributeStringListPeek took %lluus", timer.elapsedNs()/1000); + } + { CCycleTimer timer; Owned in = createInput(filename, bufferSize, nullptr, 0, true); From a07e3ef8a0e6dec5776fb824437ed65710a92403 Mon Sep 17 00:00:00 2001 From: Dave Streeter Date: Thu, 23 Oct 2025 15:59:34 +0100 Subject: [PATCH 2/5] Comment Signed-off-by: Dave Streeter --- system/jlib/jstream.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/system/jlib/jstream.cpp b/system/jlib/jstream.cpp index db719441187..e148dd88f20 100644 --- a/system/jlib/jstream.cpp +++ b/system/jlib/jstream.cpp @@ -541,9 +541,12 @@ static bool attributeNameAndValueValidator(const char * start, size32_t secondCh { if (expectingValue) { + // Current null is possibly a empty string Attribute Value. if (thirdCharOffset < got) { char nextChar = start[secondCharOffset]; + // nextChar could be a null denoting eos; or the numeric value of the size of CPTValue followed by a null char; or an Attribute Name. + // So we need to valid that the nextChar is valid for an Attribute Name. return nextChar == '\0' || (nextChar == '@' && isValidXPathStartChr(start[thirdCharOffset])); } // else no more data for third character From cacd173f8621522ba610ff5cc5dbadcd99a8d99d Mon Sep 17 00:00:00 2001 From: Dave Streeter Date: Fri, 24 Oct 2025 11:39:07 +0100 Subject: [PATCH 3/5] setAttribute vector WIP Signed-off-by: Dave Streeter --- system/jlib/jptree.cpp | 128 +++++++++++++++- system/jlib/jptree.ipp | 5 + system/jlib/jstream.cpp | 70 +++------ system/jlib/jstream.hpp | 5 +- testing/unittests/jlibtests.cpp | 226 +++++++++++++++++------------ testing/unittests/jstreamtests.cpp | 33 ----- 6 files changed, 289 insertions(+), 178 deletions(-) diff --git a/system/jlib/jptree.cpp b/system/jlib/jptree.cpp index 23fd7b68f3e..242b763a554 100644 --- a/system/jlib/jptree.cpp +++ b/system/jlib/jptree.cpp @@ -3091,13 +3091,16 @@ void PTree::deserializeSelf(IBufferedSerialInputStream &src) // Read attributes until we encounter a zero byte (attribute list terminator) attrStringOffsets.clear(); - const char * base = peekAttributeStringList(attrStringOffsets, src, skipLen); + const char * base = peekStringList(attrStringOffsets, src, skipLen); if (base) // there is at least one attribute to process { +/* size_t numStringOffsets = attrStringOffsets.size(); if (numStringOffsets % 2 != 0) throwUnexpectedX("PTree deserialization error: end of stream, expected attribute value"); - +*/ + setAttribute(base, attrStringOffsets); +/* DJPS constexpr bool attributeNameNotEncoded = false; // Deserialized attribute name is in its original unencoded form for (size_t i = 0; i < numStringOffsets; i += 2) { @@ -3105,6 +3108,7 @@ void PTree::deserializeSelf(IBufferedSerialInputStream &src) const char *attrValue = base + attrStringOffsets[i + 1]; setAttribute(attrName, attrValue, attributeNameNotEncoded); } +*/ src.skip(skipLen); // Skip over all attributes and the terminator } @@ -3888,6 +3892,59 @@ void LocalPTree::setAttribute(const char *inputkey, const char *val, bool encode AttrStr::destroy(goer); } +void LocalPTree::setAttribute(const char *base, const std::vector &offsets) +{ + size_t numStringOffsets = offsets.size(); + if (numStringOffsets % 2 != 0) + throwUnexpectedX("setAttribute error: offsets vector must contain pairs of key/value offsets"); + + for (size_t i = 0; i < numStringOffsets; i += 2) + { + const char *inputkey = base + offsets[i]; + const char *val = base + offsets[i + 1]; + + if (!inputkey) + continue; + const char *key = inputkey; + // Note: encoded is always false for deserialized attributes + if (!validateXMLTag(key+1)) + throw MakeIPTException(-1, "Invalid xml attribute: %s", key); + if (!val) + val = ""; // cannot have NULL value + AttrValue *v = findAttribute(key); + AttrStr *goer = nullptr; + if (v) + { + if (streq(v->value.get(), val)) + continue; + goer = v->value.getPtr(); + } + else + { + attrs = (AttrValue *)realloc(attrs, (numAttrs+1)*sizeof(AttrValue)); + v = new(&attrs[numAttrs++]) AttrValue; // Initialize new AttrValue + if (!v->key.set(inputkey)) //AttrStr will not return encoding marker when get() is called + v->key.setPtr(isnocase() ? AttrStr::createNC(inputkey) : AttrStr::create(inputkey)); + } + if (arrayOwner) + { + CQualifierMap *map = arrayOwner->queryMap(); + if (map) + { + if (goer) + map->replaceEntryIfMapped(key, v->value.get(), val, this); + else + map->insertEntryIfMapped(key, val, this); + } + } + + if (!v->value.set(val)) + v->value.setPtr(AttrStr::create(val)); + if (goer) + AttrStr::destroy(goer); + } +} + #ifdef TRACE_STRING_SIZE std::atomic<__int64> AttrStr::totsize { 0 }; std::atomic<__int64> AttrStr::maxsize { 0 }; @@ -4102,6 +4159,73 @@ void CAtomPTree::setAttribute(const char *key, const char *val, bool encoded) } } +void CAtomPTree::setAttribute(const char *base, const std::vector &offsets) +{ + size_t numStringOffsets = offsets.size(); + if (numStringOffsets % 2 != 0) + throwUnexpectedX("setAttribute error: offsets vector must contain pairs of key/value offsets"); + + CriticalBlock block(hashcrit); + for (size_t i = 0; i < numStringOffsets; i += 2) + { + const char *key = base + offsets[i]; + const char *val = base + offsets[i + 1]; + + if (!key) + continue; + if (!validateXMLTag(key+1)) + throw MakeIPTException(-1, "Invalid xml attribute: %s", key); + if (!val) + val = ""; // cannot have NULL value + AttrValue *v = findAttribute(key); + if (v) + { + if (streq(v->value.get(), val)) + continue; + if (arrayOwner) + { + CQualifierMap *map = arrayOwner->queryMap(); + if (map) + map->replaceEntryIfMapped(key, v->value.get(), val, this); + } + + AttrStr * goer = v->value.getPtr(); + if (!v->value.set(val)) + { + if (goer) + attrHT->removeval(goer); + v->value.setPtr(attrHT->addval(val)); + } + else if (goer) + { + attrHT->removeval(goer); + } + } + else + { + AttrValue *newattrs = newAttrArray(numAttrs+1); + if (attrs) + { + memcpy(newattrs, attrs, numAttrs*sizeof(AttrValue)); + freeAttrArray(attrs, numAttrs); + } + if (arrayOwner) + { + CQualifierMap *map = arrayOwner->queryMap(); + if (map) + map->insertEntryIfMapped(key, val, this); + } + v = &newattrs[numAttrs]; + if (!v->key.set(key)) + v->key.setPtr(attrHT->addkey(key, isnocase())); + if (!v->value.set(val)) + v->value.setPtr(attrHT->addval(val)); + numAttrs++; + attrs = newattrs; + } + } +} + bool CAtomPTree::removeAttribute(const char *key) { AttrValue *del = findAttribute(key); diff --git a/system/jlib/jptree.ipp b/system/jlib/jptree.ipp index 3e4f35311f7..ee69df5411a 100644 --- a/system/jlib/jptree.ipp +++ b/system/jlib/jptree.ipp @@ -19,6 +19,8 @@ #ifndef _PTREE_IPP #define _PTREE_IPP +#include + #include "jarray.hpp" #include "jexcept.hpp" #include "jhash.hpp" @@ -701,6 +703,7 @@ public: return a->key.isEncoded(); } virtual void setAttribute(const char *attr, const char *val, bool encoded) = 0; + virtual void setAttribute(const char *base, const std::vector &offsets) = 0; // IPropertyTree impl. virtual bool hasProp(const char * xpath) const override; @@ -864,6 +867,7 @@ public: virtual unsigned queryHash() const override; virtual void setName(const char *_name) override; virtual void setAttribute(const char *attr, const char *val, bool encoded) override; + virtual void setAttribute(const char *base, const std::vector &offsets) override; virtual bool isEquivalent(IPropertyTree *tree) const override { return (nullptr != QUERYINTERFACE(tree, CAtomPTree)); } virtual IPropertyTree *create(const char *name=nullptr, IPTArrayValue *value=nullptr, ChildMap *children=nullptr, bool existing=false) override { @@ -905,6 +909,7 @@ public: } virtual void setName(const char *_name) override; virtual void setAttribute(const char *attr, const char *val, bool encoded) override; + virtual void setAttribute(const char *base, const std::vector &offsets) override; virtual bool isEquivalent(IPropertyTree *tree) const override { return (nullptr != QUERYINTERFACE(tree, LocalPTree)); } virtual IPropertyTree *create(const char *name=nullptr, IPTArrayValue *value=nullptr, ChildMap *children=nullptr, bool existing=false) override { diff --git a/system/jlib/jstream.cpp b/system/jlib/jstream.cpp index e148dd88f20..db4f23f7f61 100644 --- a/system/jlib/jstream.cpp +++ b/system/jlib/jstream.cpp @@ -530,33 +530,7 @@ extern jlib_decl std::pair peekKeyValuePair(IBuffere } } -using NextValueValidator = std::function; - -static bool rejectAllValidator(const char * start, size32_t secondCharOffset, size32_t thirdCharOffset, size32_t got, bool expectingValue) -{ - return false; -} - -static bool attributeNameAndValueValidator(const char * start, size32_t secondCharOffset, size32_t thirdCharOffset, size32_t got, bool expectingValue) -{ - if (expectingValue) - { - // Current null is possibly a empty string Attribute Value. - if (thirdCharOffset < got) - { - char nextChar = start[secondCharOffset]; - // nextChar could be a null denoting eos; or the numeric value of the size of CPTValue followed by a null char; or an Attribute Name. - // So we need to valid that the nextChar is valid for an Attribute Name. - return nextChar == '\0' || (nextChar == '@' && isValidXPathStartChr(start[thirdCharOffset])); - } - // else no more data for third character - } - // else empty name is not allowed - - return false; -} - -const char * peekStringList(std::vector & matches, IBufferedSerialInputStream & in, size32_t & len, NextValueValidator isValidNextValue) +const char * peekStringList(std::vector & matchOffsets, IBufferedSerialInputStream & in, size32_t & len) { size32_t scanned = 0; size32_t startNext = 0; @@ -592,17 +566,29 @@ const char * peekStringList(std::vector & matches, IBufferedSerialInpu size32_t secondCharOffset = offset + 1; size32_t thirdCharOffset = offset + 2; - if (isValidNextValue(start, secondCharOffset, thirdCharOffset, got, expectingValue)) + if (expectingValue) { - // Valid empty value - matches.push_back(startNext); - expectingValue = false; - startNext = secondCharOffset; - - searchStart = nullPos + 1; - searchLen = got - secondCharOffset; - continue; + // Current null is possibly a empty string Attribute Value. + if (thirdCharOffset < got) + { + char nextChar = start[secondCharOffset]; + // nextChar could be a null denoting eos; or the numeric value of the size of CPTValue followed by a null char; or an Attribute Name. + // So we need to valid that the nextChar is valid for an Attribute Name. + if (nextChar == '\0' || (nextChar == '@' && isValidXPathStartChr(start[thirdCharOffset]))) + { + // Valid empty value + matchOffsets.push_back(startNext); + expectingValue = false; + startNext = secondCharOffset; + + searchStart = nullPos + 1; + searchLen = got - secondCharOffset; + continue; + } + } + // else no more data for third character } + // else empty name is not allowed // Either not enough data to decide, or invalid empty field, treat as list terminator len = offset + 1; @@ -610,7 +596,7 @@ const char * peekStringList(std::vector & matches, IBufferedSerialInpu } else { - matches.push_back(startNext); + matchOffsets.push_back(startNext); expectingValue = !expectingValue; startNext = offset + 1; @@ -624,16 +610,6 @@ const char * peekStringList(std::vector & matches, IBufferedSerialInpu } } -const char * peekStringList(std::vector & matches, IBufferedSerialInputStream & in, size32_t & len) -{ - return peekStringList(matches, in, len, rejectAllValidator); -} - -const char * peekAttributeStringList(std::vector & matches, IBufferedSerialInputStream & in, size32_t & len) -{ - return peekStringList(matches, in, len, attributeNameAndValueValidator); -} - //--------------------------------------------------------------------------- class CFileSerialInputStream final : public CInterfaceOf diff --git a/system/jlib/jstream.hpp b/system/jlib/jstream.hpp index f5cf53d18e4..babdd2e58a9 100644 --- a/system/jlib/jstream.hpp +++ b/system/jlib/jstream.hpp @@ -70,11 +70,10 @@ extern jlib_decl const char * queryZeroTerminatedString(IBufferedSerialInputStre //return a key value pair - if the key is a null string then return null for the value extern jlib_decl std::pair peekKeyValuePair(IBufferedSerialInputStream & in, size32_t & len); -//Return a vector of offsets of the starts of null terminated strings - terminated by a null string or end of file. +//Return a vector of offsets of the starts of null terminated Attribute Name/Value strings (Value can be empty string) +// - terminated by a null string or end of file. //Returns a pointer to the base string if valid. extern jlib_decl const char * peekStringList(std::vector & matches, IBufferedSerialInputStream & in, size32_t & len); -extern jlib_decl const char * peekAttributeStringList(std::vector & matches, IBufferedSerialInputStream & in, size32_t & len); - interface ISerialOutputStream : extends IInterface { diff --git a/testing/unittests/jlibtests.cpp b/testing/unittests/jlibtests.cpp index 7537298113c..11e7b89c5ba 100644 --- a/testing/unittests/jlibtests.cpp +++ b/testing/unittests/jlibtests.cpp @@ -3416,7 +3416,7 @@ CPPUNIT_TEST_SUITE_NAMED_REGISTRATION(JlibIPTTest, "JlibIPTTest"); IPropertyTree *createCompatibilityConfigPropertyTree() { // Creates a complex nested property tree with multiple compatibility elements for serialization testing - Owned root = createPTree("__array__"); + Owned root = createPTree("Property"); // Helper lambda to add property elements with name/value attributes auto addProperty = [](IPropertyTree *parent, const char *name, const char *value = nullptr) @@ -3575,13 +3575,10 @@ IPropertyTree *createCompatibilityConfigPropertyTree() return root.getClear(); } -IPropertyTree *createBinaryDataCompressionTestPTree(const char *testXml) +IPropertyTree *createBinaryDataCompressionTestPTree() { - // Validate the testXml parameter - CPPUNIT_ASSERT(testXml && *testXml); - // Create a tree with various binary data sizes to test compression thresholds - Owned tree = createPTree(testXml); + Owned tree = createPTree("Compressed"); // Add some regular properties first tree->setProp("normalProp", "normalValue"); @@ -3630,97 +3627,105 @@ class PTreeSerializationDeserializationTest : public CppUnit::TestFixture CPPUNIT_TEST(testRoundTripForRootOnlyPTree); CPPUNIT_TEST(testRoundTripForCompatibilityConfigPropertyTree); CPPUNIT_TEST(testRoundTripForBinaryDataCompressionTestPTree); + CPPUNIT_TEST(testRoundTripForRootOnlyPTree_lowmem); + CPPUNIT_TEST(testRoundTripForCompatibilityConfigPropertyTree_lowmem); + CPPUNIT_TEST(testRoundTripForBinaryDataCompressionTestPTree_lowmem); CPPUNIT_TEST_SUITE_END(); protected: - static constexpr const char *testXml{ - "" - "" - " " - " " - " " - " " - "
" - " " - " " - " " - " " - " " - " " - " " - ""}; - // Complete round-trip test method that performs serialization, deserialization, and validation - void performRoundTripTest(const char *testName, IPropertyTree *tree) + void performRoundTripTest(const char *testName, IPropertyTree *tree, byte flags=ipt_none) { - Owned originalTree; - originalTree.setown(tree); - CCycleTimer timer; - - // Serialization - MemoryBuffer memoryBuffer, streamBuffer; - - // Time serialize() method - timer.reset(); - originalTree->serialize(memoryBuffer); - __uint64 serializeElapsedNs = timer.elapsedNs(); - size_t memoryBufferSize = memoryBuffer.length(); - - // Time serializeToStream() method - Owned out = createBufferedSerialOutputStream(streamBuffer); - timer.reset(); - originalTree->serializeToStream(*out); - out->flush(); - __uint64 serializeToStreamElapsedNs = timer.elapsedNs(); - size_t streamBufferSize = streamBuffer.length(); - - // Validation - serialized data matches - CPPUNIT_ASSERT_EQUAL(memoryBufferSize, streamBufferSize); - CPPUNIT_ASSERT(memcmp(memoryBuffer.toByteArray(), streamBuffer.toByteArray(), memoryBufferSize) == 0); - - // Time deserialize() method - Owned memoryBufferDeserialized = createPTree(); - timer.reset(); - memoryBufferDeserialized->deserialize(memoryBuffer); - __uint64 deserializeElapsedNs = timer.elapsedNs(); - - // Time deserializeFromStream() method - Owned in = createBufferedSerialInputStream(streamBuffer); - Owned streamDeserialized = createPTree(); - timer.reset(); - streamDeserialized->deserializeFromStream(*in); - __uint64 deserializeFromStreamElapsedNs = timer.elapsedNs(); - - // Create PTree from Binary tests - // - // Test 1: Call with null nodeCreator (should fall back to createPTree(src, ipt_none)) - streamBuffer.reset(); - Owned in2 = createBufferedSerialInputStream(streamBuffer); - Owned deserializedCreatePTreeFromBinaryWithNull = createPTreeFromBinary(*in2, nullptr); - // Test 2: Call with custom nodeCreator - class TestNodeCreator : public CSimpleInterfaceOf + try { - public: - bool wasCalled = false; + Owned originalTree; + originalTree.setown(tree); + CCycleTimer timer; - virtual IPropertyTree *create(const char *tag) override + // Serialization + MemoryBuffer memoryBuffer, streamBuffer; + + // Time serialize() method + DBGLOG("=== Starting serialize() for test: %s ===", testName); + timer.reset(); + originalTree->serialize(memoryBuffer); + __uint64 serializeElapsedNs = timer.elapsedNs(); + size_t memoryBufferSize = memoryBuffer.length(); + DBGLOG("=== serialize() completed, size: %zu bytes ===", memoryBufferSize); + + // Time serializeToStream() method + DBGLOG("=== Starting serializeToStream() ==="); + Owned out = createBufferedSerialOutputStream(streamBuffer); + timer.reset(); + originalTree->serializeToStream(*out); + out->flush(); + __uint64 serializeToStreamElapsedNs = timer.elapsedNs(); + size_t streamBufferSize = streamBuffer.length(); + DBGLOG("=== serializeToStream() completed, size: %zu bytes ===", streamBufferSize); + + // Validation - serialized data matches + CPPUNIT_ASSERT_EQUAL(memoryBufferSize, streamBufferSize); + CPPUNIT_ASSERT(memcmp(memoryBuffer.toByteArray(), streamBuffer.toByteArray(), memoryBufferSize) == 0); + + // Copy streamBuffer for deserializationFromStream() tests + MemoryBuffer streamBuffer2, streamBuffer3; + streamBuffer2.append(streamBuffer.length(), streamBuffer.toByteArray()); + streamBuffer3.append(streamBuffer.length(), streamBuffer.toByteArray()); + + // Time deserialize() method + DBGLOG("=== Starting deserialize() ==="); + Owned memoryBufferDeserialized = createPTree(flags); + timer.reset(); + memoryBufferDeserialized->deserialize(memoryBuffer); + __uint64 deserializeElapsedNs = timer.elapsedNs(); + DBGLOG("=== deserialize() completed ==="); + + // Time deserializeFromStream() method + DBGLOG("=== Starting deserializeFromStream() ==="); + Owned in = createBufferedSerialInputStream(streamBuffer); + Owned streamDeserialized = createPTree(flags); + timer.reset(); + streamDeserialized->deserializeFromStream(*in); + __uint64 deserializeFromStreamElapsedNs = timer.elapsedNs(); + DBGLOG("=== deserializeFromStream() completed ==="); + + // Create PTree from Binary tests + // + // Test 1: Call with flags parameter + DBGLOG("=== Starting createPTreeFromBinary() with flags ==="); + Owned in2 = createBufferedSerialInputStream(streamBuffer2); + Owned deserializedCreatePTreeFromBinaryWithFlags = createPTreeFromBinary(*in2, flags); + DBGLOG("=== createPTreeFromBinary() with flags completed ==="); + // Test 2: Call with custom nodeCreator + DBGLOG("=== Starting createPTreeFromBinary() with custom nodeCreator ==="); + class TestNodeCreator : public CSimpleInterfaceOf { - wasCalled = true; - return createPTree(tag); - } - }; - Owned nodeCreator = new TestNodeCreator(); - // Reset stream position - streamBuffer.reset(); - Owned in3 = createBufferedSerialInputStream(streamBuffer); - Owned deserializedCreatePTreeFromBinaryWithCreator = createPTreeFromBinary(*in3, nodeCreator); - - // Validation - verify both deserialized trees are equivalent to the original - CPPUNIT_ASSERT(areMatchingPTrees(originalTree, memoryBufferDeserialized)); - CPPUNIT_ASSERT(areMatchingPTrees(originalTree, streamDeserialized)); - CPPUNIT_ASSERT(areMatchingPTrees(originalTree, deserializedCreatePTreeFromBinaryWithNull)); - CPPUNIT_ASSERT(nodeCreator->wasCalled); // Verify nodeCreator was called - CPPUNIT_ASSERT(areMatchingPTrees(originalTree, deserializedCreatePTreeFromBinaryWithCreator)); + public: + bool wasCalled = false; + byte nodeFlags; + + TestNodeCreator(byte _flags) : nodeFlags(_flags) {} + + virtual IPropertyTree *create(const char *tag) override + { + wasCalled = true; + return createPTree(tag, nodeFlags); + } + }; + Owned nodeCreator = new TestNodeCreator(flags); + // Reset stream position + Owned in3 = createBufferedSerialInputStream(streamBuffer3); + Owned deserializedCreatePTreeFromBinaryWithCreator = createPTreeFromBinary(*in3, nodeCreator); + DBGLOG("=== createPTreeFromBinary() with custom nodeCreator completed ==="); + + // Validation - verify both deserialized trees are equivalent to the original + DBGLOG("=== Starting tree comparisons ==="); + CPPUNIT_ASSERT(areMatchingPTrees(originalTree, memoryBufferDeserialized)); + CPPUNIT_ASSERT(areMatchingPTrees(originalTree, streamDeserialized)); + CPPUNIT_ASSERT(areMatchingPTrees(originalTree, deserializedCreatePTreeFromBinaryWithFlags)); + CPPUNIT_ASSERT(nodeCreator->wasCalled); // Verify nodeCreator was called + CPPUNIT_ASSERT(areMatchingPTrees(originalTree, deserializedCreatePTreeFromBinaryWithCreator)); + DBGLOG("=== Tree comparisons completed ==="); double serializeTimeMs = serializeElapsedNs / 1e6; double serializeToStreamTimeMs = serializeToStreamElapsedNs / 1e6; @@ -3736,10 +3741,29 @@ class PTreeSerializationDeserializationTest : public CppUnit::TestFixture double deserializeFromStreamTimeMs = deserializeFromStreamElapsedNs / 1e6; DBGLOG("=== DESERIALIZATION TEST RESULTS:"); DBGLOG(" deserialize() time: %.6f ms", deserializeTimeMs); - DBGLOG(" deserializeFromStream() time: %.6f ms", deserializeFromStreamTimeMs); - DBGLOG(" Performance ratio (deserializeFromStream/deserialize): %.6f", deserializeFromStreamTimeMs / deserializeTimeMs); + DBGLOG(" deserializeFromStream() time: %.6f ms", deserializeFromStreamTimeMs); + DBGLOG(" Performance ratio (deserializeFromStream/deserialize): %.6f", deserializeFromStreamTimeMs / deserializeTimeMs); - DBGLOG("=== ROUND-TRIP TEST COMPLETED SUCCESSFULLY"); + DBGLOG("=== ROUND-TRIP TEST COMPLETED SUCCESSFULLY"); + } + catch (IException *e) + { + StringBuffer msg; + e->errorMessage(msg); + DBGLOG("=== EXCEPTION CAUGHT in %s: [%d] %s ===", testName, e->errorCode(), msg.str()); + e->Release(); + throw; + } + catch (std::exception &e) + { + DBGLOG("=== STD::EXCEPTION CAUGHT in %s: %s ===", testName, e.what()); + throw; + } + catch (...) + { + DBGLOG("=== UNKNOWN EXCEPTION CAUGHT in %s ===", testName); + throw; + } } public: @@ -3756,7 +3780,23 @@ class PTreeSerializationDeserializationTest : public CppUnit::TestFixture void testRoundTripForBinaryDataCompressionTestPTree() { - performRoundTripTest(__func__, createBinaryDataCompressionTestPTree(testXml)); + performRoundTripTest(__func__, createBinaryDataCompressionTestPTree()); + } + + // ipt_lowmem tests + void testRoundTripForRootOnlyPTree_lowmem() + { + performRoundTripTest(__func__, createPTree("EmptyRoot", ipt_lowmem), ipt_lowmem); + } + + void testRoundTripForCompatibilityConfigPropertyTree_lowmem() + { + performRoundTripTest(__func__, createCompatibilityConfigPropertyTree(), ipt_lowmem); + } + + void testRoundTripForBinaryDataCompressionTestPTree_lowmem() + { + performRoundTripTest(__func__, createBinaryDataCompressionTestPTree(), ipt_lowmem); } }; diff --git a/testing/unittests/jstreamtests.cpp b/testing/unittests/jstreamtests.cpp index 4d2ff679c49..d9da589f13a 100644 --- a/testing/unittests/jstreamtests.cpp +++ b/testing/unittests/jstreamtests.cpp @@ -1018,25 +1018,6 @@ class JlibStreamStressTest : public CppUnit::TestFixture CPPUNIT_ASSERT(in.eos()); } - void testAttributeStringListPeek(IBufferedSerialInputStream & in) - { - offset_t offset = 0; - std::vector matches; - unsigned skipLen = 0; - const char * base = peekAttributeStringList(matches, in, skipLen); - - unsigned delta = 0; - for (size32_t next : matches) - { - unsigned len = check("testAttributeStringListPeek", offset, base + next, delta); - offset += len+1; - delta++; - } - assertex(skipLen == offset); - in.skip(skipLen); - CPPUNIT_ASSERT(in.eos()); - } - void testVarStringBuffer() { size32_t maxStringLength = 0x10000; @@ -1079,13 +1060,6 @@ class JlibStreamStressTest : public CppUnit::TestFixture testStringListPeek(*in); DBGLOG("Buffer:testStringListPeek took %lluus", timer.elapsedNs()/1000); } - - { - CCycleTimer timer; - Owned in = createBufferedSerialInputStream(buffer.reset(0)); - testAttributeStringListPeek(*in); - DBGLOG("Buffer:testAttributeStringListPeek took %lluus", timer.elapsedNs()/1000); - } } void testVarStringFile(size32_t maxStringLength) { @@ -1125,13 +1099,6 @@ class JlibStreamStressTest : public CppUnit::TestFixture DBGLOG("File:testStringListPeek took %lluus", timer.elapsedNs()/1000); } - { - CCycleTimer timer; - Owned in = createInput(filename, bufferSize, nullptr, 0, false); - testAttributeStringListPeek(*in); - DBGLOG("File:testAttributeStringListPeek took %lluus", timer.elapsedNs()/1000); - } - { CCycleTimer timer; Owned in = createInput(filename, bufferSize, nullptr, 0, true); From b2fc74a74938b16c839230f9f504db5db6f3b14c Mon Sep 17 00:00:00 2001 From: Dave Streeter Date: Fri, 24 Oct 2025 12:55:49 +0100 Subject: [PATCH 4/5] Add back tests removed unintentionally Signed-off-by: Dave Streeter --- testing/unittests/CMakeLists.txt | 2 + testing/unittests/jlibtests.cpp | 535 +++++++++++++++++++++++++++++++ 2 files changed, 537 insertions(+) diff --git a/testing/unittests/CMakeLists.txt b/testing/unittests/CMakeLists.txt index f218d72fa58..29e54e8be3a 100644 --- a/testing/unittests/CMakeLists.txt +++ b/testing/unittests/CMakeLists.txt @@ -82,6 +82,7 @@ include_directories ( ./../../common/thorhelper ./../../dali/base ./../../system/security/shared + ./../../system/security/zcrypt ./../../common/deftype ./../../common/workunit ./../../system/security/cryptohelper @@ -128,6 +129,7 @@ target_link_libraries ( unittests logginglib workunit eventconsumption + zcrypt ${CppUnit_LIBRARIES} ) diff --git a/testing/unittests/jlibtests.cpp b/testing/unittests/jlibtests.cpp index 11e7b89c5ba..186101fffe2 100644 --- a/testing/unittests/jlibtests.cpp +++ b/testing/unittests/jlibtests.cpp @@ -27,6 +27,10 @@ #include #include +#ifdef CALLGRIND_PROFILING +#include +#endif + #include "jsem.hpp" #include "jfile.hpp" #include "jdebug.hpp" @@ -38,6 +42,7 @@ #include "jsecrets.hpp" #include "jutil.hpp" #include "junicode.hpp" +#include "zcrypt.hpp" #include "opentelemetry/sdk/common/attribute_utils.h" #include "opentelemetry/sdk/resource/resource.h" @@ -3803,6 +3808,536 @@ class PTreeSerializationDeserializationTest : public CppUnit::TestFixture CPPUNIT_TEST_SUITE_REGISTRATION(PTreeSerializationDeserializationTest); CPPUNIT_TEST_SUITE_NAMED_REGISTRATION(PTreeSerializationDeserializationTest, "PTreeSerializationDeserializationTest"); +// Base class with shared functionality for PTree timing tests +class PTreeTimingTestBase : public CppUnit::TestFixture +{ +protected: + struct TimingResults + { + double avgDeserializeTimeMs; + double avgSerializeTimeMs; + double totalDeserializeTimeMs; + double totalSerializeTimeMs; + const char *testName; + byte flags; + }; + + const char *getBinaryFilePath() + { + const char *envPath = getenv("DALI_TEST_BINARY_FILE"); + constexpr const char *defaultPath = "~/HPCC-Platform/testing/unittests/dalisds1.bin.gz"; + return envPath ? envPath : defaultPath; + } + + const char *getXmlFilePath() + { + const char *envPath = getenv("DALI_TEST_XML_FILE"); + constexpr const char *defaultPath = "~/HPCC-Platform/testing/unittests/dalisds1.xml.gz"; + return envPath ? envPath : defaultPath; + } + + const char *expandTilde(const char *path, StringBuffer &expanded) + { + if (!path || !path[0]) + return path; + + if (path[0] == '~' && (path[1] == '/' || path[1] == '\0')) + { + const char *home = getenv("HOME"); + if (home) + { + expanded.append(home); + if (path[1] == '/') + expanded.append(path + 1); + return expanded.str(); + } + } + return path; + } + + void readXmlFile(const char *filePath, StringBuffer &output) + { + StringBuffer expandedPath; + const char *actualPath = expandTilde(filePath, expandedPath); + if (!actualPath) + throw MakeStringException(-1, "XML file path is null"); + Owned xmlFile = createIFile(actualPath); + if (!xmlFile->exists()) + throw MakeStringException(-1, "XML file \"%s\" does not exist", actualPath); + + size32_t fileSize = (size32_t)xmlFile->size(); + MemoryBuffer fileData; + Owned fileIO = xmlFile->open(IFOread); + fileData.reserveTruncate(fileSize); + size32_t bytesRead = fileIO->read(0, fileSize, fileData.bufferBase()); + fileData.setLength(bytesRead); + + const char *ext = pathExtension(filePath); + if (ext && streq(ext, ".gz")) + gunzip((const byte *)fileData.toByteArray(), fileData.length(), output); + else + output.append(fileData.length(), fileData.toByteArray()); + } + + void createBinaryDataFromXml(const char *xmlFilePath, MemoryBuffer &binaryData) + { + StringBuffer xmlData; + try + { + readXmlFile(xmlFilePath, xmlData); + } + catch(IException *e) + { + throw; + } + Owned tree = createPTreeFromXMLString(xmlData); + binaryData.clear(); + Owned out = createBufferedSerialOutputStream(binaryData); + tree->serializeToStream(*out); + out->flush(); + } + + bool readBinaryFile(const char *filePath, MemoryBuffer &output) + { + StringBuffer expandedPath; + const char *actualPath = expandTilde(filePath, expandedPath); + if (!actualPath) + return false; + Owned binaryFile = createIFile(actualPath); + if (!binaryFile->exists()) + return false; + + size32_t fileSize = (size32_t)binaryFile->size(); + Owned fileIO = binaryFile->open(IFOread); + output.reserveTruncate(fileSize); + size32_t bytesRead = fileIO->read(0, fileSize, output.bufferBase()); + output.setLength(bytesRead); + + const char *ext = pathExtension(actualPath); + if (ext && streq(ext, ".gz")) + { + StringBuffer tempOutput; + gunzip((const byte *)output.toByteArray(), output.length(), tempOutput); + output.clear(); + output.append(tempOutput.length(), tempOutput.str()); + } + + return true; + } + + TimingResults performBinaryTimingTestWithResults(const char *testName, const MemoryBuffer &binaryDataBuffer, int iterations, byte flags) + { + assertex(testName); + unsigned binaryDataLen = binaryDataBuffer.length(); + assertex(binaryDataLen > 0); + + CCycleTimer timer; + __uint64 totalDeserializeNs{0}; + __uint64 totalSerializeNs{0}; + + MemoryBuffer streamBufferIn; + MemoryBuffer streamBufferOut; + for (int i = 0; i < iterations; i++) + { + streamBufferIn.clear(); + streamBufferIn.append(binaryDataLen, binaryDataBuffer.toByteArray()); + Owned in = createBufferedSerialInputStream(streamBufferIn); + timer.reset(); + Owned deserializedTree = createPTreeFromBinary(*in, flags); + __uint64 deserializeElapsedNs = timer.elapsedNs(); + Owned copyDeserializedTree = createPTreeFromIPT(deserializedTree); + totalDeserializeNs += deserializeElapsedNs; + + streamBufferOut.clear(); + Owned out = createBufferedSerialOutputStream(streamBufferOut); + timer.reset(); + deserializedTree->serializeToStream(*out); + out->flush(); + __uint64 serializeElapsedNs = timer.elapsedNs(); + totalSerializeNs += serializeElapsedNs; + + // Validation - serialized data matches + CPPUNIT_ASSERT(areMatchingPTrees(copyDeserializedTree, deserializedTree)); + } + + TimingResults results; + results.avgDeserializeTimeMs = (totalDeserializeNs / iterations) / 1e6; + results.avgSerializeTimeMs = (totalSerializeNs / iterations) / 1e6; + results.totalDeserializeTimeMs = totalDeserializeNs / 1e6; + results.totalSerializeTimeMs = totalSerializeNs / 1e6; + results.testName = testName; + results.flags = flags; + + return results; + } + + TimingResults performXmlTimingTestWithResults(const char *testName, const char *xmlData, int iterations, byte flags) + { + assertex(testName); + assertex(xmlData); + + CCycleTimer timer; + __uint64 totalDeserializeNs{0}; + __uint64 totalSerializeNs{0}; + + for (int i = 0; i < iterations; i++) + { + timer.reset(); + Owned tree = createPTreeFromXMLString(xmlData, flags); + __uint64 deserializeElapsedNs = timer.elapsedNs(); + totalDeserializeNs += deserializeElapsedNs; + + timer.reset(); + StringBuffer xmlOutput; + toXML(tree, xmlOutput); + __uint64 serializeElapsedNs = timer.elapsedNs(); + totalSerializeNs += serializeElapsedNs; + } + + TimingResults results; + results.avgDeserializeTimeMs = (totalDeserializeNs / iterations) / 1e6; + results.avgSerializeTimeMs = (totalSerializeNs / iterations) / 1e6; + results.totalDeserializeTimeMs = totalDeserializeNs / 1e6; + results.totalSerializeTimeMs = totalSerializeNs / 1e6; + results.testName = testName; + results.flags = flags; + + return results; + } +}; + +// Test suite for XML timing tests +class PTreeXmlTimingStressTest : public PTreeTimingTestBase +{ + CPPUNIT_TEST_SUITE(PTreeXmlTimingStressTest); + CPPUNIT_TEST(testXmlTimingWithNormalVsLowMem); + CPPUNIT_TEST_SUITE_END(); + +public: + void testXmlTimingWithNormalVsLowMem() + { + constexpr const int iterations{100}; + + // Load XML data + StringBuffer xmlData; + readXmlFile(getXmlFilePath(), xmlData); + unsigned xmlDataLen = (unsigned)strlen(xmlData.str()); + + // Run XML timing tests + TimingResults xmlNormalResults = performXmlTimingTestWithResults("XML Normal", xmlData.str(), iterations, ipt_none); + TimingResults xmlLowMemResults = performXmlTimingTestWithResults("XML Low Memory", xmlData.str(), iterations, ipt_lowmem); + + // Calculate differences + double lowMemDeserializeDiff = ((xmlLowMemResults.avgDeserializeTimeMs - xmlNormalResults.avgDeserializeTimeMs) / xmlNormalResults.avgDeserializeTimeMs) * 100; + double lowMemSerializeDiff = ((xmlLowMemResults.avgSerializeTimeMs - xmlNormalResults.avgSerializeTimeMs) / xmlNormalResults.avgSerializeTimeMs) * 100; + + // Display results + fprintf(stdout, "\n=== XML TIMING COMPARISON TEST ===\n"); + fprintf(stdout, "XML data size: %u bytes\n", xmlDataLen); + fprintf(stdout, "Iterations: %d\n", iterations); + fprintf(stdout, "┌──────────────────────┬─────────────────┬─────────────────┐\n"); + fprintf(stdout, "│ Mode │ Avg Deserialize │ Avg Serialize │\n"); + fprintf(stdout, "│ │ (ms) │ (ms) │\n"); + fprintf(stdout, "├──────────────────────┼─────────────────┼─────────────────┤\n"); + fprintf(stdout, "│ XML Normal │ %15.6f │ %15.6f │\n", + xmlNormalResults.avgDeserializeTimeMs, + xmlNormalResults.avgSerializeTimeMs); + fprintf(stdout, "│ XML Low Memory │ %15.6f │ %15.6f │\n", + xmlLowMemResults.avgDeserializeTimeMs, + xmlLowMemResults.avgSerializeTimeMs); + fprintf(stdout, "│ XML LowMem Diff │ %+14.2f%% │ %+14.2f%% │\n", + lowMemDeserializeDiff, + lowMemSerializeDiff); + fprintf(stdout, "└──────────────────────┴─────────────────┴─────────────────┘\n"); + fflush(stdout); + } +}; + +// Test suite for Binary timing tests +class PTreeBinaryTimingStressTest : public PTreeTimingTestBase +{ + CPPUNIT_TEST_SUITE(PTreeBinaryTimingStressTest); + CPPUNIT_TEST(testBinaryTimingWithNormalVsLowMem); + CPPUNIT_TEST_SUITE_END(); + +public: + void testBinaryTimingWithNormalVsLowMem() + { + constexpr const int iterations{100}; + + // Load Binary data + // If the binary file does not exist then create it from the XML file + MemoryBuffer binaryData; + if (!readBinaryFile(getBinaryFilePath(), binaryData)) + createBinaryDataFromXml(getXmlFilePath(), binaryData); + unsigned binaryDataLen = (unsigned)binaryData.length(); + + // Run Binary timing tests + TimingResults binaryNormalResults = performBinaryTimingTestWithResults("Binary Normal", binaryData, iterations, ipt_none); + TimingResults binaryLowMemResults = performBinaryTimingTestWithResults("Binary Low Memory", binaryData, iterations, ipt_lowmem); + + // Calculate differences + double lowMemDeserializeDiff = ((binaryLowMemResults.avgDeserializeTimeMs - binaryNormalResults.avgDeserializeTimeMs) / binaryNormalResults.avgDeserializeTimeMs) * 100; + double lowMemSerializeDiff = ((binaryLowMemResults.avgSerializeTimeMs - binaryNormalResults.avgSerializeTimeMs) / binaryNormalResults.avgSerializeTimeMs) * 100; + + // Display results + fprintf(stdout, "\n=== BINARY TIMING COMPARISON TEST ===\n"); + fprintf(stdout, "Binary data size: %u bytes\n", binaryDataLen); + fprintf(stdout, "Iterations: %d\n", iterations); + fprintf(stdout, "┌──────────────────────┬─────────────────┬─────────────────┐\n"); + fprintf(stdout, "│ Mode │ Avg Deserialize │ Avg Serialize │\n"); + fprintf(stdout, "│ │ (ms) │ (ms) │\n"); + fprintf(stdout, "├──────────────────────┼─────────────────┼─────────────────┤\n"); + fprintf(stdout, "│ Binary Normal │ %15.6f │ %15.6f │\n", + binaryNormalResults.avgDeserializeTimeMs, + binaryNormalResults.avgSerializeTimeMs); + fprintf(stdout, "│ Binary Low Memory │ %15.6f │ %15.6f │\n", + binaryLowMemResults.avgDeserializeTimeMs, + binaryLowMemResults.avgSerializeTimeMs); + fprintf(stdout, "│ Binary LowMem Diff │ %+14.2f%% │ %+14.2f%% │\n", + lowMemDeserializeDiff, + lowMemSerializeDiff); + fprintf(stdout, "└──────────────────────┴─────────────────┴─────────────────┘\n"); + fflush(stdout); + } +}; + +// Test suite for combined XML and Binary timing tests +class PTreeCombinedTimingStressTest : public PTreeTimingTestBase +{ + CPPUNIT_TEST_SUITE(PTreeCombinedTimingStressTest); + CPPUNIT_TEST(testCombinedXmlAndBinaryTimingWithNormalVsLowMem); + CPPUNIT_TEST_SUITE_END(); + +public: + void testCombinedXmlAndBinaryTimingWithNormalVsLowMem() + { + constexpr const int iterations{100}; + + // Load XML data + StringBuffer xmlData; + readXmlFile(getXmlFilePath(), xmlData); + unsigned xmlDataLen = (unsigned)strlen(xmlData.str()); + + // Load Binary data + // If the binary file does not exist then create it from the XML file + MemoryBuffer binaryData; + if (!readBinaryFile(getBinaryFilePath(), binaryData)) + createBinaryDataFromXml(getXmlFilePath(), binaryData); + unsigned binaryDataLen = (unsigned)binaryData.length(); + + // Run all timing tests + TimingResults xmlNormalResults = performXmlTimingTestWithResults("XML Normal", xmlData.str(), iterations, ipt_none); + TimingResults xmlLowMemResults = performXmlTimingTestWithResults("XML Low Memory", xmlData.str(), iterations, ipt_lowmem); + TimingResults binaryNormalResults = performBinaryTimingTestWithResults("Binary Normal", binaryData, iterations, ipt_none); + TimingResults binaryLowMemResults = performBinaryTimingTestWithResults("Binary Low Memory", binaryData, iterations, ipt_lowmem); + + // Calculate all comparisons + double xmlLowMemDeserializeDiff = ((xmlLowMemResults.avgDeserializeTimeMs - xmlNormalResults.avgDeserializeTimeMs) / xmlNormalResults.avgDeserializeTimeMs) * 100; + double xmlLowMemSerializeDiff = ((xmlLowMemResults.avgSerializeTimeMs - xmlNormalResults.avgSerializeTimeMs) / xmlNormalResults.avgSerializeTimeMs) * 100; + double binaryLowMemDeserializeDiff = ((binaryLowMemResults.avgDeserializeTimeMs - binaryNormalResults.avgDeserializeTimeMs) / binaryNormalResults.avgDeserializeTimeMs) * 100; + double binaryLowMemSerializeDiff = ((binaryLowMemResults.avgSerializeTimeMs - binaryNormalResults.avgSerializeTimeMs) / binaryNormalResults.avgSerializeTimeMs) * 100; + double binaryVsXmlNormalDeserialize = ((binaryNormalResults.avgDeserializeTimeMs - xmlNormalResults.avgDeserializeTimeMs) / xmlNormalResults.avgDeserializeTimeMs) * 100; + double binaryVsXmlNormalSerialize = ((binaryNormalResults.avgSerializeTimeMs - xmlNormalResults.avgSerializeTimeMs) / xmlNormalResults.avgSerializeTimeMs) * 100; + double binaryVsXmlLowMemDeserialize = ((binaryLowMemResults.avgDeserializeTimeMs - xmlLowMemResults.avgDeserializeTimeMs) / xmlLowMemResults.avgDeserializeTimeMs) * 100; + double binaryVsXmlLowMemSerialize = ((binaryLowMemResults.avgSerializeTimeMs - xmlLowMemResults.avgSerializeTimeMs) / xmlLowMemResults.avgSerializeTimeMs) * 100; + + // Display combined results + fprintf(stdout, "\n=== COMBINED XML & BINARY TIMING COMPARISON TEST ===\n"); + fprintf(stdout, "XML data size: %u bytes\n", xmlDataLen); + fprintf(stdout, "Binary data size: %u bytes\n", binaryDataLen); + fprintf(stdout, "Iterations: %d\n", iterations); + fprintf(stdout, "┌──────────────────────┬─────────────────┬─────────────────┐\n"); + fprintf(stdout, "│ Format & Mode │ Avg Deserialize │ Avg Serialize │\n"); + fprintf(stdout, "│ │ (ms) │ (ms) │\n"); + fprintf(stdout, "├──────────────────────┼─────────────────┼─────────────────┤\n"); + fprintf(stdout, "│ XML Normal │ %15.6f │ %15.6f │\n", + xmlNormalResults.avgDeserializeTimeMs, + xmlNormalResults.avgSerializeTimeMs); + fprintf(stdout, "│ XML Low Memory │ %15.6f │ %15.6f │\n", + xmlLowMemResults.avgDeserializeTimeMs, + xmlLowMemResults.avgSerializeTimeMs); + fprintf(stdout, "│ XML LowMem Diff │ %+14.2f%% │ %+14.2f%% │\n", + xmlLowMemDeserializeDiff, + xmlLowMemSerializeDiff); + fprintf(stdout, "├──────────────────────┼─────────────────┼─────────────────┤\n"); + fprintf(stdout, "│ Binary Normal │ %15.6f │ %15.6f │\n", + binaryNormalResults.avgDeserializeTimeMs, + binaryNormalResults.avgSerializeTimeMs); + fprintf(stdout, "│ Binary Low Memory │ %15.6f │ %15.6f │\n", + binaryLowMemResults.avgDeserializeTimeMs, + binaryLowMemResults.avgSerializeTimeMs); + fprintf(stdout, "│ Binary LowMem Diff │ %+14.2f%% │ %+14.2f%% │\n", + binaryLowMemDeserializeDiff, + binaryLowMemSerializeDiff); + fprintf(stdout, "├──────────────────────┼─────────────────┼─────────────────┤\n"); + fprintf(stdout, "│ Binary vs XML Normal │ %+14.2f%% │ %+14.2f%% │\n", + binaryVsXmlNormalDeserialize, + binaryVsXmlNormalSerialize); + fprintf(stdout, "│ Binary vs XML LowMem │ %+14.2f%% │ %+14.2f%% │\n", + binaryVsXmlLowMemDeserialize, + binaryVsXmlLowMemSerialize); + fprintf(stdout, "└──────────────────────┴─────────────────┴─────────────────┘\n"); + fflush(stdout); + } +}; + +// Test suite for profiling Binary deserialization +class PTreeBinaryDeserializationProfilingStressTest : public PTreeTimingTestBase +{ + CPPUNIT_TEST_SUITE(PTreeBinaryDeserializationProfilingStressTest); + CPPUNIT_TEST(testBinaryLowMemDeserializationForProfiling); + CPPUNIT_TEST_SUITE_END(); + +public: + void testBinaryLowMemDeserializationForProfiling() + { + // This test is designed specifically for profiling tools + // It focuses solely on binary deserialization with low memory flags + // Run with: perf record -g ./unittests --test testBinaryLowMemDeserializationForProfiling + // Or with callgrind: valgrind --tool=callgrind ./unittests -e PTreeSerializationDeserializationXmlTimingStressTest + + constexpr const int iterations{1}; // More iterations for better profiling data + constexpr const byte flags = ipt_lowmem; + + // Load Binary data - DECOMPRESS BEFORE PROFILING STARTS + MemoryBuffer binaryData; + const char *binaryPath = getBinaryFilePath(); + try + { + // Read raw file (without automatic decompression) + if (!readBinaryFileRaw(binaryPath, binaryData)) + createBinaryDataFromXml(getXmlFilePath(), binaryData); + + // Decompress outside the profiled section to exclude gunzip from profiling + if (!decompressIfNeeded(binaryPath, binaryData)) + CPPUNIT_FAIL("Failed to decompress binary data"); + } + catch (IException *e) + { + StringBuffer msg; + e->errorMessage(msg); + e->Release(); + CPPUNIT_FAIL(msg.str()); + } + catch (...) + { + CPPUNIT_FAIL("Failed to load test data files"); + } + + unsigned binaryDataLen = binaryData.length(); + if (binaryDataLen == 0) + CPPUNIT_FAIL("Binary data is empty - test data files not found or empty"); + + DBGLOG("=== BINARY LOW MEMORY DESERIALIZATION PROFILING TEST ==="); + DBGLOG("Binary data size: %u bytes (decompressed)", binaryDataLen); + DBGLOG("Iterations: %d", iterations); + DBGLOG("Flags: 0x%02X (ipt_lowmem)", flags); + DBGLOG("Decompression completed BEFORE profiling begins"); + DBGLOG("This test is optimized for profiling - running deserialization only"); + + CCycleTimer timer; + __uint64 totalDeserializeNs{0}; + + MemoryBuffer streamBufferIn; + try + { + for (int i = 0; i < iterations; i++) + { + streamBufferIn.clear(); + streamBufferIn.append(binaryDataLen, binaryData.toByteArray()); + Owned in = createBufferedSerialInputStream(streamBufferIn); + + timer.reset(); + #ifdef CALLGRIND_PROFILING + CALLGRIND_START_INSTRUMENTATION; + CALLGRIND_TOGGLE_COLLECT; + #endif + Owned deserializedTree = createPTreeFromBinary(*in, flags); + #ifdef CALLGRIND_PROFILING + CALLGRIND_TOGGLE_COLLECT; + CALLGRIND_STOP_INSTRUMENTATION; + #endif + __uint64 deserializeElapsedNs = timer.elapsedNs(); + totalDeserializeNs += deserializeElapsedNs; + + // Keep a reference to prevent optimization + CPPUNIT_ASSERT(deserializedTree != nullptr); + } + } + catch (IException *e) + { + StringBuffer msg; + e->errorMessage(msg); + e->Release(); + CPPUNIT_FAIL(msg.str()); + } + catch (...) + { + CPPUNIT_FAIL("Unexpected exception during deserialization"); + } + + double avgDeserializeTimeMs = (totalDeserializeNs / iterations) / 1e6; + double totalDeserializeTimeMs = totalDeserializeNs / 1e6; + + DBGLOG("=== PROFILING TEST RESULTS ==="); + DBGLOG("Total deserialize time: %.6f ms", totalDeserializeTimeMs); + DBGLOG("Average deserialize time: %.6f ms", avgDeserializeTimeMs); + DBGLOG("Iterations per second: %.2f", 1000.0 / avgDeserializeTimeMs); + DBGLOG("=== PROFILING TEST COMPLETED ==="); + } + +private: + bool readBinaryFileRaw(const char *filePath, MemoryBuffer &output) + { + // Read file without decompression - for profiling tests + StringBuffer expandedPath; + const char *actualPath = expandTilde(filePath, expandedPath); + if (!actualPath) + return false; + Owned binaryFile = createIFile(actualPath); + if (!binaryFile->exists()) + return false; + + size32_t fileSize = (size32_t)binaryFile->size(); + Owned fileIO = binaryFile->open(IFOread); + output.reserveTruncate(fileSize); + size32_t bytesRead = fileIO->read(0, fileSize, output.bufferBase()); + output.setLength(bytesRead); + + return true; + } + + bool decompressIfNeeded(const char *filePath, MemoryBuffer &data) + { + // Decompress if file has .gz extension + StringBuffer expandedPath; + const char *actualPath = expandTilde(filePath, expandedPath); + if (!actualPath) + return false; + + const char *ext = pathExtension(actualPath); + if (ext && streq(ext, ".gz")) + { + StringBuffer tempOutput; + gunzip((const byte *)data.toByteArray(), data.length(), tempOutput); + data.clear(); + data.append(tempOutput.length(), tempOutput.str()); + } + + return true; + } +}; + +CPPUNIT_TEST_SUITE_REGISTRATION(PTreeXmlTimingStressTest); +CPPUNIT_TEST_SUITE_NAMED_REGISTRATION(PTreeXmlTimingStressTest, "PTreeXmlTimingStressTest"); +CPPUNIT_TEST_SUITE_REGISTRATION(PTreeBinaryTimingStressTest); +CPPUNIT_TEST_SUITE_NAMED_REGISTRATION(PTreeBinaryTimingStressTest, "PTreeBinaryTimingStressTest"); +CPPUNIT_TEST_SUITE_REGISTRATION(PTreeCombinedTimingStressTest); +CPPUNIT_TEST_SUITE_NAMED_REGISTRATION(PTreeCombinedTimingStressTest, "PTreeCombinedTimingStressTest"); +CPPUNIT_TEST_SUITE_REGISTRATION(PTreeBinaryDeserializationProfilingStressTest); +CPPUNIT_TEST_SUITE_NAMED_REGISTRATION(PTreeBinaryDeserializationProfilingStressTest, "PTreeBinaryDeserializationProfilingStressTest"); + #include "jdebug.hpp" #include "jmutex.hpp" #include From 3cc92fdb4950a52653ec3070e5b054fad180c358 Mon Sep 17 00:00:00 2001 From: Dave Streeter Date: Fri, 24 Oct 2025 16:59:21 +0100 Subject: [PATCH 5/5] setAttribute vector WIP Signed-off-by: Dave Streeter --- system/jlib/jptree.cpp | 144 +++++++++++++++++++++++++++++- testing/unittests/jlibtests.cpp | 151 ++++++++++++++++++++++++++++++++ 2 files changed, 294 insertions(+), 1 deletion(-) diff --git a/system/jlib/jptree.cpp b/system/jlib/jptree.cpp index 242b763a554..0bfdd8d7625 100644 --- a/system/jlib/jptree.cpp +++ b/system/jlib/jptree.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include "platform.h" #include "jarray.hpp" @@ -3100,7 +3101,7 @@ void PTree::deserializeSelf(IBufferedSerialInputStream &src) throwUnexpectedX("PTree deserialization error: end of stream, expected attribute value"); */ setAttribute(base, attrStringOffsets); -/* DJPS +/* constexpr bool attributeNameNotEncoded = false; // Deserialized attribute name is in its original unencoded form for (size_t i = 0; i < numStringOffsets; i += 2) { @@ -4159,6 +4160,7 @@ void CAtomPTree::setAttribute(const char *key, const char *val, bool encoded) } } +/* void CAtomPTree::setAttribute(const char *base, const std::vector &offsets) { size_t numStringOffsets = offsets.size(); @@ -4225,6 +4227,146 @@ void CAtomPTree::setAttribute(const char *base, const std::vector &off } } } +*/ + +void CAtomPTree::setAttribute(const char *base, const std::vector &offsets) +{ + size_t numStringOffsets = offsets.size(); + if (numStringOffsets == 0) + return; + + if (numStringOffsets % 2 != 0) + throwUnexpectedX("setAttribute error: offsets vector must contain pairs of key/value offsets"); + + CriticalBlock block(hashcrit); + + // Pre-pass for new attributes to allocate them in one call + // Count the new attributes + std::set newAttributesMap; + std::unordered_set *processed{nullptr}; + for (size_t i = 0; i < numStringOffsets; i += 2) + { + const char *key = base + offsets[i]; + if (!key) + continue; + if (!validateXMLTag(key+1)) + throw MakeIPTException(-1, "Invalid xml attribute: %s", key); + + AttrValue *v = findAttribute(key); + if (!v) + newAttributesMap.emplace(i); + } + size_t numNewAttributes = newAttributesMap.size(); + AttrValue *newattrs{nullptr}; + if (numNewAttributes) + { + // Allocate the new attributes in one call + newattrs = newAttrArray(numAttrs+numNewAttributes); + assertex(newattrs); + if (attrs) + { + memcpy(newattrs, attrs, numAttrs*sizeof(AttrValue)); + freeAttrArray(attrs, numAttrs); + } + + // process new additions only + size_t newAttributeIndex{0}; + processed = new std::unordered_set(); + for (size_t i : newAttributesMap) + { + const char *key = base + offsets[i]; + const char *val = base + offsets[i + 1]; + + if (!val) + val = ""; // cannot have NULL value + + if (arrayOwner) + { + CQualifierMap *map = arrayOwner->queryMap(); + if (map) + map->insertEntryIfMapped(key, val, this); + } + AttrValue *v = &newattrs[numAttrs + newAttributeIndex]; + assertex(v); + if (!v->key.set(key)) + v->key.setPtr(attrHT->addkey(key, isnocase())); + if (!v->value.set(val)) + v->value.setPtr(attrHT->addval(val)); + + ++newAttributeIndex; + processed->emplace(i); + } + attrs = newattrs; + numAttrs += numNewAttributes; + } + + // Process existing attributes + for (size_t i = 0; i < numStringOffsets; i += 2) + { + if (processed && (processed->find(i) != processed->end())) // ignore new attributes + continue; + + const char *key = base + offsets[i]; + const char *val = base + offsets[i + 1]; + + if (!key) + continue; + if (!val) + val = ""; // cannot have NULL value + AttrValue *v = findAttribute(key); + if (v) + { + if (streq(v->value.get(), val)) + continue; + if (arrayOwner) + { + CQualifierMap *map = arrayOwner->queryMap(); + if (map) + map->replaceEntryIfMapped(key, v->value.get(), val, this); + } + + AttrStr * goer = v->value.getPtr(); + if (!v->value.set(val)) + { + if (goer) + attrHT->removeval(goer); + v->value.setPtr(attrHT->addval(val)); + } + else if (goer) + { + attrHT->removeval(goer); + } + } + else + { + assertex(false); + +/* + AttrValue *newattrs = newAttrArray(numAttrs+1); + if (attrs) + { + memcpy(newattrs, attrs, numAttrs*sizeof(AttrValue)); + freeAttrArray(attrs, numAttrs); + } + if (arrayOwner) + { + CQualifierMap *map = arrayOwner->queryMap(); + if (map) + map->insertEntryIfMapped(key, val, this); + } + v = &newattrs[numAttrs]; + if (!v->key.set(key)) + v->key.setPtr(attrHT->addkey(key, isnocase())); + if (!v->value.set(val)) + v->value.setPtr(attrHT->addval(val)); + numAttrs++; + attrs = newattrs; +*/ + + } + } + delete processed; +} bool CAtomPTree::removeAttribute(const char *key) { diff --git a/testing/unittests/jlibtests.cpp b/testing/unittests/jlibtests.cpp index 186101fffe2..19b238f6f17 100644 --- a/testing/unittests/jlibtests.cpp +++ b/testing/unittests/jlibtests.cpp @@ -3417,6 +3417,7 @@ CPPUNIT_TEST_SUITE_NAMED_REGISTRATION(JlibIPTTest, "JlibIPTTest"); #include "jio.hpp" #include "jstring.hpp" #include +#include IPropertyTree *createCompatibilityConfigPropertyTree() { @@ -3635,6 +3636,9 @@ class PTreeSerializationDeserializationTest : public CppUnit::TestFixture CPPUNIT_TEST(testRoundTripForRootOnlyPTree_lowmem); CPPUNIT_TEST(testRoundTripForCompatibilityConfigPropertyTree_lowmem); CPPUNIT_TEST(testRoundTripForBinaryDataCompressionTestPTree_lowmem); + // Direct tests for vector-based setAttribute (tests both new and update paths) + CPPUNIT_TEST(testSetAttributeWithVector); + CPPUNIT_TEST(testSetAttributeWithVector_lowmem); CPPUNIT_TEST_SUITE_END(); protected: @@ -3803,6 +3807,153 @@ class PTreeSerializationDeserializationTest : public CppUnit::TestFixture { performRoundTripTest(__func__, createBinaryDataCompressionTestPTree(), ipt_lowmem); } + + // Direct tests for vector-based setAttribute function + // Tests both new attribute insertion and existing attribute update code paths + void testSetAttributeWithVector() + { + testSetAttributeWithVectorImpl(ipt_none); + } + + void testSetAttributeWithVector_lowmem() + { + testSetAttributeWithVectorImpl(ipt_lowmem); + } + +private: + void testSetAttributeWithVectorImpl(byte flags) + { + try + { + // Create a simple tree for testing + Owned tree = createPTree("TestRoot", flags); + + // Cast to PTree* to access the internal setAttribute(base, vector) method + PTree *ptree = dynamic_cast(tree.get()); + CPPUNIT_ASSERT_MESSAGE("Failed to cast IPropertyTree to PTree", ptree != nullptr); + + // Build a string buffer containing key/value pairs and a vector of offsets + // Format matches what peekStringList produces in deserializeSelf: + // - String buffer contains null-terminated strings + // - Vector contains pairs: [key_offset, value_offset, key_offset, value_offset, ...] + + MemoryBuffer attrData; + std::vector offsets; + + // Test 1: Add new attributes (tests "new attribute" code path) + DBGLOG("=== Test 1: Adding new attributes ==="); + + // Add attribute: @newAttr1="newValue1" + offsets.push_back(attrData.length()); + attrData.append("@newAttr1").append('\0'); + + offsets.push_back(attrData.length()); + attrData.append("newValue1").append('\0'); + + // Add attribute: @newAttr2="newValue2" + offsets.push_back(attrData.length()); + attrData.append("@newAttr2").append('\0'); + + offsets.push_back(attrData.length()); + attrData.append("newValue2").append('\0'); + + DBGLOG("Calling setAttribute with %u offsets", (unsigned)offsets.size()); // Call setAttribute with the vector (testing new attribute insertion) + const char *base = (const char *)attrData.toByteArray(); + ptree->setAttribute(base, offsets); + + DBGLOG("setAttribute completed for new attributes"); + + // Verify new attributes were added using areMatchingPTrees + Owned expected1 = createPTree("TestRoot", flags); + expected1->setProp("@newAttr1", "newValue1"); + expected1->setProp("@newAttr2", "newValue2"); + CPPUNIT_ASSERT_MESSAGE("Test 1: Tree doesn't match expected after adding new attributes", areMatchingPTrees(expected1, tree)); + + DBGLOG("Test 1 passed"); + + // Test 2: Update existing attributes (tests "update existing attribute" code path) + DBGLOG("=== Test 2: Updating existing attributes ==="); + attrData.clear(); + offsets.clear(); + + // Update attribute: @newAttr1="updatedValue1" + offsets.push_back(attrData.length()); + attrData.append("@newAttr1").append('\0'); + + offsets.push_back(attrData.length()); + attrData.append("updatedValue1").append('\0'); + + // Add one more new attribute: @newAttr3="newValue3" + offsets.push_back(attrData.length()); + attrData.append("@newAttr3").append('\0'); + + offsets.push_back(attrData.length()); + attrData.append("newValue3").append('\0'); + + DBGLOG("Calling setAttribute to update 1 existing + add 1 new"); + + // Call setAttribute again (testing both update and new attribute paths) + base = (const char *)attrData.toByteArray(); + ptree->setAttribute(base, offsets); + + DBGLOG("setAttribute completed for updates"); + + // Verify updates using areMatchingPTrees + Owned expected2 = createPTree("TestRoot", flags); + expected2->setProp("@newAttr1", "updatedValue1"); // Updated value + expected2->setProp("@newAttr2", "newValue2"); // Unchanged from Test 1 + expected2->setProp("@newAttr3", "newValue3"); // Newly added + CPPUNIT_ASSERT_MESSAGE("Test 2: Tree doesn't match expected after updating attributes", areMatchingPTrees(expected2, tree)); + + DBGLOG("Test 2 passed"); + + // Test 3: Update attributes that were added via setProp (exercises the existing attribute update path) + DBGLOG("=== Test 3: Testing setAttribute on pre-existing attributes ==="); + + // Add attributes using traditional setProp method + tree->setProp("@preExisting", "original"); + + // Now update it using setAttribute with vector + attrData.clear(); + offsets.clear(); + + offsets.push_back(attrData.length()); + attrData.append("@preExisting").append('\0'); + offsets.push_back(attrData.length()); + attrData.append("modified").append('\0'); + + DBGLOG("Calling setAttribute to update pre-existing attribute"); + + base = (const char *)attrData.toByteArray(); + ptree->setAttribute(base, offsets); + + // Verify the update worked using areMatchingPTrees + Owned expected3 = createPTree("TestRoot", flags); + expected3->setProp("@newAttr1", "updatedValue1"); + expected3->setProp("@newAttr2", "newValue2"); + expected3->setProp("@newAttr3", "newValue3"); + expected3->setProp("@preExisting", "modified"); // Updated via setAttribute + CPPUNIT_ASSERT_MESSAGE("Test 3: Tree doesn't match expected after updating pre-existing attribute", areMatchingPTrees(expected3, tree)); + + DBGLOG("Test 3 passed"); + DBGLOG("=== testSetAttributeWithVector completed successfully ==="); + } + catch (IException *e) + { + StringBuffer msg; + e->errorMessage(msg); + e->Release(); + CPPUNIT_FAIL(StringBuffer("IException in testSetAttributeWithVector: ").append(msg).str()); + } + catch (std::exception &e) + { + CPPUNIT_FAIL(StringBuffer("std::exception in testSetAttributeWithVector: ").append(e.what()).str()); + } + catch (...) + { + CPPUNIT_FAIL("Unknown exception in testSetAttributeWithVector"); + } + } }; CPPUNIT_TEST_SUITE_REGISTRATION(PTreeSerializationDeserializationTest);