Skip to content

Commit

Permalink
Fix QArrayDataOps generic and relocatable emplace()
Browse files Browse the repository at this point in the history
Emplace() implemented with std::rotate is just awful on my system
(Ubuntu 18.04 GCC 7.5.0). Custom code is much faster, so go for
it. Cannot really use insert() code, which is also fast, because
it doesn't forward-reference values but copies them always

Changes in performance (approximately) for emplacing 100k elements
into the middle:
Complex 7600ms -> 1700ms
Movable 7600ms -> 200ms

Task-number: QTBUG-86583
Change-Id: If883c9b8498a89e757f3806aea11f8fd3aa3c709
Reviewed-by: Mårten Nordheim <[email protected]>
Reviewed-by: Lars Knoll <[email protected]>
Reviewed-by: Thiago Macieira <[email protected]>
  • Loading branch information
andrey-golubev committed Nov 9, 2020
1 parent 9ede51d commit 250b69a
Show file tree
Hide file tree
Showing 3 changed files with 333 additions and 27 deletions.
107 changes: 100 additions & 7 deletions src/corelib/tools/qarraydataops.h
Original file line number Diff line number Diff line change
Expand Up @@ -772,10 +772,31 @@ struct QGenericArrayOps
Q_ASSERT(where >= this->begin() && where <= this->end());
Q_ASSERT(this->freeSpaceAtEnd() >= 1);

createInPlace(this->end(), std::forward<Args>(args)...);
++this->size;
if (where == this->end()) {
createInPlace(this->end(), std::forward<Args>(args)...);
++this->size;
} else {
T tmp(std::forward<Args>(args)...);

T *const end = this->end();
T *readIter = end - 1;
T *writeIter = end;

// Create new element at the end
new (writeIter) T(std::move(*readIter));
++this->size;

// Move assign over existing elements
while (readIter != where) {
--readIter;
--writeIter;
*writeIter = std::move(*readIter);
}

std::rotate(where, this->end() - 1, this->end());
// Assign new element
--writeIter;
*writeIter = std::move(tmp);
}
}

template <typename iterator, typename ...Args>
Expand All @@ -785,11 +806,35 @@ struct QGenericArrayOps
Q_ASSERT(where >= this->begin() && where <= this->end());
Q_ASSERT(this->freeSpaceAtBegin() >= 1);

createInPlace(this->begin() - 1, std::forward<Args>(args)...);
--this->ptr;
++this->size;
if (where == this->begin()) {
createInPlace(this->begin() - 1, std::forward<Args>(args)...);
--this->ptr;
++this->size;
} else {
T tmp(std::forward<Args>(args)...);

T *const begin = this->begin();
T *readIter = begin;
T *writeIter = begin - 1;

// Create new element at the beginning
new (writeIter) T(std::move(*readIter));
--this->ptr;
++this->size;

++readIter;
++writeIter;

// Move assign over existing elements
while (readIter != where) {
*writeIter = std::move(*readIter);
++readIter;
++writeIter;
}

std::rotate(this->begin(), this->begin() + 1, where);
// Assign new element
*writeIter = std::move(tmp);
}
}

void erase(T *b, T *e)
Expand Down Expand Up @@ -998,6 +1043,54 @@ struct QMovableArrayOps
// use moving insert
using QGenericArrayOps<T>::insert;

template<typename iterator, typename... Args>
void emplace(iterator where, Args &&... args)
{
emplace(GrowsForwardTag {}, where, std::forward<Args>(args)...);
}

template<typename iterator, typename... Args>
void emplace(GrowsForwardTag, iterator where, Args &&... args)
{
Q_ASSERT(!this->isShared());
Q_ASSERT(where >= this->begin() && where <= this->end());
Q_ASSERT(this->freeSpaceAtEnd() >= 1);

if (where == this->end()) {
this->createInPlace(where, std::forward<Args>(args)...);
} else {
T tmp(std::forward<Args>(args)...);
typedef typename QArrayExceptionSafetyPrimitives<T>::Displacer ReversibleDisplace;
ReversibleDisplace displace(where, this->end(), 1);
this->createInPlace(where, std::move(tmp));
displace.commit();
}
++this->size;
}

template<typename iterator, typename... Args>
void emplace(GrowsBackwardsTag, iterator where, Args &&... args)
{
Q_ASSERT(!this->isShared());
Q_ASSERT(where >= this->begin() && where <= this->end());
Q_ASSERT(this->freeSpaceAtBegin() >= 1);

if (where == this->begin()) {
this->createInPlace(where - 1, std::forward<Args>(args)...);
} else {
T tmp(std::forward<Args>(args)...);
typedef typename QArrayExceptionSafetyPrimitives<T>::Displacer ReversibleDisplace;
ReversibleDisplace displace(this->begin(), where, -1);
this->createInPlace(where - 1, std::move(tmp));
displace.commit();
}
--this->ptr;
++this->size;
}

// use moving emplace
using QGenericArrayOps<T>::emplace;

void erase(T *b, T *e)
{ erase(GrowsForwardTag{}, b, e); }

Expand Down
153 changes: 153 additions & 0 deletions tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ private slots:
void freeSpace();
void dataPointerAllocate_data();
void dataPointerAllocate();
void selfEmplaceBackwards();
void selfEmplaceForward();
#ifndef QT_NO_EXCEPTIONS
void exceptionSafetyPrimitives_constructor();
void exceptionSafetyPrimitives_destructor();
Expand Down Expand Up @@ -2128,6 +2130,157 @@ void tst_QArrayData::dataPointerAllocate()
}
}

struct MyQStringWrapper : public QString
{
bool movedTo = false;
bool movedFrom = false;
MyQStringWrapper() = default;
MyQStringWrapper(QChar c) : QString(c) { }
MyQStringWrapper(MyQStringWrapper &&other) : QString(std::move(static_cast<QString>(other)))
{
movedTo = true;
movedFrom = other.movedFrom;
other.movedFrom = true;
}
MyQStringWrapper &operator=(MyQStringWrapper &&other)
{
QString::operator=(std::move(static_cast<QString>(other)));
movedTo = true;
movedFrom = other.movedFrom;
other.movedFrom = true;
return *this;
}
MyQStringWrapper(const MyQStringWrapper &) = default;
MyQStringWrapper &operator=(const MyQStringWrapper &) = default;
~MyQStringWrapper() = default;
};

struct MyMovableQString : public MyQStringWrapper
{
MyMovableQString() = default;
MyMovableQString(QChar c) : MyQStringWrapper(c) { }

private:
friend bool operator==(const MyMovableQString &a, QChar c)
{
return static_cast<QString>(a) == QString(c);
}

friend bool operator==(const MyMovableQString &a, const MyMovableQString &b)
{
return static_cast<QString>(a) == static_cast<QString>(b);
}
};

QT_BEGIN_NAMESPACE
Q_DECLARE_TYPEINFO(MyMovableQString, Q_RELOCATABLE_TYPE);
QT_END_NAMESPACE
static_assert(QTypeInfo<MyMovableQString>::isComplex);
static_assert(QTypeInfo<MyMovableQString>::isRelocatable);

struct MyComplexQString : public MyQStringWrapper
{
MyComplexQString() = default;
MyComplexQString(QChar c) : MyQStringWrapper(c) { }

private:
friend bool operator==(const MyComplexQString &a, QChar c)
{
return static_cast<QString>(a) == QString(c);
}

friend bool operator==(const MyComplexQString &a, const MyComplexQString &b)
{
return static_cast<QString>(a) == static_cast<QString>(b);
}
};
static_assert(QTypeInfo<MyComplexQString>::isComplex);
static_assert(!QTypeInfo<MyComplexQString>::isRelocatable);

