Patchwork [2,of,2,mainline] format: introduce 'format.generaldelta=accept'

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1446479952 0
#      Mon Nov 02 15:59:12 2015 +0000
# Node ID 2176eee3dd6c33a51fab5cf4b4e0ac4fba034f12
# Parent  69c64edb25dd96a23564db5b23b8b432b343ec88
# EXP-Topic generaldelta
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 2176eee3dd6c
format: introduce 'format.generaldelta=accept'

This value will make repository created as general delta by default but will not
make Mercurial aggressively recompute delta for all incoming bundle. Instead,
the delta contained in the bundle will be used. This will allow use to start having
general delta repository created everywhere without triggering massive
re-computation cost for all new clients cloning from old server.

On Fri, 06 Nov 2015 10:47:16 -0500, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1446479952 0
> #      Mon Nov 02 15:59:12 2015 +0000
> # Node ID 2176eee3dd6c33a51fab5cf4b4e0ac4fba034f12
> # Parent  69c64edb25dd96a23564db5b23b8b432b343ec88
> # EXP-Topic generaldelta
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 2176eee3dd6c
> format: introduce 'format.generaldelta=accept'
> 
> This value will make repository created as general delta by default but will not
> make Mercurial aggressively recompute delta for all incoming bundle. Instead,
> the delta contained in the bundle will be used. This will allow use to start having
> general delta repository created everywhere without triggering massive
> re-computation cost for all new clients cloning from old server.
> 
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -256,11 +256,11 @@ class localrepository(object):
>                      self.vfs.append(
>                          "00changelog.i",
>                          '\0\0\0\2' # represents revlogv2
>                          ' dummy changelog to prevent using the old repo layout'
>                      )
> -                if scmutil.gdconfig(self.ui):
> +                if scmutil.gdconfig(self.ui)[0]:
>                      self.requirements.add("generaldelta")
>                  if self.ui.configbool('experimental', 'treemanifest', False):
>                      self.requirements.add("treemanifest")
>                  if self.ui.configbool('experimental', 'manifestv2', False):
>                      self.requirements.add("manifestv2")
> @@ -356,10 +356,11 @@ class localrepository(object):
>              self.svfs.options['manifestcachesize'] = manifestcachesize
>          # experimental config: format.aggressivemergedeltas
>          aggressivemergedeltas = self.ui.configbool('format',
>              'aggressivemergedeltas', False)
>          self.svfs.options['aggressivemergedeltas'] = aggressivemergedeltas
> +        self.svfs.options['lazydeltabase'] = scmutil.gdconfig(self.ui)[1]
>  
>      def _writerequirements(self):
>          scmutil.writerequires(self.vfs, self.requirements)
>  
>      def _checknested(self, path):
> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -228,10 +228,12 @@ class revlog(object):
>                  self._chunkcachesize = opts['chunkcachesize']
>              if 'maxchainlen' in opts:
>                  self._maxchainlen = opts['maxchainlen']
>              if 'aggressivemergedeltas' in opts:
>                  self._aggressivemergedeltas = opts['aggressivemergedeltas']
> +            if 'lazydeltabase' in opts:
> +                self._lazydeltabase = bool(opts['lazydeltabase'])

Just nitpicking, _lazydeltabase isn't initialized if no opts['lazydeltabase']
specified.

On Fri, Nov 6, 2015 at 7:47 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1446479952 0
> #      Mon Nov 02 15:59:12 2015 +0000
> # Node ID 2176eee3dd6c33a51fab5cf4b4e0ac4fba034f12
> # Parent  69c64edb25dd96a23564db5b23b8b432b343ec88
> # EXP-Topic generaldelta
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
> 2176eee3dd6c
> format: introduce 'format.generaldelta=accept'
>
> This value will make repository created as general delta by default but
> will not
> make Mercurial aggressively recompute delta for all incoming bundle.
> Instead,
> the delta contained in the bundle will be used. This will allow use to
> start having
> general delta repository created everywhere without triggering massive
> re-computation cost for all new clients cloning from old server.
>

I'll raise a similar objection that I raised IRL at the Sprint: I find
overloading format.generaldelta to be both a bool and a special string
value to be somewhat confusing. It feels like there are two separate
intents we wish to communicate: a) create revlogs with generaldelta b)
whether to force generaldelta on imported data. Shoehorning them into the
same config doesn't tickle me the right way.

I also think I brought this up at the sprint, but I had this idea of
allowing additional controls for forcing generaldelta. e.g. if the incoming
changegroup is only 20 changesets, it's probably fine to take the re-encode
hit. But if it's 2000 changesets, then we may want to avoid re-encoding.


