Skip to content

perf: batch E2EE queries, batch dir rename, hash lookup, larger Windows watcher buffer #9993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
qoole wants to merge 1 commit into
base: master
Choose a base branch
from
+156 −52
Open

perf: batch E2EE queries, batch dir rename, hash lookup, larger Windows watcher buffer #9993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 112 additions & 14 deletions src/common/syncjournaldb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1147,23 +1147,71 @@ bool SyncJournalDb::getRootE2eFolderRecord(const QString &remoteFolderPath, Sync
return false;
}

auto remoteFolderPathSplit = remoteFolderPath.split(QLatin1Char('/'), Qt::SkipEmptyParts);
const auto remoteFolderPathSplit = remoteFolderPath.split(QLatin1Char('/'), Qt::SkipEmptyParts);

if (remoteFolderPathSplit.isEmpty()) {
qCWarning(lcDb) << "Invalid folder path!";
return false;
}

while (!remoteFolderPathSplit.isEmpty()) {
const auto result = getFileRecord(remoteFolderPathSplit.join(QLatin1Char('/')), rec);
if (!result) {
// Build all ancestor paths and their phashes upfront (deepest to shallowest)
QVector<QPair<QByteArray, qint64>> ancestors;
{
QStringList components = remoteFolderPathSplit;
while (!components.isEmpty()) {
const auto path = components.join(QLatin1Char('/')).toUtf8();
ancestors.append({path, getPHash(path)});
components.removeLast();
}
}

QMutexLocker locker(&_mutex);
if (_metadataTableIsEmpty)
return true;
if (!checkConnect())
return false;

// Single IN query for all ancestors instead of N separate queries
QByteArray sql = QByteArrayLiteral(GET_FILE_RECORD_QUERY " WHERE phash IN (");
for (int i = 0; i < ancestors.size(); ++i) {
if (i > 0) sql += ',';
sql += '?' + QByteArray::number(i + 1);
}
sql += ')';

SqlQuery query(_db);
query.prepare(sql);
for (int i = 0; i < ancestors.size(); ++i) {
query.bindValue(i + 1, ancestors[i].second);
}

if (!query.exec()) {
qCWarning(lcDb) << "database error:" << query.error();
return false;
}

// Collect results keyed by path
QHash<QByteArray, SyncJournalFileRecord> results;
forever {
auto next = query.next();
if (!next.ok) {
qCWarning(lcDb) << "database error:" << query.error();
return false;
}
if (rec->isE2eEncrypted() && rec->_e2eMangledName.isEmpty()) {
// it's a toplevel folder record
if (!next.hasData)
break;
SyncJournalFileRecord r;
fillFileRecordFromGetQuery(r, query);
results.insert(r._path, r);
}

// Walk from deepest to shallowest (same order as original)
for (const auto &ancestor : ancestors) {
const auto it = results.constFind(ancestor.first);
if (it != results.constEnd() && it->isE2eEncrypted() && it->_e2eMangledName.isEmpty()) {
*rec = *it;
return true;
}
remoteFolderPathSplit.removeLast();
}

return true;
Expand Down Expand Up @@ -1226,19 +1274,69 @@ bool SyncJournalDb::findEncryptedAncestorForRecord(const QString &filename, Sync
const auto slashPosition = filename.lastIndexOf(QLatin1Char('/'));
const auto parentPath = slashPosition >= 0 ? filename.left(slashPosition) : QString();

auto pathComponents = parentPath.split(QLatin1Char('/'));
while (!pathComponents.isEmpty()) {
const auto pathCompontentsJointed = pathComponents.join(QLatin1Char('/'));
if (!getFileRecord(pathCompontentsJointed, rec)) {
qCWarning(lcDb) << "could not get file from local DB" << pathCompontentsJointed;
if (parentPath.isEmpty())
return true;

// Build all ancestor paths and their phashes upfront (deepest to shallowest)
QVector<QPair<QByteArray, qint64>> ancestors;
{
auto pathComponents = parentPath.split(QLatin1Char('/'));
while (!pathComponents.isEmpty()) {
const auto path = pathComponents.join(QLatin1Char('/')).toUtf8();
ancestors.append({path, getPHash(path)});
pathComponents.removeLast();
}
}

QMutexLocker locker(&_mutex);
if (_metadataTableIsEmpty)
return true;
if (!checkConnect())
return false;

// Single IN query for all ancestors instead of N separate queries
QByteArray sql = QByteArrayLiteral(GET_FILE_RECORD_QUERY " WHERE phash IN (");
for (int i = 0; i < ancestors.size(); ++i) {
if (i > 0) sql += ',';
sql += '?' + QByteArray::number(i + 1);
}
sql += ')';

SqlQuery query(_db);
query.prepare(sql);
for (int i = 0; i < ancestors.size(); ++i) {
query.bindValue(i + 1, ancestors[i].second);
}

if (!query.exec()) {
qCWarning(lcDb) << "database error:" << query.error();
return false;
}

// Collect results keyed by path
QHash<QByteArray, SyncJournalFileRecord> results;
forever {
auto next = query.next();
if (!next.ok) {
qCWarning(lcDb) << "database error:" << query.error();
return false;
}
if (!next.hasData)
break;
SyncJournalFileRecord r;
fillFileRecordFromGetQuery(r, query);
results.insert(r._path, r);
}

if (rec->isValid() && rec->isE2eEncrypted()) {
// Walk from deepest to shallowest (same order as original)
for (const auto &ancestor : ancestors) {
const auto it = results.constFind(ancestor.first);
if (it != results.constEnd() && it->isValid() && it->isE2eEncrypted()) {
*rec = *it;
break;
}
pathComponents.removeLast();
}

return true;
}

Expand Down
4 changes: 2 additions & 2 deletions src/gui/folderwatcher_win.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,8 @@ void WatcherThread::run()

// If this buffer fills up before we've extracted its data we will lose
// change information. Therefore start big.
size_t bufferSize = 4096 * 10;
const size_t maxBuffer = 64 * 1024;
size_t bufferSize = 256 * 1024;
const size_t maxBuffer = 2 * 1024 * 1024;

while (!_done) {
bool increaseBufferSize = false;
Expand Down
4 changes: 0 additions & 4 deletions src/gui/tray/CurrentAccountHeaderButton.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,6 @@ Button {
closePolicy: Menu.CloseOnPressOutsideParent | Menu.CloseOnEscape

onClosed: {
// HACK: reload account Instantiator immediately by resetting it - could be done better I guess
// see also onVisibleChanged above
userLineInstantiator.active = false;
userLineInstantiator.active = true;
}

Instantiator {
Expand Down
4 changes: 0 additions & 4 deletions src/gui/tray/MainWindow.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@ ApplicationWindow {
onClosing: Systray.isOpen = false

onVisibleChanged: {
// HACK: reload account Instantiator immediately by restting it - could be done better I guess
// see also id:trayWindowHeader.currentAccountHeaderButton.accountMenu below
trayWindowHeader.currentAccountHeaderButton.userLineInstantiator.active = false;
trayWindowHeader.currentAccountHeaderButton.userLineInstantiator.active = true;
syncStatus.model.load();
}

Expand Down
19 changes: 11 additions & 8 deletions src/libsync/owncloudpropagator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,12 @@ void OwncloudPropagator::adjustDeletedFoldersWithNewChildren(SyncFileItemVector
instead of CSYNC_INSTRUCTION_REMOVE
NOTE: We are iterating backwards to take advantage of optimization later, when searching for the parent of current it
*/
// Build index for O(1) lookup by file name (replaces O(n) std::find_if per item)
QHash<QString, int> fileNameToIndex;
for (int i = 0; i < static_cast<int>(items.size()); ++i) {
fileNameToIndex.insert(items[i]->_file, i);
}

for (auto it = std::crbegin(items); it != std::crend(items); ++it) {
if ((*it)->_instruction != CSYNC_INSTRUCTION_NEW || (*it)->_direction != SyncFileItem::Up || !(*it)->isDirectory() || (*it)->_file == QStringLiteral("/")) {
continue;
Expand All @@ -455,17 +461,14 @@ void OwncloudPropagator::adjustDeletedFoldersWithNewChildren(SyncFileItemVector
if (itemRootFolderName.isEmpty()) {
continue;
}
// #2 iterate backwards (for optimization) and find the root folder by name
const auto itemRootFolderReverseIt = std::find_if(it, std::crend(items), [&itemRootFolderName](const auto &currentItem) {
return currentItem->_file == itemRootFolderName;
});

if (itemRootFolderReverseIt == std::rend(items)) {
// #2 O(1) lookup for the root folder by name
const auto indexIt = fileNameToIndex.constFind(itemRootFolderName);
if (indexIt == fileNameToIndex.constEnd()) {
continue;
}

// #3 convert reverse iterator to normal iterator
const auto itemFolderIt = (itemRootFolderReverseIt + 1).base();
// #3 get forward iterator directly from index
const auto itemFolderIt = items.begin() + indexIt.value();

// #4 if the root folder is set to be removed, then we will need to fix this by reuploading every folder in
// the tree, including the root
Expand Down
51 changes: 31 additions & 20 deletions src/libsync/propagatorjobs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,35 +520,46 @@ void PropagateLocalRename::start()
}
} else if (!fileAlreadyMoved) {
qCDebug(lcPropagateLocalRename) << "propagate child items after move from" << existingFile << "to" << targetFile;
const auto dbQueryResult = propagator()->_journal->getFilesBelowPath(previousNameInDb.toUtf8(), [previousNameInDb, this] (const SyncJournalFileRecord &record) -> void {
const auto oldFileNameString = propagator()->adjustRenamedPath(QString::fromUtf8(record._path));
auto newFileNameString = oldFileNameString;
newFileNameString.replace(0, previousNameInDb.length(), _item->_renameTarget);
// Collect all child records first, then batch the DB mutations
// (avoids interleaving reads/writes while the streaming query cursor is open)
struct RenameEntry {
QString oldName;
QString newName;
SyncJournalFileRecord record;
};
QVector<RenameEntry> renameEntries;

qCDebug(lcPropagateLocalRename) << "child rename from" << oldFileNameString << "to" << newFileNameString;
const auto dbQueryResult = propagator()->_journal->getFilesBelowPath(previousNameInDb.toUtf8(), [&renameEntries, &previousNameInDb, this] (const SyncJournalFileRecord &record) -> void {
RenameEntry entry;
entry.oldName = propagator()->adjustRenamedPath(QString::fromUtf8(record._path));
entry.newName = entry.oldName;
entry.newName.replace(0, previousNameInDb.length(), _item->_renameTarget);
entry.record = record;

if (oldFileNameString == newFileNameString) {
Q_ASSERT(false);
return;
if (entry.oldName != entry.newName) {
renameEntries.append(std::move(entry));
}
});

propagator()->_journal->commit("localRename pre-batch", true);
// Process collected entries in batch
for (const auto &entry : renameEntries) {
qCDebug(lcPropagateLocalRename) << "child rename from" << entry.oldName << "to" << entry.newName;

SyncJournalFileRecord oldRecord;
if (!propagator()->_journal->getFileRecord(oldFileNameString, &oldRecord)) {
qCWarning(lcPropagateLocalRename) << "Could not get file from local DB" << oldFileNameString;
return;
if (!propagator()->_journal->getFileRecord(entry.oldName, &oldRecord)) {
qCWarning(lcPropagateLocalRename) << "Could not get file from local DB" << entry.oldName;
continue;
}
if (!propagator()->_journal->deleteFileRecord(oldFileNameString)) {
qCWarning(lcPropagateLocalRename) << "could not delete file from local DB" << oldFileNameString;
return;
if (!propagator()->_journal->deleteFileRecord(entry.oldName)) {
qCWarning(lcPropagateLocalRename) << "could not delete file from local DB" << entry.oldName;
continue;
}

const auto newItem = SyncFileItem::fromSyncJournalFileRecord(oldRecord);
newItem->_file = newFileNameString;
const auto result = propagator()->updateMetadata(*newItem);
if (!result) {
return;
}
});
newItem->_file = entry.newName;
propagator()->updateMetadata(*newItem);
}
if (!dbQueryResult) {
done(SyncFileItem::FatalError, tr("Failed to propagate directory rename in hierarchy"), OCC::ErrorCategory::GenericError);
return;
Expand Down