Commit d1f66f33 authored by David Faure's avatar David Faure

Optimize DataStore::setItemsFlags to not fetch item flags again.

Attaching perf to akonadiserver while deleting 10k items
(and possibly some background mailcheck...) showed this in Hotspot:
http://www.davidfaure.fr/2020/hotspot_pimitem_flags.png

Clearly this is calling PimItem::flags() (which makes a SQL query)
twice, once in ItemCreateHandler and again in DataStore::setItemFlags.
Just pass the flags from the former to the latter.
parent 61ae4984
Pipeline #29967 passed with stage
in 46 minutes and 29 seconds
......@@ -62,6 +62,7 @@ bool FakeDataStore::init()
}
bool FakeDataStore::setItemsFlags(const PimItem::List &items,
const QVector<Flag> *currentFlags,
const QVector<Flag> &flags,
bool *flagsChanged,
const Collection &col,
......@@ -72,7 +73,7 @@ bool FakeDataStore::setItemsFlags(const PimItem::List &items,
<< QVariant::fromValue(flags)
<< QVariant::fromValue(col)
<< silent);
return DataStore::setItemsFlags(items, flags, flagsChanged, col, silent);
return DataStore::setItemsFlags(items, currentFlags, flags, flagsChanged, col, silent);
}
bool FakeDataStore::appendItemsFlags(const PimItem::List &items,
......
......@@ -38,6 +38,7 @@ public:
}
bool setItemsFlags(const PimItem::List &items,
const QVector<Flag> *currentFlags,
const QVector<Flag> &flags,
bool *flagsChanged = nullptr,
const Collection &col = Collection(),
......
......@@ -209,14 +209,15 @@ bool ItemCreateHandler::mergeItem(const Protocol::CreateItemCommand &cmd,
// through from Resource during ItemSync, like $ATTACHMENT, because the
// resource is not aware of them (they are usually assigned by client
// upon inspecting the payload)
Q_FOREACH (const Flag &currentFlag, currentItem.flags()) {
const Flag::List currentFlags = currentItem.flags();
for (const Flag &currentFlag : currentFlags) {
const QByteArray currentFlagName = currentFlag.name().toLatin1();
if (localFlagsToPreserve.contains(currentFlagName)) {
flagNames.insert(currentFlagName);
}
}
const auto flags = HandlerHelper::resolveFlags(flagNames);
storageBackend()->setItemsFlags({currentItem}, flags, &flagsChanged, col, true);
storageBackend()->setItemsFlags({currentItem}, &currentFlags, flags, &flagsChanged, col, true);
if (flagsChanged) {
changedParts.insert(AKONADI_PARAM_FLAGS);
needsUpdate = true;
......
......@@ -38,12 +38,13 @@ ItemModifyHandler::ItemModifyHandler(AkonadiServer &akonadi)
: Handler(akonadi)
{}
bool ItemModifyHandler::replaceFlags(const PimItem::List &item, const QSet<QByteArray> &flags, bool &flagsChanged)
bool ItemModifyHandler::replaceFlags(const PimItem::List &items, const QSet<QByteArray> &flags, bool &flagsChanged)
{
Flag::List flagList = HandlerHelper::resolveFlags(flags);
DataStore *store = connection()->storageBackend();
if (!store->setItemsFlags(item, flagList, &flagsChanged)) {
// TODO: why doesn't this have the "Make sure we don't overwrite some local-only flags" code that itemcreatehandler has?
if (!store->setItemsFlags(items, nullptr, flagList, &flagsChanged)) {
qCWarning(AKONADISERVER_LOG) << "ItemModifyHandler::replaceFlags: Unable to replace flags";
return false;
}
......
......@@ -231,7 +231,9 @@ bool DataStore::hasDataStore()
/* --- ItemFlags ----------------------------------------------------- */
bool DataStore::setItemsFlags(const PimItem::List &items, const QVector<Flag> &flags,
bool DataStore::setItemsFlags(const PimItem::List &items,
const QVector<Flag> *currentFlags,
const QVector<Flag> &newFlags,
bool *flagsChanged, const Collection &col_, bool silent)
{
QSet<QString> removedFlags;
......@@ -244,9 +246,9 @@ bool DataStore::setItemsFlags(const PimItem::List &items, const QVector<Flag> &f
setBoolPtr(flagsChanged, false);
for (const PimItem &item : items) {
const Flag::List itemFlags = item.flags();
const Flag::List itemFlags = currentFlags ? *currentFlags : item.flags(); // optimization
for (const Flag &flag : itemFlags) {
if (!flags.contains(flag)) {
if (!newFlags.contains(flag)) {
removedFlags << flag.name();
Query::Condition cond;
cond.addValueCondition(PimItemFlagRelation::leftFullColumnName(), Query::Equals, item.id());
......@@ -255,7 +257,7 @@ bool DataStore::setItemsFlags(const PimItem::List &items, const QVector<Flag> &f
}
}
for (const Flag &flag : flags) {
for (const Flag &flag : newFlags) {
if (!itemFlags.contains(flag)) {
addedFlags << flag.name();
insIds << item.id();
......
......@@ -131,7 +131,8 @@ public:
static bool hasDataStore();
/* --- ItemFlags ----------------------------------------------------- */
virtual bool setItemsFlags(const PimItem::List &items, const QVector<Flag> &flags,
virtual bool setItemsFlags(const PimItem::List &items, const QVector<Flag> *currentFlags,
const QVector<Flag> &newFlags,
bool *flagsChanged = nullptr, const Collection &col = Collection(), bool silent = false);
virtual bool appendItemsFlags(const PimItem::List &items, const QVector<Flag> &flags, bool *flagsChanged = nullptr,
bool checkIfExists = true, const Collection &col = Collection(), bool silent = false);
......
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