Merge pull request #2194 from mcrha/wip/performance

multifilesystem: Improve performance of PROPFIND

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Changelog
 
 ## 3.8.0.dev
+* Fix: storage/multifilesystem: depth:1 PROPFIND no longer re-runs the filesystem collision check (path_to_filesystem) for every item in a collection; this made listing large collections O(n^2) on file systems not detected as collision-free
+* Improve: storage/multifilesystem: avoid redundant stat() calls per item in get/upload when use_mtime_and_size_for_item_cache is enabled
 
 ## 3.7.8
 * Fix: time-range filter on a VTODO having DTSTART/DUE and also CREATED/COMPLETED used the CREATED->COMPLETED duration instead of the DTSTART->DUE one, so completed tasks were missing from (or wrongly returned by) calendar-query REPORT results

radicale/storage/multifilesystem/discover.py

@@ -88,7 +88,9 @@ class StoragePartDiscover(StorageBase):
 
         for href in collection._list():
             with child_context_manager(sane_path, href):
-                item = collection._get(href)
+                # We don't need to check for collisions, because the file
+                # names are from _list() (os.scandir).
+                item = collection._get(href, verify_href=False)
                 if item is not None:
                     yield item
 

radicale/storage/multifilesystem/get.py

@@ -68,6 +68,7 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock,
                 return None
         else:
             path = os.path.join(self._filesystem_path, href)
+        item_stat: Optional[os.stat_result] = None
         try:
             if self._storage._use_mtime_and_size_for_item_cache is True:
                 # try to avoid "open"
@@ -78,6 +79,9 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock,
                         raise IsADirectoryError(path)
                 if not os.access(path, os.R_OK):
                     raise PermissionError(path)
+                # Single stat() reused below for both the cache hash and
+                # last_modified, instead of stat-ing the same path 3 times.
+                item_stat = os.stat(path)
             else:
                 with open(path, "rb") as f:
                     # early read of the content
@@ -95,7 +99,8 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock,
         # The hash of the component in the file system. This is used to check,
         # if the entry in the cache is still valid.
         if self._storage._use_mtime_and_size_for_item_cache is True:
-            cache_hash = self._item_cache_mtime_and_size(os.stat(path).st_size, os.stat(path).st_mtime_ns)
+            assert item_stat is not None
+            cache_hash = self._item_cache_mtime_and_size(item_stat.st_size, item_stat.st_mtime_ns)
             if self._storage._debug_cache_actions is True:
                 logger.debug("Item cache check  for: %r with mtime and size %r", path, cache_hash)
         else:
@@ -149,7 +154,8 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock,
                 logger.debug("Item cache hit    for: %r", path)
         last_modified = time.strftime(
             "%a, %d %b %Y %H:%M:%S GMT",
-            time.gmtime(os.path.getmtime(path)))
+            time.gmtime(item_stat.st_mtime if item_stat is not None
+                        else os.path.getmtime(path)))
         # Don't keep reference to ``vobject_item``, because it requires a lot
         # of memory.
         return radicale_item.Item(

radicale/storage/multifilesystem/upload.py

@@ -50,7 +50,8 @@ class CollectionPartUpload(CollectionPartGet, CollectionPartCache,
                              (href, self.path, e)) from e
         # store cache file
         if self._storage._use_mtime_and_size_for_item_cache is True:
-            cache_hash = self._item_cache_mtime_and_size(os.stat(path).st_size, os.stat(path).st_mtime_ns)
+            path_stat = os.stat(path)
+            cache_hash = self._item_cache_mtime_and_size(path_stat.st_size, path_stat.st_mtime_ns)
             if self._storage._debug_cache_actions is True:
                 logger.debug("Item cache store  for: %r with mtime and size %r", path, cache_hash)
         else:
@@ -121,7 +122,8 @@ class CollectionPartUpload(CollectionPartGet, CollectionPartCache,
 
             # store cache file
             if self._storage._use_mtime_and_size_for_item_cache is True:
-                cache_hash = self._item_cache_mtime_and_size(os.stat(path).st_size, os.stat(path).st_mtime_ns)
+                path_stat = os.stat(path)
+                cache_hash = self._item_cache_mtime_and_size(path_stat.st_size, path_stat.st_mtime_ns)
                 if self._storage._debug_cache_actions is True:
                     logger.debug("Item cache store  for: %r with mtime and size %r", path, cache_hash)
             else:

radicale/tests/test_storage.py

@@ -344,6 +344,41 @@ class TestMultiFileSystem(BaseTest):
             logs = caplog.messages
             assert len([log for log in logs if "File name collision" in log]) == 2
 
+    def test_propfind_depth1_does_not_check_collisions_per_item(self) -> None:
+        """Listing a collection (PROPFIND depth:1) must not re-run the
+        filesystem collision check (path_to_filesystem) once per item.
+        """
+        self.mkcalendar("/calendar.ics/")
+        item_count = 20
+        for index in range(item_count):
+            uid = "event%d" % index
+            event = ("BEGIN:VCALENDAR\r\nVERSION:2.0\r\n"
+                     "BEGIN:VEVENT\r\nUID:%s\r\nSUMMARY:Event %d\r\n"
+                     "DTSTART:20130901T180000Z\r\nEND:VEVENT\r\n"
+                     "END:VCALENDAR\r\n" % (uid, index))
+            self.put("/calendar.ics/%s.ics" % uid, event)
+
+        call_count = 0
+        original_path_to_filesystem = pathutils.path_to_filesystem
+
+        def counting_path_to_filesystem(*args, **kwargs):
+            nonlocal call_count
+            call_count += 1
+            return original_path_to_filesystem(*args, **kwargs)
+
+        with pytest.MonkeyPatch.context() as monkeypatch:
+            monkeypatch.setattr(pathutils, "path_to_filesystem",
+                                counting_path_to_filesystem)
+            _, responses = self.propfind("/calendar.ics/", """\
+<?xml version="1.0" encoding="utf-8" ?>
+<D:propfind xmlns:D="DAV:">
+  <D:prop><D:getetag/></D:prop>
+</D:propfind>
+""", HTTP_DEPTH="1")
+
+        assert len(responses) == item_count + 1
+        assert call_count < item_count
+
     @pytest.mark.skipif(not shutil.which("flock"), reason="flock command not found")
     @pytest.mark.skipif(radicale.log.logger.getEffectiveLevel() == logging.INFO, reason="requires loglevel DEBUG")
     def test_hook_placeholders_PUT(self, caplog) -> None: