Speed up local discovery by not doing it #6087

ckamm · 2017-10-09T12:58:12Z

Use the data from the db instead and only check the file system when a file watcher event suggests it's necessary.

Since we're not confident that file watcher events are good enough, we still do a full local discovery every hour for now. This also doesn't get rid of the initial sync after client startup. But incremental syncs should be sped up significantly.

See #2297

ckamm · 2017-10-09T13:03:12Z

This seemed to work in simple tests but clearly needs a lot more testing. I also broke the unittests - they still need fixing.

ckamm · 2017-10-10T10:54:19Z

The existing unittests work again now. Next steps: Add new unittests that verify it works.

ogoffart · 2017-10-13T09:47:01Z

src/common/syncjournaldb.cpp

@@ -550,7 +550,7 @@ bool SyncJournalDb::checkConnect()
    _getFilesBelowPathQuery.reset(new SqlQuery(_db));
    if (_getFilesBelowPathQuery->prepare(
            GET_FILE_RECORD_QUERY
-            " WHERE path > (?1||'/') AND path < (?1||'0') ORDER BY path||'/' ASC")) {
+            " WHERE ?1 == '' OR (path > (?1||'/') AND path < (?1||'0')) ORDER BY path||'/' ASC")) {


Just wondering: does the query uses the index?

Good catch, adding that term does indeed break index use (checked with EXPLAIN QUERY PLAN)

The problem is that our database doesn't keep paths with a initial /, so path > (''||'/') AND path < (''||'0'') matches nothing. Something like path > '' AND path < 'last-unicode-character' would work, but I think we just need a different query for clarity.

ogoffart · 2017-10-13T09:51:18Z

src/csync/csync_update.cpp

