Patchwork [1,of,4] manifest: make lru size configurable

login
register
mail settings
Submitter Durham Goode
Date Feb. 3, 2015, 3:19 a.m.
Message ID <83bf8c0ecb425fb64903.1422933599@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/7620/
State Superseded
Commit ed5e8a9598cea22b20503ef969cc796ad9a71ea9
Headers show

Comments

Durham Goode - Feb. 3, 2015, 3:19 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1422061563 28800
#      Fri Jan 23 17:06:03 2015 -0800
# Node ID 83bf8c0ecb425fb64903d93918227c162bca111d
# Parent  8b88870cbd1eeefaee0af053ae36728f8c0a1847
manifest: make lru size configurable

On machines with lots of ram, it's beneficial to increase the lru size of the
manifest cache.  On a large repo, configuring the lru to be size 10 can shave a
large rebase (~12 commits) down from 95s to 70s.
Pierre-Yves David - Feb. 3, 2015, 3:20 p.m.
On 02/03/2015 03:19 AM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1422061563 28800
> #      Fri Jan 23 17:06:03 2015 -0800
> # Node ID 83bf8c0ecb425fb64903d93918227c162bca111d
> # Parent  8b88870cbd1eeefaee0af053ae36728f8c0a1847
> manifest: make lru size configurable
>
> On machines with lots of ram, it's beneficial to increase the lru size of the
> manifest cache.  On a large repo, configuring the lru to be size 10 can shave a
> large rebase (~12 commits) down from 95s to 70s.

The option for this appears to be

'ui.manifestcachesize' this is hardly a 'ui' option. We currently do not 
have a good section to fit this. (not really format, not really ui). 
maybe we could rename the `workers` section to `ressources` or 
`performances` or `tuning`. And put them both there.

Patch code looks good otherwise (and feature is cool)
Augie Fackler - Feb. 3, 2015, 3:26 p.m.
On Mon, Feb 02, 2015 at 07:19:59PM -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1422061563 28800
> #      Fri Jan 23 17:06:03 2015 -0800
> # Node ID 83bf8c0ecb425fb64903d93918227c162bca111d
> # Parent  8b88870cbd1eeefaee0af053ae36728f8c0a1847
> manifest: make lru size configurable
>
> On machines with lots of ram, it's beneficial to increase the lru size of the
> manifest cache.  On a large repo, configuring the lru to be size 10 can shave a
> large rebase (~12 commits) down from 95s to 70s.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -323,6 +323,9 @@ class localrepository(object):
>          maxchainlen = self.ui.configint('format', 'maxchainlen')
>          if maxchainlen is not None:
>              self.svfs.options['maxchainlen'] = maxchainlen
> +        manifestcachesize = self.ui.configint('ui', 'manifestcachesize')

After looking at patch 4, I started channelling my inner marmoute, and
manifestcachesize doesn't want to be in [ui] either.

> +        if manifestcachesize is not None:
> +            self.svfs.options['manifestcachesize'] = manifestcachesize
>
>      def _writerequirements(self):
>          reqfile = self.vfs("requires", "w")
> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
> --- a/mercurial/manifest.py
> +++ b/mercurial/manifest.py
> @@ -222,7 +222,11 @@ class manifest(revlog.revlog):
>      def __init__(self, opener):
>          # we expect to deal with not more than four revs at a time,

This comment appears to be out of date. Are you sure it's worth making
this a knob, rather than just doing some experimentation and changing
the fixed value?

>          # during a commit --amend
> -        self._mancache = util.lrucachedict(4)
> +        cachesize = 4
> +        opts = getattr(opener, 'options', None)
> +        if opts is not None:
> +            cachesize = opts.get('manifestcachesize', cachesize)
> +        self._mancache = util.lrucachedict(cachesize)
>          revlog.revlog.__init__(self, opener, "00manifest.i")
>
>      def readdelta(self, node):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - Feb. 4, 2015, 12:19 a.m.
On Tue, 2015-02-03 at 15:20 +0000, Pierre-Yves David wrote:
> 
> On 02/03/2015 03:19 AM, Durham Goode wrote:
> > # HG changeset patch
> > # User Durham Goode <durham@fb.com>
> > # Date 1422061563 28800
> > #      Fri Jan 23 17:06:03 2015 -0800
> > # Node ID 83bf8c0ecb425fb64903d93918227c162bca111d
> > # Parent  8b88870cbd1eeefaee0af053ae36728f8c0a1847
> > manifest: make lru size configurable
> >
> > On machines with lots of ram, it's beneficial to increase the lru size of the
> > manifest cache.  On a large repo, configuring the lru to be size 10 can shave a
> > large rebase (~12 commits) down from 95s to 70s.
> 
> The option for this appears to be
> 
> 'ui.manifestcachesize' this is hardly a 'ui' option. We currently do not 
> have a good section to fit this. (not really format, not really ui). 
> maybe we could rename the `workers` section to `ressources` or 
> `performances` or `tuning`. And put them both there.

