Exclude regular expression improvements #6200

ckamm · 2017-11-29T13:06:54Z

A collection of refactor and improvement of the regex-based codepath. Calls to fnmatch are entirely gone.

ogoffart

I only looked at the first commit so far.

👎 Because I don't want to introduce more dependency between the syncengine and the config file. I want to move ConfigFile to gui, because i don't want the tools or test to mess or depend on the user's config file

Also, it is a bit strange that the ExcludeFiles class goes in a csync_xxx file, i'd rather it to be named excludesfiles.{cpp,h} and not have a mix of C-style code and proper C++ in the same file.

ogoffart · 2017-12-04T07:42:36Z

test/csync/csync_tests/check_csync_exclude.cpp

-    assert_int_equal(rc, 0);
-    rc = _csync_exclude_add(&(csync->excludes), "latex/*/*.tex.tmp");
-    assert_int_equal(rc, 0);
+    excludedFiles->addManualExclude("*.💩"); // is this source file utf8 encoded?


ogoffart · 2017-12-04T07:50:33Z

src/libsync/syncengine.cpp

@@ -26,6 +26,7 @@
 #include "propagateremotedelete.h"
 #include "propagatedownload.h"
 #include "common/asserts.h"
+#include "configfile.h"


Can we avoid dependency between the syncengine and the config file?
Ideally all the config should be handled by the app.
Please put that in the Folder instead.

Yes, absolutely.

ogoffart · 2017-12-04T07:58:06Z

src/csync/csync_private.h

+   *
+   * See ExcludedFiles in csync_exclude.
+   */
+  std::function<CSYNC_EXCLUDE_TYPE(const char *path, int filetype)> exclude_traversal_fn;


Why not put a (smart?) pointer to the ExcludeFiles here and call the relevant function, instead of using this hook-like indirection?

I prefer tying ExcludedFiles and csync together through this function because it's more composable/decoupled. Now csync only ever cares about the traversal exclude match function which makes it easy to hook in a different exclude function for testing.

ogoffart · 2017-12-04T07:59:56Z

src/csync/csync_exclude.h

+     */
+    bool reloadExcludeFiles();
+
+#ifdef CSYNC_TEST


add a friend?

(This breaks ODR, and will not work on windows because it mangle the access in the symbol name)

ogoffart · 2017-12-04T08:28:08Z

src/csync/csync_exclude.cpp

+    if (OCC::Utility::fsCasePreserving())
+        patternOptions |= QRegularExpression::CaseInsensitiveOption;
+    _regex.setPatternOptions(patternOptions);
+    _regex.optimize();


Why pass the OptimizeOnFirstUsageOption flag if you optimize now?

That was kept from the initial version. I agree that it can be removed.

ogoffart · 2017-12-04T08:30:01Z

src/csync/csync_exclude.cpp

+    _nonRegexExcludes.clear();
+
+    // Start out with regexes that would match nothing
+    QString exclude_only = "a^";


Maybe this can be done at the end (if exclude_only.isEmpty()) exclude_only = "a^";)

ogoffart · 2017-12-04T08:38:10Z

src/csync/csync_exclude.cpp

