AuroraMiddleware/qt5base-lts
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Do not move data around on insert neither

Browse Source 
The while insertion logic will need further work to make it
more efficient. Currently it does use copy construction and
assignment for internal moving instead of move operations.

Change-Id: I7ae3094daa43a44629d8fa89ab6562c2a21b6cbd
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
This commit is contained in:
Lars Knoll 2020-10-30 11:15:19 +01:00
parent 90fadbb365
commit 50ec3252ac
2 changed files with 9 additions and 414 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

216
src/corelib/tools/qarraydataops.h
View File

@ -1055,210 +1055,6 @@ protected:
qsizetype freeSpace(GrowsBackwardsTag) const noexcept { return this->freeSpaceAtBegin(); }
qsizetype freeSpace(GrowsForwardTag) const noexcept { return this->freeSpaceAtEnd(); }
struct RelocatableMoveOps
{
// The necessary evil. Performs move "to the left" when grows backwards and
// move "to the right" when grows forward
template<typename GrowthTag>
static void moveInGrowthDirection(GrowthTag tag, Self *this_, size_t futureGrowth)
{
Q_ASSERT(this_->isMutable());
Q_ASSERT(!this_->isShared());
Q_ASSERT(futureGrowth <= size_t(this_->freeSpace(tag)));
const auto oldBegin = this_->begin();
this_->adjustPointer(tag, futureGrowth);
// Note: move all elements!
::memmove(static_cast<void *>(this_->begin()), static_cast<const void *>(oldBegin),
this_->size * sizeof(T));
}
};
struct GenericMoveOps
{
template <typename ...Args>
static void createInPlace(T *where, Args&&... args)
{
new (where) T(std::forward<Args>(args)...);
}
template <typename ...Args>
static void createInPlace(std::reverse_iterator<iterator> where, Args&&... args)
{
// Note: instead of std::addressof(*where)
createInPlace(where.base() - 1, std::forward<Args>(args)...);
}
// Moves non-pod data range. Handles overlapping regions. By default, expect
// this method to perform move to the _right_. When move to the _left_ is
// needed, use reverse iterators.
template<typename GrowthTag, typename It>
static void moveNonPod(GrowthTag, Self *this_, It where, It begin, It end)
{
Q_ASSERT(begin <= end);
Q_ASSERT(where > begin); // move to the right
using Destructor = typename QArrayExceptionSafetyPrimitives<T>::template Destructor<It>;
auto start = where + std::distance(begin, end);
auto e = end;
Destructor destroyer(start); // Keep track of added items
auto [oldRangeEnd, overlapStart] = std::minmax(where, end);
// step 1. move-initialize elements in uninitialized memory region
while (start != overlapStart) {
--e;
createInPlace(start - 1, std::move_if_noexcept(*e));
// change tracked iterator only after creation succeeded - avoid
// destructing partially constructed objects if exception thrown
--start;
}
// re-created the range. now there is an initialized memory region
// somewhere in the allocated area. if something goes wrong, we must
// clean it up, so "freeze" the position for now (cannot commit yet)
destroyer.freeze();
// step 2. move assign over existing elements in the overlapping
// region (if there's an overlap)
while (e != begin) {
--start;
--e;
*start = std::move_if_noexcept(*e);
}
// step 3. destroy elements in the old range
const qsizetype originalSize = this_->size;
start = begin; // delete elements in reverse order to prevent any gaps
while (start != oldRangeEnd) {
// Exceptions or not, dtor called once per instance
if constexpr (std::is_same_v<std::decay_t<GrowthTag>, GrowsForwardTag>)
++this_->ptr;
--this_->size;
(start++)->~T();
}
destroyer.commit();
// restore old size as we consider data move to be done, the pointer
// still has to be adjusted!
this_->size = originalSize;
}
// Super inefficient function. The necessary evil. Performs move "to
// the left" when grows backwards and move "to the right" when grows
// forward
template<typename GrowthTag>
static void moveInGrowthDirection(GrowthTag tag, Self *this_, size_t futureGrowth)
{
Q_ASSERT(this_->isMutable());
Q_ASSERT(!this_->isShared());
Q_ASSERT(futureGrowth <= size_t(this_->freeSpace(tag)));
if (futureGrowth == 0) // avoid doing anything if there's no need
return;
// Note: move all elements!
if constexpr (std::is_same_v<std::decay_t<GrowthTag>, GrowsBackwardsTag>) {
auto where = this_->begin() - futureGrowth;
// here, magic happens. instead of having move to the right, we'll
// have move to the left by using reverse iterators
moveNonPod(tag, this_,
std::make_reverse_iterator(where + this_->size), // rwhere
std::make_reverse_iterator(this_->end()), // rbegin
std::make_reverse_iterator(this_->begin())); // rend
this_->ptr = where;
} else {
auto where = this_->begin() + futureGrowth;
moveNonPod(tag, this_, where, this_->begin(), this_->end());
this_->ptr = where;
}
}
};
// Moves all elements in a specific direction by moveSize if available
// free space at one of the ends is smaller than required. Free space
// becomes available at the beginning if grows backwards and at the end
// if grows forward
template<typename GrowthTag>
qsizetype prepareFreeSpace(GrowthTag tag, size_t required, size_t moveSize)
{
Q_ASSERT(this->isMutable() || required == 0);
Q_ASSERT(!this->isShared() || required == 0);
Q_ASSERT(required <= size_t(this->constAllocatedCapacity() - this->size));
using MoveOps = std::conditional_t<QTypeInfo<T>::isRelocatable,
RelocatableMoveOps,
GenericMoveOps>;
// if free space at the end is not enough, we need to move the data,
// move is performed in an inverse direction
if (size_t(freeSpace(tag)) < required) {
using MoveTag = typename InverseTag<std::decay_t<GrowthTag>>::tag;
MoveOps::moveInGrowthDirection(MoveTag{}, this, moveSize);
if constexpr (std::is_same_v<MoveTag, GrowsBackwardsTag>) {
return -qsizetype(moveSize); // moving data to the left
} else {
return qsizetype(moveSize); // moving data to the right
}
}
return 0;
}
// Helper wrapper that adjusts passed iterators along with moving the data
// around. The adjustment is necessary when iterators point inside the
// to-be-moved range
template<typename GrowthTag, typename It>
void prepareFreeSpace(GrowthTag tag, size_t required, size_t moveSize, It &b, It &e) {
// Returns whether passed iterators are inside [this->begin(), this->end()]
const auto iteratorsInRange = [&] (const It &first, const It &last) {
using DecayedIt = std::decay_t<It>;
using RemovedConstVolatileIt = std::remove_cv_t<It>;
constexpr bool selfIterator =
// if passed type is an iterator type:
std::is_same_v<DecayedIt, iterator>
|| std::is_same_v<DecayedIt, const_iterator>
// if passed type is a pointer type:
|| std::is_same_v<RemovedConstVolatileIt, T *>
|| std::is_same_v<RemovedConstVolatileIt, const T *>
|| std::is_same_v<RemovedConstVolatileIt, const volatile T *>;
if constexpr (selfIterator) {
return (first >= this->begin() && last <= this->end());
} else {
return false;
}
};
const bool inRange = iteratorsInRange(b, e);
const auto diff = prepareFreeSpace(tag, required, moveSize);
if (inRange) {
std::advance(b, diff);
std::advance(e, diff);
}
}
size_t moveSizeForPrepend(size_t required)
{
// Qt5 QList in prepend: make 33% of all space at front if not enough space
// Now:
qsizetype space = this->allocatedCapacity() / 3;
space = qMax(space, qsizetype(required)); // in case required > 33% of all space
return qMin(space, this->freeSpaceAtEnd());
}
void prepareSpaceForPrepend(size_t required)
{
prepareFreeSpace(GrowsBackwardsTag{}, required, moveSizeForPrepend(required));
}
template<typename It>
void prepareSpaceForPrepend(It &b, It &e, size_t required)
{
prepareFreeSpace(GrowsBackwardsTag{}, required, moveSizeForPrepend(required), b, e);
}
// Tells how much of the given size to insert at the beginning of the
// container. This is insert-specific helper function
qsizetype sizeToInsertAtBegin(const T *const where, qsizetype maxSize)
@ -1406,20 +1202,19 @@ public:
void insert(T *where, const T *b, const T *e)
{
qsizetype n = e - b;
Q_ASSERT(this->isMutable() || (b == e && where == this->end()));
Q_ASSERT(!this->isShared() || (b == e && where == this->end()));
Q_ASSERT(where >= this->begin() && where <= this->end());
Q_ASSERT(b <= e);
Q_ASSERT(e <= where || b > this->end() || where == this->end()); // No overlap or append
Q_ASSERT((e - b) <= this->allocatedCapacity() - this->size);
if (b == e) // short-cut and handling the case b and e == nullptr
if (!n) // short-cut and handling the case b and e == nullptr
return;
if (this->size > 0 && where == this->begin()) { // prepend case - special space arrangement
prepareSpaceForPrepend(b, e, e - b); // ### perf. loss
if (this->size > 0 && where == this->begin() && n < this->freeSpaceAtBegin()) { // prepend case - special space arrangement
Base::insert(GrowsBackwardsTag{}, this->begin(), b, e);
return;
} else if (where == this->end()) { // append case - special space arrangement
} else if (where == this->end() && n < this->freeSpaceAtEnd()) { // append case - special space arrangement
copyAppend(b, e);
return;
}
@ -1440,10 +1235,9 @@ public:
Q_ASSERT(where >= this->begin() && where <= this->end());
Q_ASSERT(this->allocatedCapacity() - this->size >= n);
if (this->size > 0 && where == this->begin()) { // prepend case - special space arrangement
if (this->size > 0 && where == this->begin() && n < this->freeSpaceAtBegin()) { // prepend case - special space arrangement
// Preserve the value, because it might be a reference to some part of the moved chunk
T tmp(t);
prepareSpaceForPrepend(n); // ### perf. loss
Base::insert(GrowsBackwardsTag{}, this->begin(), n, tmp);
return;
} else if (where == this->end() && n <= this->freeSpaceAtEnd()) { // append case - special space arrangement

207
tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp
View File

@ -89,8 +89,6 @@ private slots:
void exceptionSafetyPrimitives_destructor();
void exceptionSafetyPrimitives_mover();
void exceptionSafetyPrimitives_displacer();
void moveNonPod_data();
void moveNonPod();
#endif
};
@ -922,8 +920,10 @@ void tst_QArrayData::arrayOps()
// Insertion at begin (prepend) caused the elements to move, meaning
// that instead of being displaced, newly added elements got constructed
// in uninitialized memory with DefaultConstructed | CopyConstructed
QCOMPARE(int(vo[i].flags), CountedObject::DefaultConstructed
| CountedObject::CopyConstructed);
// ### QArrayData::insert does copy assign some of the values, so this test doesn't
// work
// QCOMPARE(int(vo[i].flags), CountedObject::DefaultConstructed
// | CountedObject::CopyConstructed);
}
for (int i = 5; i < 15; ++i) {
@ -2840,205 +2840,6 @@ void tst_QArrayData::exceptionSafetyPrimitives_displacer()
}
}
}
struct GenericThrowingType
{
std::shared_ptr<int> data = std::shared_ptr<int>(new int(42)); // helper for double free
ThrowingType throwingData = ThrowingType(0);
GenericThrowingType(int id = 0) : throwingData(id)
{
QVERIFY(data.use_count() > 0);
}
~GenericThrowingType()
{
// if we're in dtor but use_count is 0, it's double free
QVERIFY(data.use_count() > 0);
}
enum MoveCase {
MoveRightNoOverlap = 0,
MoveRightOverlap = 1,
MoveLeftNoOverlap = 2,
MoveLeftOverlap = 3,
};
enum ThrowCase {
ThrowInDtor = 0,
ThrowInUninitializedRegion = 1,
ThrowInOverlapRegion = 2,
};
};
struct PublicGenericMoveOps : QtPrivate::QCommonArrayOps<GenericThrowingType>
{
template<typename GrowthTag>
void public_moveInGrowthDirection(GrowthTag tag, qsizetype futureGrowth)
{
static_assert(!QTypeInfo<GenericThrowingType>::isRelocatable);
using MoveOps = QtPrivate::QCommonArrayOps<GenericThrowingType>::GenericMoveOps;
MoveOps::moveInGrowthDirection(tag, this, futureGrowth);
}
};
void tst_QArrayData::moveNonPod_data()
{
QTest::addColumn<GenericThrowingType::MoveCase>("moveCase");
QTest::addColumn<GenericThrowingType::ThrowCase>("throwCase");
// Throwing in dtor
QTest::newRow("throw-in-dtor-move-right-no-overlap")
<< GenericThrowingType::MoveRightNoOverlap << GenericThrowingType::ThrowInDtor;
QTest::newRow("throw-in-dtor-move-right-overlap")
<< GenericThrowingType::MoveRightOverlap << GenericThrowingType::ThrowInDtor;
QTest::newRow("throw-in-dtor-move-left-no-overlap")
<< GenericThrowingType::MoveLeftNoOverlap << GenericThrowingType::ThrowInDtor;
QTest::newRow("throw-in-dtor-move-left-overlap")
<< GenericThrowingType::MoveLeftOverlap << GenericThrowingType::ThrowInDtor;
// Throwing in uninitialized region
QTest::newRow("throw-in-uninit-region-move-right-no-overlap")
<< GenericThrowingType::MoveRightNoOverlap
<< GenericThrowingType::ThrowInUninitializedRegion;
QTest::newRow("throw-in-uninit-region-move-right-overlap")
<< GenericThrowingType::MoveRightOverlap << GenericThrowingType::ThrowInUninitializedRegion;
QTest::newRow("throw-in-uninit-region-move-left-no-overlap")
<< GenericThrowingType::MoveLeftNoOverlap
<< GenericThrowingType::ThrowInUninitializedRegion;
QTest::newRow("throw-in-uninit-region-move-left-overlap")
<< GenericThrowingType::MoveLeftOverlap << GenericThrowingType::ThrowInUninitializedRegion;
// Throwing in overlap region
QTest::newRow("throw-in-overlap-region-move-right-overlap")
<< GenericThrowingType::MoveRightOverlap << GenericThrowingType::ThrowInOverlapRegion;
QTest::newRow("throw-in-overlap-region-move-left-overlap")
<< GenericThrowingType::MoveLeftOverlap << GenericThrowingType::ThrowInOverlapRegion;
}
void tst_QArrayData::moveNonPod()
{
// Assume that non-throwing moves perform correctly. Otherwise, all previous
// tests would've failed. Test only what happens when exceptions are thrown.
QFETCH(GenericThrowingType::MoveCase, moveCase);
QFETCH(GenericThrowingType::ThrowCase, throwCase);
struct WatcherScope
{
WatcherScope() { throwingTypeWatcher().watch = true; }
~WatcherScope()
{
throwingTypeWatcher().watch = false;
throwingTypeWatcher().destroyedIds.clear();
}
};
const auto cast = [] (auto &dataPointer) {
return static_cast<PublicGenericMoveOps*>(std::addressof(dataPointer));
};
const auto setThrowingFlag = [throwCase] () {
switch (throwCase) {
case GenericThrowingType::ThrowInDtor: ThrowingType::throwOnceInDtor = 2; break;
case GenericThrowingType::ThrowInUninitializedRegion: ThrowingType::throwOnce = 2; break;
case GenericThrowingType::ThrowInOverlapRegion: ThrowingType::throwOnce = 3; break;
default: QFAIL("Unknown throwCase");
}
};
const auto checkExceptionText = [throwCase] (const char *what) {
if (throwCase == GenericThrowingType::ThrowInDtor) {
QCOMPARE(std::string(what), ThrowingType::throwStringDtor);
} else {
QCOMPARE(std::string(what), ThrowingType::throwString);
}
};
const auto checkNoMemoryLeaks = [throwCase] (size_t extraDestroyedElements = 0) {
const size_t destroyedElementsCount = throwingTypeWatcher().destroyedIds.size();
switch (throwCase) {
case GenericThrowingType::ThrowInDtor:
// 2 elements from uinitialized region + 2 elements from old range + extra if no overlap
QCOMPARE(destroyedElementsCount, 2u + 2u + extraDestroyedElements);
break;
case GenericThrowingType::ThrowInUninitializedRegion:
// always 1 element from uninitialized region
QCOMPARE(destroyedElementsCount, 1u);
break;
case GenericThrowingType::ThrowInOverlapRegion:
// always 2 elements from uninitialized region
QCOMPARE(destroyedElementsCount, 2u);
break;
default: QFAIL("Unknown throwCase");
}
};
switch (moveCase) {
case GenericThrowingType::MoveRightNoOverlap : { // moving right without overlap
auto storage = createDataPointer<GenericThrowingType>(20, 3);
QVERIFY(storage.freeSpaceAtEnd() > 3);
WatcherScope scope; Q_UNUSED(scope);
try {
setThrowingFlag();
cast(storage)->public_moveInGrowthDirection(QtPrivate::GrowsForwardTag{}, 4);
QFAIL("Unreachable line!");
} catch (const std::runtime_error &e) {
checkExceptionText(e.what());
}
checkNoMemoryLeaks(1);
break;
}
case GenericThrowingType::MoveRightOverlap: { // moving right with overlap
auto storage = createDataPointer<GenericThrowingType>(20, 3);
QVERIFY(storage.freeSpaceAtEnd() > 3);
WatcherScope scope; Q_UNUSED(scope);
try {
setThrowingFlag();
cast(storage)->public_moveInGrowthDirection(QtPrivate::GrowsForwardTag{}, 2);
QFAIL("Unreachable line!");
} catch (const std::runtime_error &e) {
checkExceptionText(e.what());
}
checkNoMemoryLeaks();
break;
}
case GenericThrowingType::MoveLeftNoOverlap: { // moving left without overlap
auto storage = createDataPointer<GenericThrowingType>(20, 2);
storage->insert(storage.begin(), 1, GenericThrowingType(42));
QVERIFY(storage.freeSpaceAtBegin() > 3);
WatcherScope scope; Q_UNUSED(scope);
try {
setThrowingFlag();
cast(storage)->public_moveInGrowthDirection(QtPrivate::GrowsBackwardsTag{}, 4);
QFAIL("Unreachable line!");
} catch (const std::runtime_error &e) {
checkExceptionText(e.what());
}
checkNoMemoryLeaks(1);
break;
}
case GenericThrowingType::MoveLeftOverlap: {
auto storage = createDataPointer<GenericThrowingType>(20, 2);
storage->insert(storage.begin(), 1, GenericThrowingType(42));
QVERIFY(storage.freeSpaceAtBegin() > 3);
WatcherScope scope; Q_UNUSED(scope);
try {
setThrowingFlag();
cast(storage)->public_moveInGrowthDirection(QtPrivate::GrowsBackwardsTag{}, 2);
QFAIL("Unreachable line!");
} catch (const std::runtime_error &e) {
checkExceptionText(e.what());
}
checkNoMemoryLeaks();
break;
}
default: QFAIL("Unknown moveCase");
};
}
#endif // QT_NO_EXCEPTIONS
QTEST_APPLESS_MAIN(tst_QArrayData)