Excludes: During directory traversal, use QRegularExpression #6063

guruz · 2017-09-27T14:08:32Z

On my Mac, this halves the time spent in csync_excluded_traversal
when using check_csync_excluded_performance.
On Linux the changes were not so much visible, maybe the fnmatch
there is much faster.

guruz · 2017-09-27T14:10:08Z

Done with a lot of help from @ckamm !
Kudos..

ogoffart · 2017-09-28T10:21:13Z

src/csync/csync_exclude.cpp

+    QString pattern = "^(" + exclude_only + ")$|^(" + exclude_and_remove + ")$";
+    ctx->parsed_traversal_excludes.regexp_exclude.setPattern(pattern);
+    ctx->parsed_traversal_excludes.regexp_exclude.setPatternOptions(QRegularExpression::OptimizeOnFirstUsageOption
+#if defined(Q_OS_WIN) || defined(Q_OS_MAC)


should be using the check for case sensitivity in utility

ogoffart · 2017-09-28T10:23:19Z

src/csync/csync_exclude.cpp

-  }
-  return false;
+
+    return false;
 }

 static CSYNC_EXCLUDE_TYPE _csync_excluded_common(c_strlist_t *excludes, const char *path, int filetype, bool check_leading_dirs) {


My benchmark test shows that this is still significant when there are no exclude. Can they not be put in the regexp?

I don't understand what you mean? That excluded_common still shows up in benchmarks even when there are no excludes?

yes exactly. So this part is still slow and perhaps could be improved if merged within the same regexp.
(possibly in another capture?)

Altough i did not test the new patch which check that the file is not starting with . so maybe it is ok.

ogoffart · 2017-09-28T10:25:26Z

src/csync/csync_exclude.cpp

+
+    if (len_filename == 3 || (len_filename > 3 && filename[3] == '.')) {
+        size_t n_words = sizeof(win_reserved_words_3) / sizeof(char *);
+        for (size_t j = 0; j < n_words; j++) {


could be written:

for (const char *word : win_reserved_words_3)

(less error prone)

ogoffart · 2017-09-28T10:25:49Z

src/csync/csync_exclude.cpp

+
+    if (len_filename == 4 || (len_filename > 4 && filename[4] == '.')) {
+        size_t n_words = sizeof(win_reserved_words_4) / sizeof(char *);
+        for (size_t j = 0; j < n_words; j++) {


(same here)

ogoffart · 2017-09-28T10:26:08Z

src/csync/csync_exclude.cpp

+    }
+
+    size_t n_words = sizeof(win_reserved_words_n) / sizeof(char *);
+    for (size_t j = 0; j < n_words; j++) {


ogoffart · 2017-09-28T10:28:02Z

src/csync/csync_exclude.cpp

+    return s;
+}
+
+void csync_exclude_traversal_prepare(CSYNC *ctx)


wondering if this should not be a member function of the parsed_traversal_excludes class

ogoffart · 2017-09-28T10:29:45Z

src/csync/csync_exclude.cpp

+    } else {
+        bname = path;
+    }
+    QString p = QString::fromUtf8(bname);


Please move under the if

ckamm · 2017-09-28T10:49:40Z

@ogoffart updated

ogoffart · 2017-09-28T16:17:57Z

From jenkins:

CMakeFiles/owncloudsync.dir/objects.a(discoveryphase.cpp.obj):discoveryphase.cpp:(.text+0x573d): undefined reference to `csync_exclude_traversal_prepare(csync_s*)'

guruz · 2017-09-29T12:33:25Z

@ogoffart Jenkins passing, review needed:)

ogoffart · 2017-10-02T09:08:23Z

src/csync/csync_private.h

+      void prepare(c_strlist_t *excludes);
+
+      QRegularExpression regexp_exclude;
+      c_strlist_t *list_patterns_with_slashes = nullptr;


Is this not leaking? Where is it destroyed?

ogoffart

-1 because of the leak.

ogoffart · 2017-10-02T09:15:52Z

src/csync/csync_private.h

+      QRegularExpression regexp_exclude;
+      c_strlist_t *list_patterns_with_slashes = nullptr;
+      /* FIXME ^^ at a later point use QRegularExpression too if those become popular */
+  } parsed_traversal_excludes;


Maybe this should be a pointer to the ExcludedFiles object instead, and everything should be put in the ExcludeFiles class. This would avoid the leak. Curently , the ~ExcludeFiles take care about destroying the excludes pointer.
Also now i guess we could use QVector instead od c_strlist_t.

ogoffart · 2017-10-02T09:21:32Z

Also it seems that this does not optimize csync_excluded_no_ctx which is called by ExcludedFiles::isExcluded, this is important as well at startup as it is called for every directory while initiating the file system watcher... (see also #5245 )

guruz · 2017-10-02T12:35:11Z

@ogoffart Pushed a fix for the leak. Looks good?

PS: I agree with optimizing the _no_ctx too. I don't agree with having the list in Excludes object, it feels weird to have the ownership there. The ownership of something inside CSYNC* should be from CSYNC*

ogoffart

I don't agree with having the list in Excludes object, it feels weird to have the ownership there. The ownership of something inside CSYNCshould be from CSYNC

well, the question is if it should still be inside CSYNC. Also, the excludes list is already owned by the Excludes obect. The journal is already owned by the folder or the sync engine. And making it a aprt of exclude also means we keep the compled regexp there even between sync.

ogoffart · 2017-10-02T09:46:08Z

src/csync/csync_exclude.cpp

+/* Only for bnames (not paths) */
+static QString convertToBnameRegexpSyntax(QString exclude)
+{
+    QString s = QRegularExpression::escape(exclude).replace("\\*", ".*").replace("?", ".");


? would be escaped to \?, so that's wrong.

Also the [ and ] needs de-escaping, same for {}, these were supported by fnmatch, i don't know if people have used them in expressions.
And the backslashes needs de-escaping ( \\\\ -> \\ ), maybe more. (What if somebody had a pattern to ingore files with a * (yes, you can have files with * on unix), it would look like *\** after escaping it whould be \*\\\*\* and we need to convert it to .*\*.*

+1 on fixing "?" (and adding tests for it!)

I'm not convinced it's worth it to try to emulate fnmatch behavior completely. For example PathMatchSpec we use on windows also offers quite different matching options.

If we want to be 100% backwards compatible we'll need to introduce a new pattern-prefix character to go into the 'fast regex' codepath and can never get rid of the fnmatch based codepath.

we would not need a pattern-prefix. We could just detect occurence of [ or { in the pattern and then put them in the slash pattern section. they should be rare anyway.

@ogoffart True. Minor: { does not seem to have special meaning to fnmatch, only ?, *, [ and escaping. (had to check glibc's fnmatch_loop.c)

ogoffart · 2017-10-02T09:58:01Z

src/csync/csync_exclude.cpp

+static QString convertToBnameRegexpSyntax(QString exclude)
+{
+    QString s = QRegularExpression::escape(exclude).replace("\\*", ".*").replace("?", ".");
+    //qDebug() << "Converted pattern" << exclude << "to regex" << s;


please cleanup.

guruz · 2017-10-04T14:12:23Z

@ogoffart fixup pushed

guruz · 2017-10-05T07:32:00Z

thanks!

will rebase and squash my 3 commits and have @ckamm 's on top

On Mac, this halves the time spent in csync_excluded_traversal when using check_csync_excluded_performance. A similar performance increase is seen on linux.

guruz added the Performance label Sep 27, 2017

guruz added this to the 2.4.0 milestone Sep 27, 2017

guruz requested review from ckamm and ogoffart September 27, 2017 14:08

This was referenced Sep 27, 2017

Excludes: During directory traversal, use QRegularExpression #6060

Closed

Excludes: Use QRegularExpression for speedup WiP #4999

Closed

ckamm force-pushed the regexp_exclude_2017_v2 branch from 49ca551 to 781d704 Compare September 28, 2017 10:15

ogoffart reviewed Sep 28, 2017

View reviewed changes

ckamm force-pushed the regexp_exclude_2017_v2 branch from 781d704 to e1c625d Compare September 28, 2017 10:49

ckamm force-pushed the regexp_exclude_2017_v2 branch from b574e42 to b61b294 Compare September 29, 2017 12:41

ogoffart reviewed Oct 2, 2017

View reviewed changes

ogoffart suggested changes Oct 2, 2017

View reviewed changes

ogoffart reviewed Oct 4, 2017

View reviewed changes

guruz force-pushed the regexp_exclude_2017_v2 branch from f75fbab to e01b292 Compare October 4, 2017 07:29

ogoffart approved these changes Oct 5, 2017

View reviewed changes

guruz and others added 2 commits October 5, 2017 10:16

Excludes: During directory traversal, use QRegularExpression

31b85ec

On Mac, this halves the time spent in csync_excluded_traversal when using check_csync_excluded_performance. A similar performance increase is seen on linux.

Excludes: optimize csync_is_windows_reserved_word

a985b2f

guruz force-pushed the regexp_exclude_2017_v2 branch from e01b292 to a985b2f Compare October 5, 2017 08:19

guruz merged commit 99d4060 into master Oct 5, 2017

guruz deleted the regexp_exclude_2017_v2 branch October 5, 2017 08:19

ckamm mentioned this pull request Nov 22, 2017

Allow regex exclude patterns #6181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Excludes: During directory traversal, use QRegularExpression #6063

Excludes: During directory traversal, use QRegularExpression #6063

guruz commented Sep 27, 2017

guruz commented Sep 27, 2017

ogoffart Sep 28, 2017

ogoffart Sep 28, 2017

ckamm Sep 28, 2017

ogoffart Sep 28, 2017

ogoffart Sep 28, 2017

ogoffart Sep 28, 2017

ogoffart Sep 28, 2017

ogoffart Sep 28, 2017

ogoffart Sep 28, 2017

ckamm commented Sep 28, 2017

ogoffart commented Sep 28, 2017

guruz commented Sep 29, 2017

ogoffart Oct 2, 2017

ogoffart left a comment

ogoffart Oct 2, 2017

ogoffart commented Oct 2, 2017

guruz commented Oct 2, 2017

ogoffart left a comment

ogoffart Oct 2, 2017

ckamm Oct 4, 2017 •

edited

Loading

ogoffart Oct 4, 2017

ckamm Oct 5, 2017

ogoffart Oct 2, 2017

guruz commented Oct 4, 2017

guruz commented Oct 5, 2017

Excludes: During directory traversal, use QRegularExpression #6063

Excludes: During directory traversal, use QRegularExpression #6063

Conversation

guruz commented Sep 27, 2017

guruz commented Sep 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckamm commented Sep 28, 2017

ogoffart commented Sep 28, 2017

guruz commented Sep 29, 2017

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

ogoffart commented Oct 2, 2017

guruz commented Oct 2, 2017

ogoffart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckamm Oct 4, 2017 • edited Loading

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

guruz commented Oct 4, 2017

guruz commented Oct 5, 2017

ckamm Oct 4, 2017 •

edited

Loading