We have precedent in format.chunkcachesize, so format seems the obvious
place.

You might be interested in investigating the performance impact of that
variable, btw.
Durham Goode - Feb. 4, 2015, 2:39 a.m.
On 2/3/15 7:26 AM, Augie Fackler wrote:
> On Mon, Feb 02, 2015 at 07:19:59PM -0800, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1422061563 28800
>> #      Fri Jan 23 17:06:03 2015 -0800
>> # Node ID 83bf8c0ecb425fb64903d93918227c162bca111d
>> # Parent  8b88870cbd1eeefaee0af053ae36728f8c0a1847
>> manifest: make lru size configurable
>>
>> On machines with lots of ram, it's beneficial to increase the lru size of the
>> manifest cache.  On a large repo, configuring the lru to be size 10 can shave a
>> large rebase (~12 commits) down from 95s to 70s.
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -323,6 +323,9 @@ class localrepository(object):
>>           maxchainlen = self.ui.configint('format', 'maxchainlen')
>>           if maxchainlen is not None:
>>               self.svfs.options['maxchainlen'] = maxchainlen
>> +        manifestcachesize = self.ui.configint('ui', 'manifestcachesize')
> After looking at patch 4, I started channelling my inner marmoute, and
> manifestcachesize doesn't want to be in [ui] either.
>
>> +        if manifestcachesize is not None:
>> +            self.svfs.options['manifestcachesize'] = manifestcachesize
>>
>>       def _writerequirements(self):
>>           reqfile = self.vfs("requires", "w")
>> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
>> --- a/mercurial/manifest.py
>> +++ b/mercurial/manifest.py
>> @@ -222,7 +222,11 @@ class manifest(revlog.revlog):
>>       def __init__(self, opener):
>>           # we expect to deal with not more than four revs at a time,
> This comment appears to be out of date. Are you sure it's worth making
> this a knob, rather than just doing some experimentation and changing
> the fixed value?
I think making this a knob is still good.  Some of the servers we run hg 
on have a metric crap ton of RAM, and being able to crank this number up 
to some unreasonable value would be convenient.
>
>>           # during a commit --amend
>> -        self._mancache = util.lrucachedict(4)
>> +        cachesize = 4
>> +        opts = getattr(opener, 'options', None)
>> +        if opts is not None:
>> +            cachesize = opts.get('manifestcachesize', cachesize)
>> +        self._mancache = util.lrucachedict(cachesize)
>>           revlog.revlog.__init__(self, opener, "00manifest.i")
>>
>>       def readdelta(self, node):
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -323,6 +323,9 @@  class localrepository(object):
         maxchainlen = self.ui.configint('format', 'maxchainlen')
         if maxchainlen is not None:
             self.svfs.options['maxchainlen'] = maxchainlen
+        manifestcachesize = self.ui.configint('ui', 'manifestcachesize')
+        if manifestcachesize is not None:
+            self.svfs.options['manifestcachesize'] = manifestcachesize
 
     def _writerequirements(self):
         reqfile = self.vfs("requires", "w")
diff --git a/mercurial/manifest.py b/mercurial/manifest.py
--- a/mercurial/manifest.py
+++ b/mercurial/manifest.py
@@ -222,7 +222,11 @@  class manifest(revlog.revlog):
     def __init__(self, opener):
         # we expect to deal with not more than four revs at a time,
         # during a commit --amend
-        self._mancache = util.lrucachedict(4)
+        cachesize = 4
+        opts = getattr(opener, 'options', None)
+        if opts is not None:
+            cachesize = opts.get('manifestcachesize', cachesize)
+        self._mancache = util.lrucachedict(cachesize)
         revlog.revlog.__init__(self, opener, "00manifest.i")
 
     def readdelta(self, node):