Merge lp:~jameinel/bzr/1.16-no-first-delta-index into lp:~bzr/bzr/trunk-old

[John A Meinel]

[John A Meinel (jameinel)]

This makes a minor modification to the internal DeltaIndex structure for --dev6 repositories.

Namely, it makes generating the first _index "lazy". The idea is that until we do 'make_delta' or a similar function, we don't actually need the index. This has a fairly big impact on 'bzr commit', especially when combined with:
https://code.edge.launchpad.net/~jameinel/bzr/1.16-commit-fulltext/+merge/6988

With both patches, the peak memory consumption is right around 2x a single file content. I know the last bits that we could poke at to drop it down more, but it requires a bit more complexity.

As an added bonus, with both patches applied the time to commit of a single 90MB file in dev6 is down from 7.3s to 4.2s. I would imagine that the 'initial commit' benchmarks for large projects will also be down significantly.

[Andrew Bennetts (spiv)]

I haven't very carefully read the pyrex changes, but the diff is small and the description makes sense to me.

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-06-04 21:05:44 +0000
+++ NEWS	2009-06-05 02:35:36 +0000
@@ -31,6 +31,12 @@
   bugs with stacking and non default formats.)
   (John Arbash Meinel, #373455)
 
+* ``--development6-rich-root`` delays generating a delta index for the
+  first object inserted into a group. This has a beneficial impact on
+  ``bzr commit`` since each committed texts goes to its own group. For
+  committing a 90MB file, it drops peak memory by about 200MB, and speeds
+  up commit from 7s => 4s. (John Arbash Meinel)
+
 * Numerous operations are now faster for huge projects, i.e. those
   with a large number of files and/or a large number of revisions,
   particularly when the latest development format is used. These

=== modified file 'bzrlib/_groupcompress_pyx.pyx'
--- bzrlib/_groupcompress_pyx.pyx	2009-04-09 20:23:07 +0000
+++ bzrlib/_groupcompress_pyx.pyx	2009-06-05 02:35:36 +0000
@@ -118,6 +118,9 @@
             self._index = NULL
         safe_free(<void **>&self._source_infos)
 
+    def _has_index(self):
+        return (self._index != NULL)
+
     def add_delta_source(self, delta, unadded_bytes):
         """Add a new delta to the source texts.
 
@@ -171,6 +174,9 @@
         source_location = len(self._sources)
         if source_location >= self._max_num_sources:
             self._expand_sources()
+        if source_location != 0 and self._index == NULL:
+            # We were lazy about populating the index, create it now
+            self._populate_first_index()
         self._sources.append(source)
         c_source = PyString_AS_STRING(source)
         c_source_size = PyString_GET_SIZE(source)
@@ -179,11 +185,24 @@
         src.size = c_source_size
 
         src.agg_offset = self._source_offset + unadded_bytes
-        index = create_delta_index(src, self._index)
         self._source_offset = src.agg_offset + src.size
-        if index != NULL:
-            free_delta_index(self._index)
-            self._index = index
+        # We delay creating the index on the first insert
+        if source_location != 0:
+            index = create_delta_index(src, self._index)
+            if index != NULL:
+                free_delta_index(self._index)
+                self._index = index
+
+    cdef _populate_first_index(self):
+        cdef delta_index *index
+        if len(self._sources) != 1 or self._index != NULL:
+            raise AssertionError('_populate_first_index should only be'
+                ' called when we have a single source and no index yet')
+
+        # We know that self._index is already NULL, so whatever
+        # create_delta_index returns is fine
+        self._index = create_delta_index(&self._source_infos[0], NULL)
+        assert self._index != NULL
 
     cdef _expand_sources(self):
         raise RuntimeError('if we move self._source_infos, then we need to'
@@ -201,7 +220,10 @@
         cdef unsigned long delta_size
 
         if self._index == NULL:
-            return None
+            if len(self._sources) == 0:
+                return None
+            # We were just lazy about generating the index
+            self._populate_first_index()
 
         if not PyString_CheckExact(target_bytes):
             raise TypeError('target is not a str')

=== modified file 'bzrlib/groupcompress.py'
--- bzrlib/groupcompress.py	2009-05-29 10:25:37 +0000
+++ bzrlib/groupcompress.py	2009-06-05 02:35:36 +0000
@@ -746,6 +746,14 @@
 
         After calling this, the compressor should no longer be used
         """
+        # TODO: this causes us to 'bloat' to 2x the size of content in the
+        #       group. This has an impact for 'commit' of large objects.
+        #       One possibility is to use self._content_chunks, and be lazy and
+        #       only fill out self._content as a full string when we actually
+        #       need it. That would at least drop the peak memory consumption
+        #       for 'commit' down to ~1x the size of the largest file, at a
+        #       cost of increased complexity within this code. 2x is still <<
+        #       3x the size of the largest file, so we are doing ok.
         content = ''.join(self.chunks)
         self.chunks = None
         self._delta_index = None

=== modified file 'bzrlib/tests/test__groupcompress.py'
--- bzrlib/tests/test__groupcompress.py	2009-04-29 05:53:21 +0000
+++ bzrlib/tests/test__groupcompress.py	2009-06-05 02:35:36 +0000
@@ -272,6 +272,25 @@
         di = self._gc_module.DeltaIndex('test text\n')
         self.assertEqual('DeltaIndex(1, 10)', repr(di))
 
+    def test_first_add_source_doesnt_index_until_make_delta(self):
+        di = self._gc_module.DeltaIndex()
+        self.assertFalse(di._has_index())
+        di.add_source(_text1, 0)
+        self.assertFalse(di._has_index())
+        # However, asking to make a delta will trigger the index to be
+        # generated, and will generate a proper delta
+        delta = di.make_delta(_text2)
+        self.assertTrue(di._has_index())
+        self.assertEqual('N\x90/\x1fdiffer from\nagainst other text\n', delta)
+
+    def test_second_add_source_triggers_make_index(self):
+        di = self._gc_module.DeltaIndex()
+        self.assertFalse(di._has_index())
+        di.add_source(_text1, 0)
+        self.assertFalse(di._has_index())
+        di.add_source(_text2, 0)
+        self.assertTrue(di._has_index())
+
     def test_make_delta(self):
         di = self._gc_module.DeltaIndex(_text1)
         delta = di.make_delta(_text2)