diff --git a/renderdoc/api/replay/rdcarray.h b/renderdoc/api/replay/rdcarray.h index f71ccb3dc..2c6c8df36 100644 --- a/renderdoc/api/replay/rdcarray.h +++ b/renderdoc/api/replay/rdcarray.h @@ -85,6 +85,11 @@ struct ItemCopyHelper for(size_t i = 0; i < count; i++) new(dest + i) T(src[i]); } + static void moveRange(T *dest, T *src, size_t count) + { + for(size_t i = 0; i < count; i++) + new(dest + i) T(std::move(src[i])); + } }; template @@ -94,6 +99,10 @@ struct ItemCopyHelper { memcpy(dest, src, count * sizeof(T)); } + static void moveRange(T *dest, const T *src, size_t count) + { + memcpy(dest, src, count * sizeof(T)); + } }; // ItemDestroyHelper checks if the destructor is trivial/do-nothing and can be skipped @@ -235,13 +244,13 @@ public: if(elems) { // copy the elements to new storage - ItemCopyHelper::copyRange(newElems, elems, usedCount); + ItemCopyHelper::moveRange(newElems, elems, usedCount); // delete the old elements ItemDestroyHelper::destroyRange(elems, usedCount); } - // deallocate tee old storage + // deallocate the old storage deallocate(elems); // swap the storage. usedCount doesn't change @@ -298,7 +307,36 @@ public: setUsedCount(usedCount + 1); } - // fill the string with 'count' copies of 'el' + void push_back(T &&el) + { + // if we're pushing from within the array, save the index and move from that index after + // potentially resizing + if(begin() <= &el && &el < end()) + { + size_t idx = &el - begin(); + const size_t lastIdx = size(); + reserve(size() + 1); + new(elems + lastIdx) T(std::forward(elems[idx])); + setUsedCount(usedCount + 1); + return; + } + + const size_t lastIdx = size(); + reserve(size() + 1); + new(elems + lastIdx) T(std::forward(el)); + setUsedCount(usedCount + 1); + } + + template + void emplace_back(ConstructArgs... args) + { + const size_t lastIdx = size(); + reserve(size() + 1); + new(elems + lastIdx) T(std::forward(args...)); + setUsedCount(usedCount + 1); + } + + // fill the array with 'count' copies of 'el' void fill(size_t count, const T &el) { // destruct any old elements @@ -352,8 +390,8 @@ public: else { // we need to shuffle everything up. Iterate from the back in two stages: first into the - // newly-allocated elements that don't need to be destructed. Then one-by-one destructing an - // element (which has been moved later), and copy-constructing the new one into place + // newly-allocated elements that don't need to be destructed. Then one-by-one + // move-constructing the new one into place // // e.g. an array of 6 elements, inserting 3 more at offset 1 // @@ -363,9 +401,9 @@ public: // // first pass: // - // [8].copyConstruct([5]) - // [7].copyConstruct([4]) - // [6].copyConstruct([3]) + // [8].moveConstruct([5]) + // [7].moveConstruct([4]) + // [6].moveConstruct([3]) // // [0] [1] [2] [3] [4] [5] [6] [7] [8] // A B C D* E* F* D E F @@ -374,9 +412,9 @@ public: // // second pass: // [5].destruct() - // [5].copyConstruct([2]) + // [5].moveConstruct([2]) // [4].destruct() - // [4].copyConstruct([1]) + // [4].moveConstruct([1]) // // [0] [1] [2] [3] [4] [5] [6] [7] [8] // A B* C* D* B C D E F @@ -395,21 +433,24 @@ public: // In the next part we just need to check if the slot was < oldCount to know if we should // destruct it before inserting. - // first pass, copy - size_t copyCount = count < oldSize ? count : oldSize; - for(size_t i = 0; i < copyCount; i++) - new(elems + oldSize + count - 1 - i) T(elems[oldSize - 1 - i]); + // first pass, move construct elements in place + const size_t moveCount = count < oldSize ? count : oldSize; + for(size_t i = 0; i < moveCount; i++) + { + new(elems + oldSize + count - 1 - i) T(std::move(elems[oldSize - 1 - i])); + } - // second pass, destruct & copy if there was any overlap + // second pass, move any overlap. We're moving *into* any elements that got moved *out of* + // above if(count < oldSize - offs) { size_t overlap = oldSize - offs - count; for(size_t i = 0; i < overlap; i++) { // destruct old element - elems[oldSize - 1 - i].~T(); + ItemDestroyHelper::destroyRange(elems + oldSize - 1 - i, 1); // copy from earlier - new(elems + oldSize - 1 - i) T(elems[oldSize - 1 - count - i]); + new(elems + oldSize - 1 - i) T(std::move(elems[oldSize - 1 - count - i])); } } @@ -418,7 +459,7 @@ public: { // if this was one used previously, destruct it if(i < oldSize) - elems[offs + i].~T(); + ItemDestroyHelper::destroyRange(elems + offs + i, 1); // then copy construct the new value new(elems + offs + i) T(el[i]); @@ -447,6 +488,55 @@ public: insert(offs, ©, 1); } } + + // insert by moving + inline void insert(size_t offs, T &&el) + { + const size_t oldSize = size(); + + // invalid size + if(offs > oldSize) + return; + + if(begin() <= &el && &el < end()) + { + // if we're inserting from within our range, save the index + size_t idx = &el - begin(); + // do any potentially reallocating resize + reserve(oldSize + 1); + // then move from the index in wherever elems now points + new(elems + offs) T(std::move(elems[idx])); + return; + } + + // reserve more space if needed + reserve(oldSize + 1); + + // fast path where offs == size(), for push_back + if(offs == oldSize) + { + new(elems + offs) T(std::move(el)); + } + else + { + // we need to shuffle everything up by one + + // first pass, move construct elements and destruct as we go + const size_t moveCount = oldSize - offs; + for(size_t i = 0; i < moveCount; i++) + { + new(elems + oldSize - i) T(std::move(elems[oldSize - i - 1])); + ItemDestroyHelper::destroyRange(elems + oldSize - i - 1, 1); + } + + // then move construct the new value + new(elems + offs) T(std::move(el)); + } + + // update new size + setUsedCount(usedCount + 1); + } + // helpful shortcut for 'append at end', basically a multi-element push_back inline void append(const T *el, size_t count) { insert(size(), el, count); } inline void append(const rdcarray &in) { insert(size(), in.data(), in.size()); } @@ -471,14 +561,13 @@ public: // copy-construct into new place // destruct elements to be removed - for(size_t i = 0; i < count; i++) - elems[offs + i].~T(); + ItemDestroyHelper::destroyRange(elems + offs, count); // move remaining elements into place for(size_t i = offs + count; i < sz; i++) { - new(elems + i - count) T(elems[i]); - elems[i].~T(); + new(elems + i - count) T(std::move(elems[i])); + ItemDestroyHelper::destroyRange(elems + i, 1); } // update new size diff --git a/renderdoc/api/replay/rdcpair.h b/renderdoc/api/replay/rdcpair.h index 90612f897..68eb71f51 100644 --- a/renderdoc/api/replay/rdcpair.h +++ b/renderdoc/api/replay/rdcpair.h @@ -25,6 +25,9 @@ #pragma once +#include +#include + template struct rdcpair { @@ -34,7 +37,11 @@ struct rdcpair rdcpair(const A &a, const B &b) : first(a), second(b) {} rdcpair() = default; rdcpair(const rdcpair &o) = default; - rdcpair(rdcpair &&o) = default; + rdcpair(rdcpair &&o) : first(std::move(o.first)), second(std::move(o.second)) {} + rdcpair(typename std::decay::type &&a, typename std::decay::type &&b) + : first(std::move(a)), second(std::move(b)) + { + } ~rdcpair() = default; inline void swap(rdcpair &o) { @@ -58,6 +65,21 @@ struct rdcpair return *this; } + template + rdcpair &operator=(rdcpair &&o) + { + first = std::move(o.first); + second = std::move(o.second); + return *this; + } + + rdcpair &operator=(rdcpair &&o) + { + first = std::move(o.first); + second = std::move(o.second); + return *this; + } + bool operator==(const rdcpair &o) const { return first == o.first && second == o.second; } bool operator<(const rdcpair &o) const { diff --git a/renderdoc/replay/basic_types_tests.cpp b/renderdoc/replay/basic_types_tests.cpp index f90c66021..94f81509d 100644 --- a/renderdoc/replay/basic_types_tests.cpp +++ b/renderdoc/replay/basic_types_tests.cpp @@ -40,6 +40,9 @@ static volatile int32_t moveConstructor = 0; static volatile int32_t valueConstructor = 0; static volatile int32_t copyConstructor = 0; static volatile int32_t destructor = 0; +static volatile int32_t movedDestructor = 0; +static volatile int32_t copyAssignment = 0; +static volatile int32_t moveAssignment = 0; struct ConstructorCounter { @@ -47,28 +50,49 @@ struct ConstructorCounter ConstructorCounter() { + RDCASSERT(value != -9999); value = 0; Atomic::Inc32(&constructor); } ConstructorCounter(int v) { + RDCASSERT(value != -9999); value = v; Atomic::Inc32(&valueConstructor); } ConstructorCounter(const ConstructorCounter &other) { + RDCASSERT(value != -9999); value = other.value; Atomic::Inc32(©Constructor); } ConstructorCounter(ConstructorCounter &&other) { + RDCASSERT(value != -9999); value = other.value; other.value = -9999; Atomic::Inc32(&moveConstructor); } - ConstructorCounter &operator=(const ConstructorCounter &other) = delete; - ConstructorCounter &operator=(ConstructorCounter &&other) = delete; - ~ConstructorCounter() { Atomic::Inc32(&destructor); } + ConstructorCounter &operator=(const ConstructorCounter &other) + { + value = other.value; + Atomic::Inc32(©Assignment); + return *this; + } + ConstructorCounter &operator=(ConstructorCounter &&other) + { + value = other.value; + other.value = -9999; + Atomic::Inc32(&moveAssignment); + return *this; + } + ~ConstructorCounter() + { + Atomic::Inc32(&destructor); + if(value == -9999) + Atomic::Inc32(&movedDestructor); + value = -1234; + } bool operator==(const ConstructorCounter &other) { return value == other.value; } }; @@ -80,6 +104,7 @@ TEST_CASE("Test array type", "[basictypes]") valueConstructor = 0; copyConstructor = 0; destructor = 0; + movedDestructor = 0; SECTION("Basic test") { @@ -536,6 +561,7 @@ TEST_CASE("Test array type", "[basictypes]") CHECK(constructor == 1); CHECK(copyConstructor == 1); CHECK(valueConstructor == 0); + CHECK(moveConstructor == 0); test.push_back(ConstructorCounter(10)); @@ -545,8 +571,9 @@ TEST_CASE("Test array type", "[basictypes]") CHECK(valueConstructor == 1); // for the temporary going out of scope CHECK(destructor == 2); - // for the temporary being copied into the new element - CHECK(copyConstructor == 2); + // for the temporary being moved into the new element + CHECK(copyConstructor == 1); + CHECK(moveConstructor == 1); // previous value CHECK(constructor == 1); @@ -555,7 +582,8 @@ TEST_CASE("Test array type", "[basictypes]") // single element in test was moved to new backing storage CHECK(destructor == 3); - CHECK(copyConstructor == 3); + CHECK(copyConstructor == 1); + CHECK(moveConstructor == 2); // previous values CHECK(valueConstructor == 1); @@ -569,7 +597,8 @@ TEST_CASE("Test array type", "[basictypes]") // previous values CHECK(valueConstructor == 1); CHECK(destructor == 3); - CHECK(copyConstructor == 3); + CHECK(copyConstructor == 1); + CHECK(moveConstructor == 2); test.clear(); @@ -579,16 +608,15 @@ TEST_CASE("Test array type", "[basictypes]") // previous values CHECK(constructor == 50); CHECK(valueConstructor == 1); - CHECK(copyConstructor == 3); - - // still should have had no moves - CHECK(moveConstructor == 0); + CHECK(copyConstructor == 1); + CHECK(moveConstructor == 2); // reset counters constructor = 0; valueConstructor = 0; copyConstructor = 0; destructor = 0; + moveConstructor = 0; CHECK(constructor == 0); CHECK(moveConstructor == 0); @@ -620,6 +648,10 @@ TEST_CASE("Test array type", "[basictypes]") // check that the new value arrived CHECK(test.back().value == 9); + + // no assignments + CHECK(copyAssignment == 0); + CHECK(moveAssignment == 0); }; SECTION("operations with empty array") @@ -707,32 +739,38 @@ TEST_CASE("Test array type", "[basictypes]") CHECK(test.capacity() == 100); CHECK(test.size() == 6); - // 5 copies and 5 destructs to shift the array contents up, then another copy for inserting tmp + // 5 moves and 5 destructs to shift the array contents up, then a copy for inserting tmp CHECK(constructor == 6); CHECK(valueConstructor == 0); - CHECK(copyConstructor == 5 + 1); + CHECK(moveConstructor == 5); CHECK(destructor == 5); + CHECK(copyConstructor == 1); CHECK(test[0].value == 999); CHECK(test[1].value == 10); + constructor = valueConstructor = copyConstructor = moveConstructor = destructor = 0; + // this should copy the value, then do an insert test.insert(0, test[0]); CHECK(test.capacity() == 100); CHECK(test.size() == 7); - // on top of the above, another 6 copies & destructs to shift the array contents, 1 copy for - // inserting test[0], and a copy&destruct of the temporary copy - CHECK(constructor == 6); + // another 6 moves & destructs to shift the array contents, 2 copies for + // inserting test[0] (once to a temporary with a destructor of that once into the new storage) + CHECK(constructor == 0); CHECK(valueConstructor == 0); - CHECK(copyConstructor == (5 + 1) + 6 + 1 + 1); - CHECK(destructor == (5) + 6 + 1); + CHECK(copyConstructor == 2); + CHECK(moveConstructor == 6); + CHECK(destructor == 6 + 1); CHECK(test[0].value == 999); CHECK(test[1].value == 999); CHECK(test[2].value == 10); + constructor = valueConstructor = copyConstructor = moveConstructor = destructor = 0; + // this should detect the overlapped range, and duplicate the whole object test.insert(0, test.data(), 3); @@ -747,15 +785,15 @@ TEST_CASE("Test array type", "[basictypes]") CHECK(test[4].value == 999); CHECK(test[5].value == 10); - // on top of the above: // - 7 copies and destructs for the duplication (copies into the new storage, destructs from the // old storage) - // - 7 copies and destructs for shifting the array contents + // - 7 moves and destructs for shifting the array contents // - 3 copies for the inserted items - CHECK(constructor == 6); + CHECK(constructor == 0); CHECK(valueConstructor == 0); - CHECK(copyConstructor == (5 + 1 + 6 + 1 + 1) + 7 + 7 + 3); - CHECK(destructor == (5 + 6 + 1) + 7 + 7); + CHECK(copyConstructor == 7 + 3); + CHECK(destructor == 7 + 7); + CHECK(moveConstructor == 7); }; SECTION("Inserting from array's unused memory into itself") @@ -784,6 +822,8 @@ TEST_CASE("Test array type", "[basictypes]") CHECK(copyConstructor == 0); CHECK(destructor == 4); + constructor = destructor = 0; + // this should detect the overlapped range, and duplicate the whole object test.insert(0, test.data() + 2, 3); @@ -791,20 +831,118 @@ TEST_CASE("Test array type", "[basictypes]") CHECK(test.capacity() == 100); CHECK(test.size() == 4); - CHECK(test[0].value == 30); - CHECK(test[1].value == 40); - CHECK(test[2].value == 50); + CHECK(test[0].value == -1234); + CHECK(test[1].value == -1234); + CHECK(test[2].value == -1234); CHECK(test[3].value == 10); // on top of the above: // - 1 copy and destruct for the duplication (copy into the new storage, destruct from // the old storage) - // - 1 copy and destruct for shifting the array contents + // - 1 move and destruct for shifting the array contents // - 3 copies for the inserted items - CHECK(constructor == 5); + CHECK(constructor == 0); CHECK(valueConstructor == 0); - CHECK(copyConstructor == 1 + 1 + 3); - CHECK(destructor == 4 + 1 + 1); + CHECK(copyConstructor == 1 + 3); + CHECK(destructor == 1 + 1); + CHECK(moveConstructor == 1); + }; + + SECTION("Check move constructor is used when possible") + { + rdcarray test; + + // don't test moves due to resizes in this test + test.reserve(100); + + CHECK(constructor == 0); + CHECK(moveConstructor == 0); + CHECK(valueConstructor == 0); + CHECK(copyConstructor == 0); + CHECK(destructor == 0); + + ConstructorCounter tmp; + tmp.value = 9; + + // 1 for the temporary + CHECK(constructor == 1); + + test.push_back(tmp); + + // element should have been copied, not moved + CHECK(copyConstructor == 1); + CHECK(moveConstructor == 0); + CHECK(destructor == 0); + + test.push_back(ConstructorCounter(5)); + + // one more temporary as a value + CHECK(valueConstructor == 1); + // temporary should have been moved, not copied + CHECK(copyConstructor == 1); + CHECK(moveConstructor == 1); + // the temporary should have been destroyed, but after it was moved + CHECK(destructor == 1); + CHECK(movedDestructor == 1); + + test.emplace_back(15); + + // should be constructed in place + CHECK(valueConstructor == 2); + // no other move/copy/destruct + CHECK(copyConstructor == 1); + CHECK(moveConstructor == 1); + CHECK(destructor == 1); + CHECK(movedDestructor == 1); + + test.emplace_back(25); + test.emplace_back(35); + + CHECK(valueConstructor == 4); + CHECK(copyConstructor == 1); + CHECK(moveConstructor == 1); + CHECK(destructor == 1); + CHECK(movedDestructor == 1); + + CHECK(test.size() == 5); + + // insert a new element at 0, without allowing move of the new element + test.insert(0, &tmp, 1); + + CHECK(test.size() == 6); + CHECK(valueConstructor == 4); + // the element will be copied into place + CHECK(copyConstructor == 2); + // all the internal shifts for the resize should have happened via move constructor + CHECK(moveConstructor == 6); + CHECK(destructor == 6); + CHECK(movedDestructor == 6); + CHECK(copyAssignment == 0); + CHECK(moveAssignment == 0); + + // insert an element at 0, allowing it to move + test.insert(4, ConstructorCounter(55)); + + CHECK(test.size() == 7); + CHECK(valueConstructor == 5); + CHECK(copyConstructor == 2); + CHECK(moveConstructor == 9); + CHECK(destructor == 9); + CHECK(movedDestructor == 9); + CHECK(copyAssignment == 0); + CHECK(moveAssignment == 0); + + // erase the element at 3 + test.erase(3); + + CHECK(test.size() == 6); + CHECK(valueConstructor == 5); + CHECK(copyConstructor == 2); + CHECK(moveConstructor == 12); + CHECK(destructor == 13); + CHECK(movedDestructor == 12); + CHECK(copyAssignment == 0); + CHECK(moveAssignment == 0); }; };