>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -256,11 +256,11 @@ class localrepository(object):
>                      self.vfs.append(
>                          "00changelog.i",
>                          '\0\0\0\2' # represents revlogv2
>                          ' dummy changelog to prevent using the old repo
> layout'
>                      )
> -                if scmutil.gdconfig(self.ui):
> +                if scmutil.gdconfig(self.ui)[0]:
>                      self.requirements.add("generaldelta")
>                  if self.ui.configbool('experimental', 'treemanifest',
> False):
>                      self.requirements.add("treemanifest")
>                  if self.ui.configbool('experimental', 'manifestv2',
> False):
>                      self.requirements.add("manifestv2")
> @@ -356,10 +356,11 @@ class localrepository(object):
>              self.svfs.options['manifestcachesize'] = manifestcachesize
>          # experimental config: format.aggressivemergedeltas
>          aggressivemergedeltas = self.ui.configbool('format',
>              'aggressivemergedeltas', False)
>          self.svfs.options['aggressivemergedeltas'] = aggressivemergedeltas
> +        self.svfs.options['lazydeltabase'] = scmutil.gdconfig(self.ui)[1]
>
>      def _writerequirements(self):
>          scmutil.writerequires(self.vfs, self.requirements)
>
>      def _checknested(self, path):
> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -228,10 +228,12 @@ class revlog(object):
>                  self._chunkcachesize = opts['chunkcachesize']
>              if 'maxchainlen' in opts:
>                  self._maxchainlen = opts['maxchainlen']
>              if 'aggressivemergedeltas' in opts:
>                  self._aggressivemergedeltas =
> opts['aggressivemergedeltas']
> +            if 'lazydeltabase' in opts:
> +                self._lazydeltabase = bool(opts['lazydeltabase'])
>

"lazy delta base" isn't very intuitive to me. How about "use seen delta
base" or "don't recompute delta base?"

self._lazydeltabase = opts.get('lazydeltabase', False) ?


