[PATCH 1 of 7 upgraderepo V2] revlog: add clone method

Pierre-Yves David pierre-yves.david at ens-lyon.org
Mon Dec 12 05:04:07 EST 2016



On 11/25/2016 04:28 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc at 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


More information about the Mercurial-devel mailing list