Merge lp:~vila/bzr/374726-gc-annotate into lp:~bzr/bzr/trunk-old

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

This pach fixes the most blatant regression for any gc repository. Whether the repository is packed or not doesn't matter anymore.

There is still a performance regression compared to knit repositories but far more limited (at most 2x) and related to gc different choices for deltas (leading to different reannotate intermediate calls, the same annotations being finally produced anyway).

Since we are not satisfied with annotate performance in either case, I'd like some feedback about whether it's worth spending time on trying to catch up with knit here (inverstigating with jam showed no obvious way to achieve that though) or go for implementing an annotation cache (which is out of this bug scope).

[Ian Clatworthy (ian-clatworthy)]

Just a quick thought: I'd prefer we fixed the comment (or code) than add another comment highlighting that they differ. Assuming you're at the sprint with John, poke him about what the correct comment ought to be. :-)

review: Abstain

[John A Meinel (jameinel)]

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/374726-gc-annotate into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This pach fixes the most blatant regression for any gc repository. Whether the repository is packed or not doesn't matter anymore.
>
> There is still a performance regression compared to knit repositories but far more limited (at most 2x) and related to gc different choices for deltas (leading to different reannotate intermediate calls, the same annotations being finally produced anyway).
>
> Since we are not satisfied with annotate performance in either case, I'd like some feedback about whether it's worth spending time on trying to catch up with knit here (inverstigating with jam showed no obvious way to achieve that though) or go for implementing an annotation cache (which is out of this bug scope).
>
>
>

 Review approve

=== modified file 'bzrlib/repofmt/groupcompress_repo.py'
--- bzrlib/repofmt/groupcompress_repo.py 2009-04-20 08:37:32 +0000
+++ bzrlib/repofmt/groupcompress_repo.py 2009-05-25 19:04:59 +0000
@@ -89,6 +89,9 @@
             index_builder_class(reference_lists=1),
             # Texts: compression and per file graph, for all fileids -
so two
             # reference lists and two elements in the key tuple.
+
+ # XXX: The comment says two reference_lists, yet the param
set it
+ # to 1
             index_builder_class(reference_lists=1, key_elements=2),
             # Signatures: Just blobs to store, no compression, no parents
             # listing.

^- The original comment is just wrong. There is no "compression" parent
listed in groupcompress formats. So there is only the per-file graph,
and thus "reference_lists=1" is correct.

John
=:->

[Vincent Ladeuil (vila)]

>>>>> "Ian" == Ian Clatworthy <email address hidden> writes:

    Ian> Review: Abstain
    Ian> Just a quick thought: I'd prefer we fixed the comment (or code)
    Ian> than add another comment highlighting that they differ. Assuming
    Ian> you're at the sprint with John, poke him about what the correct
    Ian> comment ought to be. :-)

The intent was exactly that :) And, indeed, John reviewed it (mail is on
its painful way :)

    Vincent

=== modified file 'bzrlib/groupcompress.py'
--- bzrlib/groupcompress.py	2009-04-22 17:18:45 +0000
+++ bzrlib/groupcompress.py	2009-05-26 13:35:09 +0000
@@ -1018,15 +1018,19 @@
         else:
             keys = [key]
             parent_map = {key:()}
+        # So we used Graph(self) to load the parent_map, but now that we have
+        # it, we can just query the parent map directly, so create a new Graph
+        # object
+        graph = _mod_graph.Graph(_mod_graph.DictParentsProvider(parent_map))
         head_cache = _mod_graph.FrozenHeadsCache(graph)
         parent_cache = {}
         reannotate = annotate.reannotate
         for record in self.get_record_stream(keys, 'topological', True):
             key = record.key
-            chunks = osutils.chunks_to_lines(record.get_bytes_as('chunked'))
+            lines = osutils.chunks_to_lines(record.get_bytes_as('chunked'))
             parent_lines = [parent_cache[parent] for parent in parent_map[key]]
             parent_cache[key] = list(
-                reannotate(parent_lines, chunks, key, None, head_cache))
+                reannotate(parent_lines, lines, key, None, head_cache))
         return parent_cache[key]
 
     def check(self, progress_bar=None):
 
=== modified file 'bzrlib/knit.py'
--- bzrlib/knit.py	2009-04-29 09:50:57 +0000
+++ bzrlib/knit.py	2009-05-26 13:35:09 +0000
@@ -3407,7 +3407,7 @@
         fulltext.)
 
         :return: A list of (key, index_memo) records, suitable for
-            passing to read_records_iter to start reading in the raw data fro/
+            passing to read_records_iter to start reading in the raw data from
             the pack file.
         """
         if key in self._annotated_lines:
 
=== modified file 'bzrlib/repofmt/groupcompress_repo.py'
--- bzrlib/repofmt/groupcompress_repo.py	2009-04-20 08:37:32 +0000
+++ bzrlib/repofmt/groupcompress_repo.py	2009-05-26 13:35:09 +0000
@@ -87,8 +87,8 @@
             # have a regular 2-list index giving parents and compression
             # source.
             index_builder_class(reference_lists=1),
-            # Texts: compression and per file graph, for all fileids - so two
-            # reference lists and two elements in the key tuple.
+            # Texts: per file graph, for all fileids - so one reference list
+            # and two elements in the key tuple.
             index_builder_class(reference_lists=1, key_elements=2),
             # Signatures: Just blobs to store, no compression, no parents
             # listing.