+    auto rowCallback = [ctx, &count, &skipbase, &files](const OCC::SyncJournalFileRecord &rec) {
+        if (ctx->current == REMOTE_REPLICA) {
+            /* When selective sync is used, the database may have subtrees with a parent
+             * whose etag (md5) is _invalid_. These are ignored and shall not appear in the


(unrelated: we could remove the reference to md5 here. It was there before because the _etag was called _md5, but it has gotten renamed)

ogoffart · 2017-10-13T10:08:27Z

src/gui/folder.cpp

+        return;
+    }
+
+    auto relativePath = path.mid(this->path().size());


s/mid/midRef/

ogoffart · 2017-10-13T10:18:30Z

src/gui/folder.cpp

@@ -783,6 +824,21 @@ void Folder::slotSyncFinished(bool success)
        journalDb()->setSelectiveSyncList(SyncJournalDb::SelectiveSyncWhiteList, QStringList());
    }

+    // bug: This function uses many different criteria for "sync was successful" - investigate!
+    if ((_syncResult.status() == SyncResult::Success
+            || _syncResult.status() == SyncResult::Problem)


So it is problem even for soft errors?

ogoffart · 2017-10-13T10:20:31Z

src/gui/folder.cpp

+            // on 100% successful sync. ('Problem' syncs could have the touched
+            // file trigger a soft-error-and-retry scenario and wiping this list
+            // would mean the file isn't retried)
+            _locallyModifiedDirs.clear();


What if there were change hapenning during the sync!

Maybe we should clear it as we start the sync, and if the sync fails for any problem, we add them back.

@ogoffart Yes, great catch! That means we really need to remove files from this list as the individual jobs report success even in the initial version. At least that also solves the problem with 'Problem' syncs due to soft errors at the same time.

ogoffart · 2017-10-13T10:26:23Z

src/gui/folder.h

+     *
+     * Created by registerFolderWatcher(), triggers slotWatchedPathChanged()
+     */
+    FolderWatcher *_folderWatcher = 0;


ohoh! you are putting the FolderWatcher back in the Folder itself :-) cc/ @dragotin

ogoffart · 2017-10-13T10:31:50Z

src/gui/folder.h

+     *
+     * Created by registerFolderWatcher(), triggers slotWatchedPathChanged()
+     */
+    FolderWatcher *_folderWatcher = 0;


Consider using QScopedPointer. Otherwise, nullptr instead of 0.

This makes it unnecessary for FolderMan to manage the list and removes the need for some forwarders. This is done in preparation for follow-up commits that want to add diagnostics to FolderWatcher that shall be available from within Folder.

Add state and signal to catch the following two known problems: * Linux: inotify user watch pool is exhausted. Folder watcher becomes unreliable. * Windows: buffer is too small, some notifications are lost but watching stays reliable.

ckamm · 2017-10-17T10:55:29Z

@ogoffart Significantly updated! The local-discovery-path list now has pretty elaborate sync-error handling.

ogoffart

7d6487e approved.

ogoffart

Looks good, but please see the comments.

ogoffart · 2017-10-17T12:18:15Z

src/common/syncjournaldb.cpp

+    _getAllFilesQuery.reset(new SqlQuery(_db));
+    if (_getAllFilesQuery->prepare(
+            GET_FILE_RECORD_QUERY
+            " ORDER BY path||'/' ASC")) {


Why do we concatenate '/' to the path while ordering?

ogoffart · 2017-10-17T12:25:15Z

src/csync/csync_update.cpp

+      // and we want to check whether we should read from the db. Assume "d/foo a" is
+      // in locally_touched_dirs. Then this check will say no, don't read from the db!
+      // (because "d/foo" < "d/foo a" < "d/foo/bar")
+      auto it = ctx->locally_touched_dirs.lower_bound(QByteArray(local_uri));


The QByteArray allocation could be avoided in C++14, there is an overload of lower_bound that can work for types for which the comparator is transparent.

But that would imply a change to the makefile to enable C++14.
Maybe it is not needed as it should not happen so often anyway.

I'll add a note about this for C++14, but won't enable it in this PR.

ogoffart · 2017-10-17T12:27:00Z

src/gui/folder.cpp

+
+        if (lcFolder().isDebugEnabled()) {
+            QByteArrayList paths;
+            for (auto &path : _localDiscoveryPaths) {


(minor: could be done with std::copy)

ogoffart · 2017-10-17T12:29:42Z

src/gui/folder.cpp

+            for (auto &path : _localDiscoveryPaths) {
+                paths.append(path);
+            }
+            qCDebug(lcFolder) << "local discovery paths: " << qUtf8Printable(paths.join(", "));


can you not just qCDebug << paths? why do we need to convert it to a QByteArray first?
Also , qUtf8Priontable is for QString. When you have a QByteArray you can just use constData().

ogoffart · 2017-10-17T12:40:42Z

src/libsync/configfile.h

@@ -72,6 +72,9 @@ class OWNCLOUDSYNC_EXPORT ConfigFile
    /* Force sync interval, in milliseconds */
    quint64 forceSyncInterval(const QString &connection = QString()) const;

+    /** Interval within which full local discovery is required, -1 to disable */
+    qint64 fullLocalDiscoveryInterval() const;


What's the unit?
I know this is not yet your coding style. But in the future it would be nice to use things like std::chrono::milliseconds as a return type to make it explicit.

ckamm · 2017-10-18T08:27:07Z

@ogoffart Resolved remaining issues

ogoffart

👍

ogoffart · 2017-10-18T08:54:03Z

src/csync/csync_update.cpp

+      // and we want to check whether we should read from the db. Assume "d/foo a" is
+      // in locally_touched_dirs. Then this check will say no, don't read from the db!
+      // (because "d/foo" < "d/foo a" < "d/foo/bar")
+      // C++17: Could skip the conversion to QByteArray here.


We mostly trust the file watchers meaning that we don't re-scan the local tree if we have done that recently and no file watcher events have arrived. If the file watchers invalidate a subtree, we rescan only that subtree. Since we're not entirely sure the file watchers are reliable, we still do full local discoveries regularly (1h by default). There is a config file setting as well as an environment variable to control the interval.

ckamm added this to the 2.5.0 milestone Oct 9, 2017

ckamm self-assigned this Oct 9, 2017

ckamm requested review from jturcotte, ogoffart and guruz October 9, 2017 12:58

ckamm force-pushed the discovery-local branch from 2a8ee00 to 863a00d Compare October 10, 2017 10:53

ckamm force-pushed the discovery-local branch from 863a00d to 428e637 Compare October 10, 2017 12:37

ogoffart reviewed Oct 13, 2017

View reviewed changes

ckamm mentioned this pull request Oct 13, 2017

Excludes: Further optimize also patterns with slash and add trailing slash to some of the items in existing sync-exclude.lst #5017

Closed

ogoffart reviewed Oct 17, 2017

View reviewed changes

ckamm force-pushed the discovery-local branch from 428e637 to bd4f4c9 Compare October 17, 2017 10:44

ckamm added 2 commits October 17, 2017 12:50

Folder: Move FolderWatcher to Folder

7d6487e

This makes it unnecessary for FolderMan to manage the list and removes the need for some forwarders. This is done in preparation for follow-up commits that want to add diagnostics to FolderWatcher that shall be available from within Folder.

FolderWatcher: reliability and data-loss signal

c214b2b

Add state and signal to catch the following two known problems: * Linux: inotify user watch pool is exhausted. Folder watcher becomes unreliable. * Windows: buffer is too small, some notifications are lost but watching stays reliable.

ckamm force-pushed the discovery-local branch from bd4f4c9 to 6563e40 Compare October 17, 2017 10:54

ogoffart reviewed Oct 17, 2017

View reviewed changes

ogoffart approved these changes Oct 17, 2017

View reviewed changes

ckamm force-pushed the discovery-local branch from 6563e40 to 2c69d9b Compare October 18, 2017 08:26

ogoffart reviewed Oct 18, 2017

View reviewed changes

ckamm force-pushed the discovery-local branch from 2c69d9b to 2b3a31a Compare October 18, 2017 08:57

ckamm merged commit e85a339 into master Oct 24, 2017

ckamm deleted the discovery-local branch October 24, 2017 08:00

This was referenced Oct 24, 2017

Local file discovery should be done at most once per run, use file watcher events to seed Discovery #2297

Closed

Local discovery list not wiped for files that need no propagation #6120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up local discovery by not doing it #6087

Speed up local discovery by not doing it #6087

ckamm commented Oct 9, 2017

ckamm commented Oct 9, 2017 •

edited

Loading

ckamm commented Oct 10, 2017

ogoffart Oct 13, 2017

ckamm Oct 17, 2017

ckamm Oct 17, 2017

ogoffart Oct 13, 2017

ogoffart Oct 13, 2017

ogoffart Oct 13, 2017

ogoffart Oct 13, 2017

ckamm Oct 13, 2017 •

edited

Loading

ogoffart Oct 13, 2017

ogoffart Oct 13, 2017

ckamm commented Oct 17, 2017 •

edited

Loading

ogoffart left a comment

ogoffart left a comment

ogoffart Oct 17, 2017

ogoffart Oct 17, 2017

ckamm Oct 18, 2017

ogoffart Oct 17, 2017

ogoffart Oct 17, 2017

ogoffart Oct 17, 2017

ckamm Oct 18, 2017

ckamm commented Oct 18, 2017

ogoffart left a comment

ogoffart Oct 18, 2017

ckamm Oct 18, 2017

Speed up local discovery by not doing it #6087

Speed up local discovery by not doing it #6087

Conversation

ckamm commented Oct 9, 2017

ckamm commented Oct 9, 2017 • edited Loading

ckamm commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckamm Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckamm commented Oct 17, 2017 • edited Loading

ogoffart left a comment

Choose a reason for hiding this comment

ogoffart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckamm commented Oct 18, 2017

ogoffart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckamm commented Oct 9, 2017 •

edited

Loading

ckamm Oct 13, 2017 •

edited

Loading

ckamm commented Oct 17, 2017 •

edited

Loading