+    QString patternFileDirRemove = "a^";
+    QString patternDirKeep = "a^";
+    QString patternDirRemove = "a^";
+    auto regexAppend = [](QString &pattern, QString v) {


const QString &v

ogoffart · 2017-12-04T08:55:26Z

src/csync/csync_exclude.cpp

-        match = CSYNC_FILE_SILENTLY_EXCLUDED;
-        goto out;
+    if (blen == 11) {
+        rc = csync_fnmatch("Desktop.ini", bname, 0);


why not bname == "Desktop.ini" (with Qt::CaseInsensitive for file system that needs it) ?

But also, why should we not put this and all other builtin in the regexp?

Yes, that's definitely worthy of further simplification. Some of the patterns above have similar opportunities. Adding to the regex doesn't work easily since these want to be SILENTLY_EXCLUDED.

ckamm · 2017-12-06T08:55:31Z

@ogoffart Addressed comments (separate commits for easy checking, will be rebased away before merge)

ogoffart · 2017-12-06T10:09:28Z

src/csync/csync_exclude.cpp

@@ -702,7 +702,7 @@ void ExcludedFiles::prepare()
        + "(?:^|/)(?:" + bnameFileDirRemove + "|" + bnameDirRemove + ")(?:$|/)"
        + ")");

-    QRegularExpression::PatternOptions patternOptions = QRegularExpression::OptimizeOnFirstUsageOption;
+    QRegularExpression::PatternOptions patternOptions = QRegularExpression::NoPatternOption;


Actually, i would have kept this flag, but removed the optimize call.
But it does not matter anyway.

Make ExcludedFiles something that is instantiated outside of the CSYNC context and then given to it as a hook. ExcludedFiles still lives in csync_exclude and the internal workings haven't been touched.

Improves full matches by more than an order of magnitude and also improves speed of traversal matches by roughly 20%, judging by the check_csync_exclude performance test.

Neither the documentation at http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_13 nor the fnmatch source code in <glibc>/posix/fnmatch_loop.c points toward { causing special matching behavior.

Now all exclude patterns can be translated to regular expressions.

guruz · 2017-12-12T14:43:39Z

Thanks a lot @ckamm for taking this up again :)
What were your CPU improvements?

ckamm · 2017-12-13T07:42:04Z

@guruz Current benchmarks on my machine.

Numbers are 
  2.3 / 2.4 / master
  in ms per call

traversal
  0.00098 / 0.00047 / 0.00046
  reduced to 47% of original

full
  0.0266 / 0.0251 / 0.0019
  reduced to 7% of original

The traversal number is the primarily important one - it speeds up local discovery for which exclude computations had been a bottleneck. The full value is primarily used by the file watcher so the huge improvement should be easily noticable if one copies thousands of files into a synced folder.

Note that these values are for a high-end linux machine. Speedups were even larger when we profiled windows/osx during the initial investigation.

guruz · 2017-12-13T09:59:45Z

Impressive :-)

FYI @michaelstingl

ckamm added this to the 2.5.0 milestone Nov 29, 2017

ckamm self-assigned this Nov 29, 2017

ckamm requested a review from guruz November 29, 2017 13:06

ogoffart suggested changes Dec 4, 2017

View reviewed changes

ogoffart reviewed Dec 4, 2017

View reviewed changes

ogoffart approved these changes Dec 7, 2017

View reviewed changes

ckamm added 6 commits December 7, 2017 14:31

Excludes: Refactor for pending improvements

197c5a8

Make ExcludedFiles something that is instantiated outside of the CSYNC context and then given to it as a hook. ExcludedFiles still lives in csync_exclude and the internal workings haven't been touched.

Excludes: Optimize Desktop.ini check

ae3580c

Excludes: Introduce dir-only regex matches

cf7346c

Excludes: Translate full-path patterns to regex

54402a2

Improves full matches by more than an order of magnitude and also improves speed of traversal matches by roughly 20%, judging by the check_csync_exclude performance test.

Excludes: fnmatch doesn't treat '{' as special

e08bbc8

Neither the documentation at http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_13 nor the fnmatch source code in <glibc>/posix/fnmatch_loop.c points toward { causing special matching behavior.

Excludes: Remove fnmatch codepath

d1d2765

Now all exclude patterns can be translated to regular expressions.

ckamm force-pushed the exclude-regex-5017 branch from 93da62c to d1d2765 Compare December 7, 2017 13:38

ckamm merged commit 287670c into master Dec 7, 2017

guruz deleted the exclude-regex-5017 branch December 12, 2017 14:42

michaelstingl mentioned this pull request Nov 20, 2018

Document platform-wide known limitations, excluded files owncloud/docs#365

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude regular expression improvements #6200

Exclude regular expression improvements #6200

ckamm commented Nov 29, 2017 •

edited

Loading

ogoffart left a comment

ogoffart Dec 4, 2017

ogoffart Dec 4, 2017

ckamm Dec 6, 2017

ogoffart Dec 4, 2017

ckamm Dec 6, 2017

ogoffart Dec 4, 2017

ckamm Dec 6, 2017

ogoffart Dec 4, 2017

ckamm Dec 6, 2017

ogoffart Dec 4, 2017

ckamm Dec 6, 2017

ogoffart Dec 4, 2017

ogoffart Dec 4, 2017

ckamm Dec 6, 2017

ckamm commented Dec 6, 2017

ogoffart Dec 6, 2017

guruz commented Dec 12, 2017

ckamm commented Dec 13, 2017 •

edited

Loading

guruz commented Dec 13, 2017

Exclude regular expression improvements #6200

Exclude regular expression improvements #6200

Conversation

ckamm commented Nov 29, 2017 • edited Loading

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

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 Dec 6, 2017

Choose a reason for hiding this comment

guruz commented Dec 12, 2017

ckamm commented Dec 13, 2017 • edited Loading

guruz commented Dec 13, 2017

ckamm commented Nov 29, 2017 •

edited

Loading

ckamm commented Dec 13, 2017 •

edited

Loading