Merge lp:~nataliabidart/ubuntuone-client/speed-up-that-local-rescan into lp:ubuntuone-client

[Natalia Bidart]

Speeding up the local rescan by retrieving metadata per directory. As per latest profile info, we'd be gaining 24% of speed!

Added retrieval of metadata objects per directory (filesystem_manager).
Avoided using os.path.dirname when possible (event_queue).
Made Local Rescan usa the per directory metadata retriever to speed up scanning (local_rescan).

[Facundo Batista (facundo)]

Approved, but please remove "share_id" from _get_share_info() parameters (don't know why this is not catched by pylint as "unused vars").

review: Approve

[Natalia Bidart (nataliabidart)]

> Approved, but please remove "share_id" from _get_share_info() parameters
> (don't know why this is not catched by pylint as "unused vars").

Done, thank you!

[lp:~nataliabidart/ubuntuone-client/speed-up-that-local-rescan]

429. By Natalia Bidart on 2010-03-26

Removed unused share_id param.

[John O'Brien (jdobrien)]

review: Approve

=== modified file 'tests/syncdaemon/test_fsm.py'
--- tests/syncdaemon/test_fsm.py	2010-03-16 23:17:53 +0000
+++ tests/syncdaemon/test_fsm.py	2010-03-26 02:36:46 +0000
@@ -107,10 +107,12 @@
         fsm.vm.add_share(share)
         return share
 
-    def create_node(self, name, is_dir=False):
+    def create_node(self, name, is_dir=False, share=None):
         """Create a node."""
-        path = os.path.join(self.share.path, name)
-        mdid = self.fsm.create(path, self.share.volume_id, is_dir=is_dir)
+        if share is None:
+            share = self.share
+        path = os.path.join(share.path, name)
+        mdid = self.fsm.create(path, share.volume_id, is_dir=is_dir)
         self.fsm.set_node_id(path, "uuid1")
         mdobj = self.fsm.get_by_mdid(mdid)
         return mdobj
@@ -1252,6 +1254,80 @@
                           self.fsm._set_node_id, mdobj, 'bad-uuid', path)
 
 
+class GetMDObjectsInDirTests(FSMTestCase):
+    """Test the get_mdobjs_in_dir method."""
+
+    def create_some_contents(self, share):
+        a = 'a'
+        ab = os.path.join(a, 'b')
+        ab1 = os.path.join(a, 'b1')
+        ab2 = os.path.join(a, 'b2')
+        ac = os.path.join(a, 'c')
+        acd = os.path.join(a, 'd')
+
+        dirs = [a, ab, ab1, ab2, ac, acd]
+        for d in dirs:
+            self.create_node(d, is_dir=True, share=share)
+
+        x = os.path.join(a, 'x.txt')
+        y = os.path.join(ab, 'y.txt')
+        z = os.path.join(ac, 'z.txt')
+
+        files = [x, y, z]
+        for f in files:
+            self.create_node(f, is_dir=False, share=share)
+
+        self.contents[share] = sorted(dirs + files)
+
+    def setUp(self):
+        """Init."""
+        FSMTestCase.setUp(self)
+        self.contents = {}
+        self.create_some_contents(self.share)
+
+    def tearDown(self):
+        """Clean up."""
+        self.contents = {}
+        FSMTestCase.tearDown(self)
+
+    def test_basic(self):
+        """Test basic retrieval."""
+        expected = self.contents[self.share]
+        actual = sorted([d.path for d in
+                         self.fsm.get_mdobjs_in_dir(self.share.path)])
+        self.assertEqual(expected, actual)
+
+    def test_similar_paths(self):
+        """Test having similar paths (a/b, a/b1, a/b2)."""
+        expected = []
+        actual = sorted([d.path for d in
+                         self.fsm.get_mdobjs_in_dir(os.path.join('a', 'b'))])
+        self.assertEqual(expected, actual)
+
+    def test_with_two_shares(self):
+        """Test having 2 shares."""
+        second_share = self.create_share('second_share', 'the_second',
+                                          self.fsm, self.shares_dir)
+        self.create_some_contents(second_share)
+
+        expected = self.contents[second_share]
+        actual = sorted([d.path for d in
+                         self.fsm.get_mdobjs_in_dir(second_share.path)])
+        self.assertEqual(expected, actual)
+
+    def test_both_shares(self):
+        """Test having 2 shares and asking for mdobjs in shares_dir."""
+        second_share = self.create_share('second_share', 'the_second',
+                                          self.fsm, self.shares_dir)
+        self.create_some_contents(second_share)
+
+        expected = sorted(self.contents[self.share] +
+                          self.contents[second_share])
+        actual = sorted([d.path for d in
+                         self.fsm.get_mdobjs_in_dir(self.shares_dir)])
+        self.assertEqual(expected, actual)
+
+
 class StatTests(FSMTestCase):
     """Test all the behaviour regarding the stats."""
 

=== modified file 'ubuntuone/syncdaemon/event_queue.py'
--- ubuntuone/syncdaemon/event_queue.py	2010-03-18 16:51:19 +0000
+++ ubuntuone/syncdaemon/event_queue.py	2010-03-26 02:36:46 +0000
@@ -419,17 +419,16 @@
                     # self.timeout() was *just* called, do nothing here
                     pass
                 else:
-                    f_path = os.path.join(self.held_event.path,
-                                                        self.held_event.name)
-                    t_path = os.path.join(event.path, event.name)
+                    f_path_dir = self.held_event.path
+                    f_path = os.path.join(f_path_dir, self.held_event.name)
+                    t_path_dir = event.path
+                    t_path = os.path.join(t_path_dir, event.name)
 
                     is_from_forreal = not self.is_ignored(f_path)
                     is_to_forreal = not self.is_ignored(t_path)
                     if is_from_forreal and is_to_forreal:
-                        f_share_id = self.eq.fs.get_by_path(\
-                                             os.path.dirname(f_path)).share_id
-                        t_share_id = self.eq.fs.get_by_path(\
-                                             os.path.dirname(t_path)).share_id
+                        f_share_id = self.eq.fs.get_by_path(f_path_dir).share_id
+                        t_share_id = self.eq.fs.get_by_path(t_path_dir).share_id
                         this_is_a_dir = event_is_dir(event)
                         if this_is_a_dir:
                             evtname = "FS_DIR_"
@@ -500,7 +499,7 @@
 
         # check if the path is not frozen
         if self.frozen_path is not None:
-            if os.path.dirname(fullpath) == self.frozen_path:
+            if event.path == self.frozen_path:
                 # this will at least store the last one, for debug
                 # purposses
                 self.frozen_evts = (evt_name, fullpath)

=== modified file 'ubuntuone/syncdaemon/filesystem_manager.py'
--- ubuntuone/syncdaemon/filesystem_manager.py	2010-03-16 23:17:53 +0000
+++ ubuntuone/syncdaemon/filesystem_manager.py	2010-03-26 02:36:46 +0000
@@ -501,6 +501,17 @@
                     all_mdobjs.append(_MDObject(**mdobj))
         return all_mdobjs
 
+    def get_mdobjs_in_dir(self, base_path):
+        """Return all the mdobjs which dir is base_path."""
+        all_mdobjs = []
+        base_path += os.path.sep
+        len_base = len(base_path)
+        for path, mdid in self._idx_path.iteritems():
+            if path[:len_base] == base_path:
+                mdobj = self.fs[mdid]
+                all_mdobjs.append(_MDObject(**mdobj))
+        return all_mdobjs
+
     def get_data_for_server_rescan(self):
         """Generates all the (share, node, hash) tuples needed for rescan"""
         all_data = []

=== modified file 'ubuntuone/syncdaemon/local_rescan.py'
--- ubuntuone/syncdaemon/local_rescan.py	2010-03-18 14:56:25 +0000
+++ ubuntuone/syncdaemon/local_rescan.py	2010-03-26 02:36:46 +0000
@@ -59,7 +59,7 @@
 
     def start(self):
         """Start the comparison."""
-        log_info("start scan all shares")
+        log_info("start scan all volumes")
         to_scan = self._get_volumes()
         for share in to_scan:
             try:
@@ -179,10 +179,10 @@
 
         reactor.callLater(0, safe_scan)
 
-    def _get_share_info(self, share_id, path):
-        """Get all the objects information for a share."""
+    def _get_share_info(self, path):
+        """Get all the objects information for a directory path."""
         share_info = []
-        for obj in self.fsm.get_mdobjs_by_share_id(share_id, path):
+        for obj in self.fsm.get_mdobjs_in_dir(path):
             changd = self.fsm.changed(mdid=obj.mdid)
             share_info.append((obj.path, obj.is_dir, obj.stat, changd,
                                obj.node_id, obj.local_hash, obj.server_hash))
@@ -233,7 +233,7 @@
         log_debug("comparing directory %r", dirpath)
 
         # get the share info
-        share_info = self._get_share_info(share.volume_id, dirpath)
+        share_info = self._get_share_info(dirpath)
         shouldbe = self._paths_filter(share_info, dirpath, len(share.path))
 
         def despair(message, fullname, also_children=False, also_remove=None):
@@ -455,14 +455,14 @@
 
                 log_info("comp yield: directory %r is gone!", fullname)
                 # it's a directory, didn't have any info inside?
-                base_path = fullname[len(share.path)+1:]
                 to_inform = []
 
                 # get all the info inside that dir
-                for shrpath, is_dir, statinfo, _, _, _, _ in share_info:
-                    if shrpath.startswith(base_path):
-                        qparts = len(shrpath.split(os.path.sep))
-                        to_inform.append((qparts, shrpath, is_dir))
+                objs = self.fsm.get_mdobjs_by_share_id(share.volume_id, fullname)
+                for obj in objs:
+                    shrpath = obj.path
+                    qparts = len(shrpath.split(os.path.sep))
+                    to_inform.append((qparts, shrpath, obj.is_dir))
 
                 # order everything from more path components to less (this
                 # will assure correct upgoing walk in the tree)
@@ -540,7 +540,8 @@
         self.eq.freeze_begin(dirpath)
 
         def scan():
-            """the scan, really"""
+            """The scan, really."""
+
             log_debug("scanning the dir %r", dirpath)
             listdir = os.listdir(dirpath)