Commit 033667a6 authored by David Faure's avatar David Faure

ItemSync: speed up by not using takeFirst().

Summary:
perf+hotspot shows that this is a horrible algorithm:
http://www.davidfaure.fr/kde/perf_hotspot_itemsync.png

I ported the code to the std::move (the algorithm)

Test Plan:
Added a benchmark to itemsynctest.

Before:
     RESULT : ItemsyncTest::testFullSyncManyItems():
         368,935,368.19 CPU cycles per iteration (total: 36,893,536,820, iterations: 100)

After:
     RESULT : ItemsyncTest::testFullSyncManyItems():
         349,899,659.49 CPU cycles per iteration (total: 34,989,965,949, iterations: 100)

    => 5% faster (for the whole of ItemSync, which runs much more than this piece of code)

Went down to 346 million after https://commits.kde.org/kcoreaddons/42e1d6e8e7bf7f2738fbe15778b23a84c104e8a8

(Qt + Akonadi built in release-with-debug; KF5 in debug, but not much KF5 code being run here)

Reviewers: dvratil

Reviewed By: dvratil

Subscribers: mwolff, #kde_pim

Tags: #kde_pim

Differential Revision: https://phabricator.kde.org/D8465
parent 22c1d33e
......@@ -24,6 +24,7 @@
#include <control.h>
#include <collection.h>
#include <item.h>
#include <itemdeletejob.h>
#include <itemfetchjob.h>
#include <itemfetchscope.h>
#include <itemsync.h>
......@@ -49,7 +50,7 @@ class ItemsyncTest : public QObject
private:
Item::List fetchItems(const Collection &col)
{
qDebug() << col.remoteId();
qDebug() << "fetching items from collection" << col.remoteId() << col.name();
ItemFetchJob *fetch = new ItemFetchJob(col, this);
fetch->fetchScope().fetchFullPayload();
fetch->fetchScope().fetchAllAttributes();
......@@ -575,6 +576,62 @@ private Q_SLOTS:
syncer->deliveryDone();
QTRY_COMPARE(spy.count(), 1);
}
void testFullSyncManyItems()
{
const Collection col = Collection(collectionIdFromPath(QStringLiteral("res2/foo2")));
QVERIFY(col.isValid());
Akonadi::Monitor monitor;
monitor.setCollectionMonitored(col);
QSignalSpy addedSpy(&monitor, SIGNAL(itemAdded(Akonadi::Item,Akonadi::Collection)));
QVERIFY(addedSpy.isValid());
const int itemCount = 1000;
for (int i = 0; i < itemCount; ++i) {
Item item(QStringLiteral("application/octet-stream"));
item.setRemoteId(QStringLiteral("rid") + QString::number(i));
item.setGid(QStringLiteral("gid") + QString::number(i));
item.setPayload<QByteArray>("payload1");
ItemCreateJob *job = new ItemCreateJob(item, col);
AKVERIFYEXEC(job);
}
QTRY_COMPARE(addedSpy.count(), itemCount);
addedSpy.clear();
const Item::List origItems = fetchItems(col);
//Since the item sync affects the knut resource we ensure we actually managed to load all items
//This needs to be adjusted should the testdataset change
QCOMPARE(origItems.size(), itemCount);
QSignalSpy deletedSpy(&monitor, SIGNAL(itemRemoved(Akonadi::Item)));
QVERIFY(deletedSpy.isValid());
QSignalSpy changedSpy(&monitor, SIGNAL(itemChanged(Akonadi::Item,QSet<QByteArray>)));
QVERIFY(changedSpy.isValid());
QBENCHMARK {
ItemSync *syncer = new ItemSync(col);
syncer->setTransactionMode(ItemSync::SingleTransaction);
QSignalSpy transactionSpy(syncer, SIGNAL(transactionCommitted()));
QVERIFY(transactionSpy.isValid());
syncer->setFullSyncItems(origItems);
AKVERIFYEXEC(syncer);
QCOMPARE(transactionSpy.count(), 1);
}
const Item::List resultItems = fetchItems(col);
QCOMPARE(resultItems.count(), origItems.count());
QTest::qWait(100);
QCOMPARE(deletedSpy.count(), 0);
QCOMPARE(addedSpy.count(), 0);
QCOMPARE(changedSpy.count(), 0);
// delete all items; QBENCHMARK leads to the whole method being called more than once
ItemDeleteJob *job = new ItemDeleteJob(resultItems);
AKVERIFYEXEC(job);
}
};
QTEST_AKONADIMAIN(ItemsyncTest)
......
......@@ -344,9 +344,10 @@ void ItemSyncPrivate::execute()
if (mRemoteItemQueue.size() >= mBatchSize || mDeliveryDone) {
//we have a new batch to process
const int num = qMin(mBatchSize, mRemoteItemQueue.size());
for (int i = 0; i < num; i++) {
mCurrentBatchRemoteItems << mRemoteItemQueue.takeFirst();
}
mCurrentBatchRemoteItems.reserve(mBatchSize);
std::move(mRemoteItemQueue.begin(), mRemoteItemQueue.begin() + num, std::back_inserter(mCurrentBatchRemoteItems));
mRemoteItemQueue.erase(mRemoteItemQueue.begin(), mRemoteItemQueue.begin() + num);
mCurrentBatchRemovedRemoteItems += mRemovedRemoteItemQueue;
mRemovedRemoteItemQueue.clear();
} else {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment