From 761a6a36acd808a5f68ced6724e9cc99891c6c43 Mon Sep 17 00:00:00 2001 From: Ziyan Date: Wed, 28 Mar 2018 17:16:51 +0900 Subject: [PATCH 1/3] Remove gtest. --- .gitmodules | 3 --- thirdparty/gtest | 1 - 2 files changed, 4 deletions(-) delete mode 100644 .gitmodules delete mode 160000 thirdparty/gtest diff --git a/.gitmodules b/.gitmodules deleted file mode 100644 index 5e41f7c97..000000000 --- a/.gitmodules +++ /dev/null @@ -1,3 +0,0 @@ -[submodule "thirdparty/gtest"] - path = thirdparty/gtest - url = https://github.com/google/googletest.git diff --git a/thirdparty/gtest b/thirdparty/gtest deleted file mode 160000 index 0a439623f..000000000 --- a/thirdparty/gtest +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 0a439623f75c029912728d80cb7f1b8b48739ca4 From 8199640728d67701fcee479704b29d5592cc011f Mon Sep 17 00:00:00 2001 From: Ross Schlaikjer Date: Mon, 4 Dec 2023 16:55:14 +0900 Subject: [PATCH 2/3] Add LUT to speed up FindMember / Mujin's SetJsonValueByKey RapidJson's `AddMember` API, presumably by design, is append-only. That is to say, if ever it is called twice with the same key, it will produce a document with duplicate members for that key instead of overwriting a previous member. This means that AddMember can be fast (no check on whether a key exists), but in reality it just pushes the check further up the chain: Mujin's `SetJsonValueByKey` method will first look up whether a key exists, and then overwrite if it exists and create if it does not. However, `FindMember` (the real implementation behind `HasMember`) utilizes a linear search approach - it has to iterate every member in the document and perform a key compare. This means that constructing a document of N elements ends up exhibiting roughly polynomial time complexity. To address this, this patch would add a hashtable to each object that associates the first member with a given key in the map with its actual address inside the object. This means that member lookup is now _constant_ time, but requires additional memory and tracking overhead. The memory overhead is noticeable - while the cache only uses views over string data, the additional pointer members adds far more weight than expected. It is possible this could be reduced by gating this optimization at runtime (i.e, only build the cache for objects with more than some threshold member count). Some chunk of this overhead could actually be removed if we disallowed duplicate keys entirely - element deletion requires iteration of all subsequent keys in case they were a previously shadowed duplicate key member. Performance was measured using the Zozo robotbridges scene with 6k parts added to the environment. Without this patch: - Total RSS: 442MB - Average call to _SavePublishState: 98'278[us] With this patch: - Total RSS: 1'384MB - Average call to _SavePublishState: 10'622[us] --- include/rapidjson/document.h | 265 ++++++++++++++++++++++++++++++++--- 1 file changed, 247 insertions(+), 18 deletions(-) diff --git a/include/rapidjson/document.h b/include/rapidjson/document.h index 094a07e82..94b255e3e 100644 --- a/include/rapidjson/document.h +++ b/include/rapidjson/document.h @@ -24,6 +24,7 @@ #include "encodedstream.h" #include // placement new #include +#include RAPIDJSON_DIAG_PUSH #ifdef _MSC_VER @@ -52,6 +53,10 @@ RAPIDJSON_DIAG_OFF(terminate) // ignore throwing RAPIDJSON_ASSERT in RAPIDJSON_N #include // std::move #endif +#if __cplusplus >= 201703L +#include +#endif + RAPIDJSON_NAMESPACE_BEGIN // Forward declaration. @@ -73,6 +78,36 @@ struct GenericMember { GenericValue value; //!< value of member. }; +// C++ standard library compatible allocator shim for the rapidjson allocator +template +struct CxxAllocatorWrapper +{ + using value_type = T; + CxxAllocatorWrapper(Allocator* rapidjsonAllocator) noexcept + : _allocator(rapidjsonAllocator) + { + } + + T* allocate(std::size_t n) + { + T* ptr = static_cast(_allocator->Malloc(n * sizeof(T))); + return ptr; + } + + template + CxxAllocatorWrapper(const CxxAllocatorWrapper& other) + : _allocator(other._allocator) + { + } + + void deallocate(T* p, __attribute__((unused)) std::size_t n) + { + _allocator->Free(p); + } + + Allocator* const _allocator; +}; + /////////////////////////////////////////////////////////////////////////////// // GenericMemberIterator @@ -534,6 +569,57 @@ struct TypeHelper { template class GenericArray; template class GenericObject; +// If we don't have access to std::string view, create our own wrapper struct +#if __cplusplus < 201703L +template +struct StringView { + StringView(const Ch* str) + : str_(str) { + } + + bool operator==(const StringView& other) const { + const Ch* s1 = str_; + const Ch* s2 = other.str_; + + // While both strings are non-\0, check to see chars match. If not, unequal. + while (*s1 && *s2) { + if (!std::char_traits::eq(*s1, *s2)) { + return false; + } + s1++; + s2++; + } + + // If we broke the loop, the strings are equal if they both point to \0 + return *s1 == *s2; + } + + const Ch* str_; +}; + +// Also need a hash specialization so that we can use it in unordered map +RAPIDJSON_NAMESPACE_END +namespace std { +template +struct hash<::RAPIDJSON_NAMESPACE::StringView> { + std::size_t operator()(const ::RAPIDJSON_NAMESPACE::StringView &key) const { + // Use FNV hash since it's fast and easy to write and we don't want to pull in any external deps here. + const Ch* s = key.str_; + uint64_t hval = 0; + while (*s) { + /* multiply by the 64 bit FNV magic prime mod 2^64 */ + hval += (hval << 1) + (hval << 4) + (hval << 5) + + (hval << 7) + (hval << 8) + (hval << 40); + /* xor the bottom with the current octet */ + hval ^= (uint64_t)*s++; + } + return hval; + } +}; +} +RAPIDJSON_NAMESPACE_BEGIN +#endif + /////////////////////////////////////////////////////////////////////////////// // GenericValue @@ -574,8 +660,9 @@ class GenericValue { #if RAPIDJSON_HAS_CXX11_RVALUE_REFS //! Move constructor in C++11 - GenericValue(GenericValue&& rhs) RAPIDJSON_NOEXCEPT : data_(rhs.data_) { + GenericValue(GenericValue&& rhs) RAPIDJSON_NOEXCEPT : data_(rhs.data_), memberPtrsByName_(rhs.memberPtrsByName_) { rhs.data_.f.flags = kNullFlag; // give up contents + rhs.memberPtrsByName_ = nullptr; // we take ownership of the member lut } #endif @@ -634,7 +721,7 @@ class GenericValue { } data_.f.flags = kObjectFlag; data_.o.size = data_.o.capacity = count; - SetMembersPointer(lm); + SetMembersPointer(lm, allocator); } break; case kArrayType: { @@ -762,9 +849,10 @@ class GenericValue { \note \c Object is always pass-by-value. \note the source object is moved into this value and the sourec object becomes empty. */ - GenericValue(Object o) RAPIDJSON_NOEXCEPT : data_(o.value_.data_) { + GenericValue(Object o) RAPIDJSON_NOEXCEPT : data_(o.value_.data_), memberPtrsByName_(o.value_.memberPtrsByName_) { o.value_.data_ = Data(); o.value_.data_.f.flags = kObjectFlag; + o.value_.memberPtrsByName_ = nullptr; } //! Destructor. @@ -772,6 +860,12 @@ class GenericValue { */ ~GenericValue() { if (Allocator::kNeedFree) { // Shortcut by Allocator's trait + // Clear our member lookup table + if (memberPtrsByName_) { + memberPtrsByName_->~LutMap(); + Allocator::Free(memberPtrsByName_); + } + switch(data_.f.flags) { case kArrayFlag: { @@ -796,6 +890,9 @@ class GenericValue { break; // Do nothing for other types. } } + + // Always write back nullptr to the members ptr + memberPtrsByName_ = nullptr; } //@} @@ -1150,7 +1247,8 @@ class GenericValue { GenericValue& MemberReserve(SizeType newCapacity, Allocator &allocator) { RAPIDJSON_ASSERT(IsObject()); if (newCapacity > data_.o.capacity) { - SetMembersPointer(reinterpret_cast(allocator.Realloc(GetMembersPointer(), data_.o.capacity * sizeof(Member), newCapacity * sizeof(Member)))); + SetMembersPointer(reinterpret_cast(allocator.Realloc(GetMembersPointer(), data_.o.capacity * sizeof(Member), newCapacity * sizeof(Member))), allocator); + data_.o.capacity = newCapacity; } return *this; @@ -1226,11 +1324,19 @@ class GenericValue { MemberIterator FindMember(const GenericValue& name) { RAPIDJSON_ASSERT(IsObject()); RAPIDJSON_ASSERT(name.IsString()); - MemberIterator member = MemberBegin(); - for ( ; member != MemberEnd(); ++member) - if (name.StringEqual(member->name)) - break; - return member; + + // If the members pointer is null, we have no members, we will never find anything + if (!GetMembersPointer()) { + return MemberEnd(); + } + + // Use our cache map to look up this member + RAPIDJSON_ASSERT(!!memberPtrsByName_); + auto it = memberPtrsByName_->find(name.GetString()); + if (it != memberPtrsByName_->end()) { + return MemberIterator(it->second); + } + return MemberEnd(); } template ConstMemberIterator FindMember(const GenericValue& name) const { return const_cast(*this).FindMember(name); } @@ -1266,6 +1372,12 @@ class GenericValue { Member* members = GetMembersPointer(); members[o.size].name.RawAssign(name); members[o.size].value.RawAssign(value); + + // Add this member to our lookup table. + // In the event of duplicate keys, first write wins. This emulates the linear seek behaviour of the old FindMember. + RAPIDJSON_ASSERT(!!memberPtrsByName_); + memberPtrsByName_->emplace(members[o.size].name.GetString(), &members[o.size]); + o.size++; return *this; } @@ -1403,6 +1515,7 @@ class GenericValue { for (MemberIterator m = MemberBegin(); m != MemberEnd(); ++m) m->~Member(); data_.o.size = 0; + memberPtrsByName_->clear(); } //! Remove a member in object by its name. @@ -1446,13 +1559,57 @@ class GenericValue { RAPIDJSON_ASSERT(data_.o.size > 0); RAPIDJSON_ASSERT(GetMembersPointer() != 0); RAPIDJSON_ASSERT(m >= MemberBegin() && m < MemberEnd()); + RAPIDJSON_ASSERT(!!memberPtrsByName_); MemberIterator last(GetMembersPointer() + (data_.o.size - 1)); - if (data_.o.size > 1 && m != last) + if (data_.o.size > 1 && m != last) { + // Delete lookup value for old name IFF it pointed to the erased value + auto it = memberPtrsByName_->find(m->name.GetString()); + bool didErase = false; + if (it->second == &*m) { + // This was previously the first hit for this key. Remove the entry. + memberPtrsByName_->erase(m->name.GetString()); + didErase = true; + } + *m = *last; // Move the last one to this place - else - m->~Member(); // Only one left, just destroy - --data_.o.size; + + // Decrement the size to effectively delete the last element. + // We need to do this before we check for previously shadowed keys so that we ignore the swapped final element. + --data_.o.size; + + // Point the swapped value at the correct location + (*memberPtrsByName_)[m->name.GetString()] = &*m; + + // If we erased a key earlier, then in order to be compatible with objects with duplicated keys, + // we have to scan the remainder of the object and test if any other keys are emplacable now. + if (didErase) { + for (MemberIterator shadow = m; shadow < MemberEnd(); shadow++) { + const auto& emplaceResult = memberPtrsByName_->emplace(shadow->name.GetString(), &*shadow); + // If we did emplace a value, we must have re-filled a previously shadowed name. + // Since there should only ever be one key that is unshadowed after a single delete, if we see this happen, we can short circuit checking the rest of the object. + if (emplaceResult.second) { + break; + } + } + } + } + else { + // If this is the leading key for a name, delete it. + // We don't need to check successors since + // - If this is the last key there can't be any successors, and + // - If this is the only key there are no other members to shadow + auto it = memberPtrsByName_->find(m->name.GetString()); + if (it->second == &*m) { + memberPtrsByName_->erase(m->name.GetString()); + } + + // Data to erase is either the only element or the final element. + // We can just destruct the element, and decreasing the size of the object will effectively drop it. + m->~Member(); + --data_.o.size; + } + return m; } @@ -1485,12 +1642,38 @@ class GenericValue { RAPIDJSON_ASSERT(first >= MemberBegin()); RAPIDJSON_ASSERT(first <= last); RAPIDJSON_ASSERT(last <= MemberEnd()); + RAPIDJSON_ASSERT(!!memberPtrsByName_); MemberIterator pos = MemberBegin() + (first - MemberBegin()); - for (MemberIterator itr = pos; itr != last; ++itr) + + // Erase all lookup entries from the start member to the end of the object. + // Since we only store string references in our lookup table, and memmove'ing the members rearranges those referents if they are short strings, we need to perform this step early. + for (MemberIterator itr = pos; itr != MemberEnd(); ++itr) { + // Only erase if the lookup value for this member matches the pointer address. + // If we have duplocate keys, and the first occurrence is before the erase, we want to leave it be. + auto it = memberPtrsByName_->find(itr->name.GetString()); + if (it->second == &*itr) { + memberPtrsByName_->erase(it); + } + } + + // Now go through and deconstruct all objects within the erased slice + for (MemberIterator itr = pos; itr != last; ++itr) { + // Deconstruct the deleted member itr->~Member(); + } + + // Move any following members down to keep member array contiguous std::memmove(&*pos, &*last, static_cast(MemberEnd() - last) * sizeof(Member)); + data_.o.size -= static_cast(last - first); + + // Update the LUT pointers for all members that got moved. + // Use emplace to preserve first-write-wins ordering. + for (MemberIterator itr = pos; itr != MemberEnd(); ++itr) { + memberPtrsByName_->emplace(itr->name.GetString(), &*itr); + } + return pos; } @@ -2000,7 +2183,38 @@ class GenericValue { RAPIDJSON_FORCEINLINE GenericValue* GetElementsPointer() const { return RAPIDJSON_GETPOINTER(GenericValue, data_.a.elements); } RAPIDJSON_FORCEINLINE GenericValue* SetElementsPointer(GenericValue* elements) { return RAPIDJSON_SETPOINTER(GenericValue, data_.a.elements, elements); } RAPIDJSON_FORCEINLINE Member* GetMembersPointer() const { return RAPIDJSON_GETPOINTER(Member, data_.o.members); } - RAPIDJSON_FORCEINLINE Member* SetMembersPointer(Member* members) { return RAPIDJSON_SETPOINTER(Member, data_.o.members, members); } + RAPIDJSON_FORCEINLINE Member* SetMembersPointer(Member* members, Allocator& allocator) { + Member* const ret = RAPIDJSON_SETPOINTER(Member, data_.o.members, members); + + // If the members pointer is invalid, destroy the member cache if it exists + if (!members) { + if (Allocator::kNeedFree && !!memberPtrsByName_) { + memberPtrsByName_->~LutMap(); + Allocator::Free(memberPtrsByName_); + memberPtrsByName_ = nullptr; + } + return ret; + } + + // If the members pointer _is_ valid, and we don't have a lookup table allocated, create one now + if (!memberPtrsByName_) { + memberPtrsByName_ = static_cast(allocator.Malloc(sizeof(LutMap))); + new (memberPtrsByName_) LutMap(LutMapAllocator{&allocator}); + } + + // If we already had a map, clear it since the members have been realloced and their addresses have changed + else { + memberPtrsByName_->clear(); + } + + // Rebuild LUT based on new member addresses + // Note that we iterate the input pointer _not_ the member pointer, since the member pointer may be munged by the 48 bit pointer optimization + for (SizeType i = 0; i < data_.o.size; i++) { + memberPtrsByName_->emplace(members[i].name.GetString(), &members[i]); + } + + return ret; + } // Initialize this value as array with initial data, without calling destructor. void SetArrayRaw(GenericValue* values, SizeType count, Allocator& allocator) { @@ -2018,14 +2232,14 @@ class GenericValue { //! Initialize this value as object with initial data, without calling destructor. void SetObjectRaw(Member* members, SizeType count, Allocator& allocator) { data_.f.flags = kObjectFlag; + data_.o.size = data_.o.capacity = count; if (count) { Member* m = static_cast(allocator.Malloc(count * sizeof(Member))); - SetMembersPointer(m); std::memcpy(m, members, count * sizeof(Member)); + SetMembersPointer(m, allocator); } else - SetMembersPointer(0); - data_.o.size = data_.o.capacity = count; + SetMembersPointer(0, allocator); } //! Initialize this value as constant string, without calling destructor. @@ -2057,6 +2271,10 @@ class GenericValue { data_ = rhs.data_; // data_.f.flags = rhs.data_.f.flags; rhs.data_.f.flags = kNullFlag; + + // Take ownership of the RHS's member lut + memberPtrsByName_ = rhs.memberPtrsByName_; + rhs.memberPtrsByName_ = nullptr; } template @@ -2076,6 +2294,17 @@ class GenericValue { } Data data_; + +#if __cplusplus >= 201703L + using LutMapKey = std::basic_string_view; +#else + using LutMapKey = StringView; +#endif + using LutMapValue = Member*; + using LutMapTuple = std::pair; + using LutMapAllocator = CxxAllocatorWrapper; + using LutMap = std::unordered_map, std::equal_to, LutMapAllocator>; + LutMap* memberPtrsByName_ = nullptr; }; //! GenericValue with UTF8 encoding From 1192ada120362da8afff192ab04d8cc0e102ecb8 Mon Sep 17 00:00:00 2001 From: Ross Schlaikjer Date: Mon, 4 Dec 2023 17:23:27 +0900 Subject: [PATCH 3/3] Add additional test on findmember consistency --- test/unittest/valuetest.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/unittest/valuetest.cpp b/test/unittest/valuetest.cpp index 307e1b06d..997de8e45 100644 --- a/test/unittest/valuetest.cpp +++ b/test/unittest/valuetest.cpp @@ -1403,6 +1403,7 @@ static void TestObject(T& x, Allocator& allocator) { size_t i = static_cast((itr - x.MemberBegin())) + 1; EXPECT_STREQ(itr->name.GetString(), keys[i]); EXPECT_EQ(i, itr->value[0].GetInt()); + EXPECT_EQ(itr, x.FindMember(itr->name.GetString())); } // Erase the last @@ -1414,6 +1415,7 @@ static void TestObject(T& x, Allocator& allocator) { size_t i = static_cast(itr - x.MemberBegin()) + 1; EXPECT_STREQ(itr->name.GetString(), keys[i]); EXPECT_EQ(i, itr->value[0].GetInt()); + EXPECT_EQ(itr, x.FindMember(itr->name.GetString())); } // Erase the middle @@ -1426,6 +1428,7 @@ static void TestObject(T& x, Allocator& allocator) { i += (i < 4) ? 1 : 2; EXPECT_STREQ(itr->name.GetString(), keys[i]); EXPECT_EQ(i, itr->value[0].GetInt()); + EXPECT_EQ(itr, x.FindMember(itr->name.GetString())); } // EraseMember(ConstMemberIterator, ConstMemberIterator)