void tst_QArrayData::selfEmplaceBackwards()
{
const auto createDataPointer = [](qsizetype capacity, int spaceAtEnd, auto dummy) {
using Type = std::decay_t<decltype(dummy)>;
Q_UNUSED(dummy);
auto [header, ptr] = QTypedArrayData<Type>::allocate(capacity, QArrayData::Grow);
// do custom adjustments to make sure there's free space at end
ptr += header->alloc - spaceAtEnd;
return QArrayDataPointer(header, ptr);
};

const auto testSelfEmplace = [&](auto dummy, int spaceAtEnd, auto initValues) {
auto adp = createDataPointer(100, spaceAtEnd, dummy);
for (auto v : initValues) {
adp->emplaceBack(v);
}
QVERIFY(!adp.freeSpaceAtEnd());
QVERIFY(adp.freeSpaceAtBegin());

adp->emplace(adp.end(), adp.data()[0]);
for (qsizetype i = 0; i < adp.size - 1; ++i) {
QCOMPARE(adp.data()[i], initValues[i]);
}
QCOMPARE(adp.data()[adp.size - 1], initValues[0]);

adp->emplace(adp.end(), std::move(adp.data()[0]));
for (qsizetype i = 1; i < adp.size - 2; ++i) {
QCOMPARE(adp.data()[i], initValues[i]);
}
QCOMPARE(adp.data()[adp.size - 2], initValues[0]);
QCOMPARE(adp.data()[0].movedFrom, true);
QCOMPARE(adp.data()[adp.size - 1], initValues[0]);
QCOMPARE(adp.data()[adp.size - 1].movedTo, true);
};

QList<QChar> movableObjs { u'a', u'b', u'c', u'd' };
RUN_TEST_FUNC(testSelfEmplace, MyMovableQString(), 4, movableObjs);
QList<QChar> complexObjs { u'a', u'b', u'c', u'd' };
RUN_TEST_FUNC(testSelfEmplace, MyComplexQString(), 4, complexObjs);
}

void tst_QArrayData::selfEmplaceForward()
{
const auto createDataPointer = [](qsizetype capacity, int spaceAtBegin, auto dummy) {
using Type = std::decay_t<decltype(dummy)>;
Q_UNUSED(dummy);
auto [header, ptr] = QTypedArrayData<Type>::allocate(capacity, QArrayData::Grow);
// do custom adjustments to make sure there's free space at end
ptr += spaceAtBegin;
return QArrayDataPointer(header, ptr);
};

const auto testSelfEmplace = [&](auto dummy, int spaceAtBegin, auto initValues) {
auto adp = createDataPointer(100, spaceAtBegin, dummy);
auto reversedInitValues = initValues;
std::reverse(reversedInitValues.begin(), reversedInitValues.end());
for (auto v : reversedInitValues) {
adp->emplaceFront(v);
}
QVERIFY(!adp.freeSpaceAtBegin());
QVERIFY(adp.freeSpaceAtEnd());

adp->emplace(adp.begin(), adp.data()[adp.size - 1]);
for (qsizetype i = 1; i < adp.size; ++i) {
QCOMPARE(adp.data()[i], initValues[i - 1]);
}
QCOMPARE(adp.data()[0], initValues[spaceAtBegin - 1]);

adp->emplace(adp.begin(), std::move(adp.data()[adp.size - 1]));
for (qsizetype i = 2; i < adp.size; ++i) {
QCOMPARE(adp.data()[i], initValues[i - 2]);
}
QCOMPARE(adp.data()[1], initValues[spaceAtBegin - 1]);
QCOMPARE(adp.data()[adp.size - 1].movedFrom, true);
QCOMPARE(adp.data()[0], initValues[spaceAtBegin - 1]);
QCOMPARE(adp.data()[0].movedTo, true);
};

QList<QChar> movableObjs { u'a', u'b', u'c', u'd' };
RUN_TEST_FUNC(testSelfEmplace, MyMovableQString(), 4, movableObjs);
QList<QChar> complexObjs { u'a', u'b', u'c', u'd' };
RUN_TEST_FUNC(testSelfEmplace, MyComplexQString(), 4, complexObjs);
}

#ifndef QT_NO_EXCEPTIONS
struct ThrowingTypeWatcher
{
Expand Down
Loading

0 comments on commit 250b69a

Please sign in to comment.