>
>          if self._chunkcachesize <= 0:
>              raise RevlogError(_('revlog chunk cache size %r is not
> greater '
>                                  'than 0') % self._chunkcachesize)
>          elif self._chunkcachesize & (self._chunkcachesize - 1):
> @@ -1368,11 +1370,15 @@ class revlog(object):
>          else:
>              textlen = len(text)
>
>          # should we try to build a delta?
>          if prev != nullrev:
> -            if self._generaldelta:
> +            if cachedelta and self._generaldelta and self._lazydeltabase:
> +                # Assume what we received from the server is a good choice
> +                # build delta will reuse the cache
> +                d = builddelta(cachedelta[0])
> +            elif self._generaldelta:
>                  if p2r != nullrev and self._aggressivemergedeltas:
>                      d = builddelta(p1r)
>                      d2 = builddelta(p2r)
>                      p1good = self._isgooddelta(d, textlen)
>                      p2good = self._isgooddelta(d2, textlen)
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -1172,9 +1172,16 @@ def wlocksub(repo, cmd, *args, **kwargs)
>                      **kwargs)
>
>  def gdconfig(ui):
>      """helper function to retrieve general delta setting
>
> -    The need for such function is triggered by a soon to come third
> possible
> -    value to the config option."""
> +    return (usegd, lazydelta) tuple
> +
> +    usegd: create new repository as general delta,
> +    lazydelta: do not redelta incoming bundle."""
> +    # the r'' is a trick to have checkconfig ignore the type mismatch
>      # experimental config: format.generaldelta
> -    return ui.configbool('format', 'generaldelta', False)
> +    if ui.config(r'format', 'generaldelta', '') == 'accept':
> +        return True, True
> +    # experimental config: format.generaldelta
> +    usegd = ui.configbool('format', 'generaldelta', False)
> +    return usegd, False
> diff --git a/tests/test-generaldelta.t b/tests/test-generaldelta.t
> --- a/tests/test-generaldelta.t
> +++ b/tests/test-generaldelta.t
> @@ -69,10 +69,50 @@ commit.
>       rev    offset  length   base linkrev nodeid       p1           p2
>         0         0       3      0       1 1406e7411862 000000000000
> 000000000000
>
>    $ cd ..
>
> +Test "accept" as a value
> +
> +delta coming from the server base delta server are not recompressed.
> +(also include the aggressive version for comparison)
> +
> +  $ hg clone repo --pull --config format.generaldelta=accept accept
> +  requesting all changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 4 changesets with 5 changes to 2 files (+2 heads)
> +  updating to branch default
> +  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ hg clone repo --pull --config format.generaldelta=1 full
> +  requesting all changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 4 changesets with 5 changes to 2 files (+2 heads)
> +  updating to branch default
> +  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ hg -R repo debugindex -m
> +     rev    offset  length   base linkrev nodeid       p1           p2
> +       0         0      77      0       0 0273e8a1b972 000000000000
> 000000000000
> +       1        77      57      0       1 e0c49f5ef780 0273e8a1b972
> 000000000000
> +       2       134      77      2       2 de950093e41b 0273e8a1b972
> 000000000000
> +       3       211      57      2       3 db74c7cde4d0 0273e8a1b972
> 000000000000
> +  $ hg -R accept debugindex -m
> +     rev    offset  length  delta linkrev nodeid       p1           p2
> +       0         0      77     -1       0 0273e8a1b972 000000000000
> 000000000000
> +       1        77      57      0       1 e0c49f5ef780 0273e8a1b972
> 000000000000
> +       2       134      77     -1       2 de950093e41b 0273e8a1b972
> 000000000000
> +       3       211      57      2       3 db74c7cde4d0 0273e8a1b972
> 000000000000
> +  $ hg -R full debugindex -m
> +     rev    offset  length  delta linkrev nodeid       p1           p2
> +       0         0      77     -1       0 0273e8a1b972 000000000000
> 000000000000
> +       1        77      57      0       1 e0c49f5ef780 0273e8a1b972
> 000000000000
> +       2       134      57      0       2 de950093e41b 0273e8a1b972
> 000000000000
> +       3       191      57      0       3 db74c7cde4d0 0273e8a1b972
> 000000000000
> +
>  Test format.aggressivemergedeltas
>
>    $ hg init --config format.generaldelta=1 aggressive
>    $ cd aggressive
>    $ touch a b c d e
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>

On 11/08/2015 07:22 AM, Yuya Nishihara wrote:
> On Fri, 06 Nov 2015 10:47:16 -0500, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1446479952 0
>> #      Mon Nov 02 15:59:12 2015 +0000
>> # Node ID 2176eee3dd6c33a51fab5cf4b4e0ac4fba034f12
>> # Parent  69c64edb25dd96a23564db5b23b8b432b343ec88
>> # EXP-Topic generaldelta
>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 2176eee3dd6c
>> format: introduce 'format.generaldelta=accept'
>>
>> This value will make repository created as general delta by default but will not
>> make Mercurial aggressively recompute delta for all incoming bundle. Instead,
>> the delta contained in the bundle will be used. This will allow use to start having
>> general delta repository created everywhere without triggering massive
>> re-computation cost for all new clients cloning from old server.
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -256,11 +256,11 @@ class localrepository(object):
>>                       self.vfs.append(
>>                           "00changelog.i",
>>                           '\0\0\0\2' # represents revlogv2
>>                           ' dummy changelog to prevent using the old repo layout'
>>                       )
>> -                if scmutil.gdconfig(self.ui):
>> +                if scmutil.gdconfig(self.ui)[0]:
>>                       self.requirements.add("generaldelta")
>>                   if self.ui.configbool('experimental', 'treemanifest', False):
>>                       self.requirements.add("treemanifest")
>>                   if self.ui.configbool('experimental', 'manifestv2', False):
>>                       self.requirements.add("manifestv2")
>> @@ -356,10 +356,11 @@ class localrepository(object):
>>               self.svfs.options['manifestcachesize'] = manifestcachesize
>>           # experimental config: format.aggressivemergedeltas
>>           aggressivemergedeltas = self.ui.configbool('format',
>>               'aggressivemergedeltas', False)
>>           self.svfs.options['aggressivemergedeltas'] = aggressivemergedeltas
>> +        self.svfs.options['lazydeltabase'] = scmutil.gdconfig(self.ui)[1]
>>
>>       def _writerequirements(self):
>>           scmutil.writerequires(self.vfs, self.requirements)
>>
>>       def _checknested(self, path):
>> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
>> --- a/mercurial/revlog.py
>> +++ b/mercurial/revlog.py
>> @@ -228,10 +228,12 @@ class revlog(object):
>>                   self._chunkcachesize = opts['chunkcachesize']
>>               if 'maxchainlen' in opts:
>>                   self._maxchainlen = opts['maxchainlen']
>>               if 'aggressivemergedeltas' in opts:
>>                   self._aggressivemergedeltas = opts['aggressivemergedeltas']
>> +            if 'lazydeltabase' in opts:
>> +                self._lazydeltabase = bool(opts['lazydeltabase'])
>
> Just nitpicking, _lazydeltabase isn't initialized if no opts['lazydeltabase']
> specified.

Nice catch, V2 coming.

On 11/08/2015 11:19 AM, Gregory Szorc wrote:
> On Fri, Nov 6, 2015 at 7:47 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@fb.com
>     <mailto:pierre-yves.david@fb.com>>
>     # Date 1446479952 0
>     #      Mon Nov 02 15:59:12 2015 +0000
>     # Node ID 2176eee3dd6c33a51fab5cf4b4e0ac4fba034f12
>     # Parent  69c64edb25dd96a23564db5b23b8b432b343ec88
>     # EXP-Topic generaldelta
>     # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>     #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/
>     -r 2176eee3dd6c
>     format: introduce 'format.generaldelta=accept'
>
>     This value will make repository created as general delta by default
>     but will not
>     make Mercurial aggressively recompute delta for all incoming bundle.
>     Instead,
>     the delta contained in the bundle will be used. This will allow use
>     to start having
>     general delta repository created everywhere without triggering massive
>     re-computation cost for all new clients cloning from old server.
>
>
> I'll raise a similar objection that I raised IRL at the Sprint: I find
> overloading format.generaldelta to be both a bool and a special string
> value to be somewhat confusing. It feels like there are two separate
> intents we wish to communicate: a) create revlogs with generaldelta b)
> whether to force generaldelta on imported data. Shoehorning them into
> the same config doesn't tickle me the right way.

This is sensible concern but I'm not sure what is the best answer there. 
By chance, we have 2 months to go until the freeze so I'm sure "Someone 
who care enough about it" ;-) will figure out what we should do in that 
interval.

> I also think I brought this up at the sprint, but I had this idea of
> allowing additional controls for forcing generaldelta. e.g. if the
> incoming changegroup is only 20 changesets, it's probably fine to take
> the re-encode hit. But if it's 2000 changesets, then we may want to
> avoid re-encoding.

I'm open to improvement in that area, but done is better than perfect. 
I, personnaly, focussing on getting some form of generaldelta enabled by 
default. Anyone who want to push that further is welcome to do so.

I remember a couple of other point discussed that the sprint and I think 
we agree you would look at them by the freeze. So we have plently of 
time to adjust our approach here.

Cheers,

On Sun, 2015-11-08 at 12:09 -0500, Pierre-Yves David wrote:
> 
> On 11/08/2015 11:19 AM, Gregory Szorc wrote:
> > On Fri, Nov 6, 2015 at 7:47 AM, Pierre-Yves David
> > <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.
> > org>>
> > wrote:
> > 
> >     # HG changeset patch
> >     # User Pierre-Yves David <pierre-yves.david@fb.com
> >     <mailto:pierre-yves.david@fb.com>>
> >     # Date 1446479952 0
> >     #      Mon Nov 02 15:59:12 2015 +0000
> >     # Node ID 2176eee3dd6c33a51fab5cf4b4e0ac4fba034f12
> >     # Parent  69c64edb25dd96a23564db5b23b8b432b343ec88
> >     # EXP-Topic generaldelta
> >     # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> >     #              hg pull http://hg.netv6.net/marmoute-wip/mercuri
> > al/
> >     -r 2176eee3dd6c
> >     format: introduce 'format.generaldelta=accept'
> > 
> >     This value will make repository created as general delta by
> > default
> >     but will not
> >     make Mercurial aggressively recompute delta for all incoming
> > bundle.
> >     Instead,
> >     the delta contained in the bundle will be used. This will allow
> > use
> >     to start having
> >     general delta repository created everywhere without triggering
> > massive
> >     re-computation cost for all new clients cloning from old
> > server.
> > 
> > 
> > I'll raise a similar objection that I raised IRL at the Sprint: I
> > find
> > overloading format.generaldelta to be both a bool and a special
> > string
> > value to be somewhat confusing. It feels like there are two
> > separate
> > intents we wish to communicate: a) create revlogs with generaldelta
> > b)
> > whether to force generaldelta on imported data. Shoehorning them
> > into
> > the same config doesn't tickle me the right way.
> 
> This is sensible concern but I'm not sure what is the best answer
> there. 
> By chance, we have 2 months to go until the freeze so I'm sure
> "Someone 
> who care enough about it" ;-) will figure out what we should do in
> that 
> interval.
> 
> > I also think I brought this up at the sprint, but I had this idea
> > of
> > allowing additional controls for forcing generaldelta. e.g. if the
> > incoming changegroup is only 20 changesets, it's probably fine to
> > take
> > the re-encode hit. But if it's 2000 changesets, then we may want to
> > avoid re-encoding.
> 
> I'm open to improvement in that area, but done is better than
> perfect. 
> I, personnaly, focussing on getting some form of generaldelta enabled
> by 
> default. Anyone who want to push that further is welcome to do so.
> 
> I remember a couple of other point discussed that the sprint and I
> think 
> we agree you would look at them by the freeze. So we have plently of 
> time to adjust our approach here.

Here's my proposal: the new option should be named
format.allowgeneraldelta (a boolean that defaults to true).

Works like this (with generaldelta abbreviated):

config       ||      behavior
allowgd | gd || add gd to requires | compute gd | pull gd
   n       n         n                   n           n
   n       y         y                   y           y
   y       n         y                   n           y
   y       y         y                   y           y

-- 
Mathematics is the supreme nostalgia of our time.