On 11/25/2016 04:28 AM, Gregory Szorc wrote:
# HG changeset patch
# User Gregory Szorc <gregory.sz...@gmail.com>
# Date 1480031508 28800
#      Thu Nov 24 15:51:48 2016 -0800
# Node ID c60995fce14b0e34bd1416dab3712a6c33bb29bb
# Parent  906a7d8e969552536fffe0df7a5e63bf5d79b34b
revlog: add clone method

Upcoming patches will introduce functionality for in-place
repository/store "upgrades." Copying the contents of a revlog
feels sufficiently low-level to warrant being in the revlog
class. So this commit implements that functionality.

Because full delta recomputation can be *very* expensive (we're
talking several hours on the Firefox repository), we support
multiple modes of execution with regards to delta (re)use. This
will allow repository upgrades to choose the "level" of
processing/optimization they wish to perform when converting
revlogs.

It's not obvious from this commit, but "addrevisioncb" will be
used for progress reporting.

diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -1818,3 +1818,95 @@ class revlog(object):
         if not self._inline:
             res.append(self.datafile)
         return res
+
+    DELTAREUSEALWAYS = 'always'
+    DELTAREUSESAMEREVS = 'samerevs'
+    DELTAREUSENEVER = 'never'

We don't use cap in our names, even for constant, these should be lower case.

I personnaly find "delta reuse" a bit obscure and would probably go for "rediff" or something like that instead. I'm fine with keeping "delta reuse" is you are not convinced.

On a broader topic, we should probably get a config option for that. This is the kind of behavior we might what to be able to control on the client/server level. We want a config section dedicated to these (format does not seems appropriate, and "server" (used for validate) is not really really accurate either)

(Yes, this is outside of the scope of this patch but a good spot to mention it)

+    def clone(self, tr, destrevlog, addrevisioncb=None,
+              deltareuse=DELTAREUSESAMEREVS, aggressivemergedeltas=None):
+        """Copy this revlog to another, possibly with format changes.
+
+        The destination revlog will contain the same revisions and nodes.
+        However, it may not be bit-for-bit identical due to e.g. delta encoding
+        differences.

I'm curious about why we pass a destrevlog here instead of creating a new one. Is this because the creation of new revlog is going to be deferred to a "cloned stored" or something? Then why picking old.clone(new) direction instead of new.fromold(old) one?

+        The ``deltareuse`` argument control how deltas from the existing revlog
+        are preserved in the destination revlog. The argument can have the
+        following values:
+
+        DELTAREUSEALWAYS
+           Deltas will always be reused (if possible), even if the destination
+           revlog would not select the same revisions for the delta. This is 
the
+           fastest mode of operation.
+        DELTAREUSESAMEREVS
+           Deltas will be reused if the destination revlog would pick the same
+           revisions for the delta. This mode strikes a balance between speed
+           and optimization.
+        DELTAREUSENEVER
+           Deltas will never be reused. This is the slowest mode of execution.

Maybe mention that we are improving the diff algorithm and therefor it is useful to rediff sometime ?

+        Delta computation can be slow, so the choice of delta reuse policy can
+        significantly affect run time.
+
+        The default policy (``DELTAREUSESAMEREVS``) strikes a balance between
+        two extremes. Deltas will be reused if they are appropriate. But if the
+        delta could choose a better revision, it will do so. This means if you
+        are converting a non-generaldelta revlog to a generaldelta revlog,
+        deltas will be recomputed if the delta's parent isn't a parent of the
+        revision.
+
+        In addition to the delta policy, the ``aggressivemergedeltas`` argument
+        controls whether to compute deltas against both parents for merges.
+        By default, the current default is used.

This is unrelated to the current patch, but "aggressivemergedeltas" is a very obscure name for "consider both parent for delta". In addition the "official" option seems to live in the "format" config section while this is not a format option. So if we could eventually clean that up at some point that would be great.

+        """
+        if deltareuse not in (self.DELTAREUSEALWAYS, self.DELTAREUSESAMEREVS,
+                self.DELTAREUSENEVER):

We should get a set of valid flag on the class itself, that would make thing simpler:

  if deltause not in self._validdeltastrategies

+            raise ValueError(_('value for deltareuse invalid: %s') % 
deltareuse)
+
+        if len(destrevlog):
+            raise ValueError(_('destination revlog is not empty'))
+
+        # lazydeltabase controls whether to reuse a cached delta, if possible.
+        oldlazydeltabase = destrevlog._lazydeltabase
+        oldamd = destrevlog._aggressivemergedeltas
+
+        if deltareuse == self.DELTAREUSEALWAYS:
+            destrevlog._lazydeltabase = True
+        elif deltareuse == self.DELTAREUSESAMEREVS:
+            destrevlog._lazydeltabase = False

The assignment should happen within the 'try:', otherwise the old value might fail to be reinstalled.

+
+        destrevlog._aggressivemergedeltas = aggressivemergedeltas or oldamd

Same here, assignment should happen in the try.

+        populatecachedelta = deltareuse in (self.DELTAREUSEALWAYS,
+                                            self.DELTAREUSESAMEREVS)
+
+        try:
+            index = self.index
+            for rev in self:
+                entry = index[rev]
+
+                # Some classes override linkrev to take filtered revs into
+                # account. Use raw entry from index.

I don't think cloning on filtered revlog make sense. We should enforce clone to run on unfiltered ones.

+                linkrev = entry[4]
+                p1 = index[entry[5]][7]
+                p2 = index[entry[6]][7]
+                node = entry[7]
+                text = self.revision(rev)

IF we reuse delta, getting the revision here might lead to necessary processing. Am I missing something?

+                # (Possibly) reuse the delta from the revlog if allowed and
+                # the revlog chunk is a delta.
+                cachedelta = None
+                if populatecachedelta:
+                    dp = self.deltaparent(rev)
+                    if dp != nullrev:
+                        cachedelta = (dp, str(self._chunk(rev)))
+
+                destrevlog.addrevision(text, tr, linkrev, p1, p2,
+                                       cachedelta=cachedelta, node=node)
+
+                if addrevisioncb:
+                    addrevisioncb(self, rev, node)
+        finally:
+            destrevlog._lazydeltabase = oldlazydeltabase
+            destrevlog._aggressivemergedeltas = oldamd


--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Reply via email to