Patchwork [2,of,2,zstd-revlogs,V2] localrepo: experimental support for non-zlib revlog compression

login
register
mail settings
Submitter Gregory Szorc
Date Jan. 14, 2017, 4:45 a.m.
Message ID <01dce0ba83c49989c2e7.1484369157@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/18214/
State Accepted
Headers show

Comments

Gregory Szorc - Jan. 14, 2017, 4:45 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1484367416 28800
#      Fri Jan 13 20:16:56 2017 -0800
# Node ID 01dce0ba83c49989c2e75bbda111f2816b1eb61e
# Parent  081a7a0c0d5665056476ed35875d663ea5eaac73
localrepo: experimental support for non-zlib revlog compression

The final part of integrating the compression manager APIs into
revlog storage is the plumbing for repositories to advertise they
are using non-zlib storage and for revlogs to instantiate a non-zlib
compression engine.

The main intent of the compression manager work was to zstd all
of the things. Adding zstd to revlogs has proved to be more involved
than other places because revlogs are... special. Very small inputs
and the use of delta chains (which are themselves a form of
compression) are a completely different use case from streaming
compression, which bundles and the wire protocol employ. I've
conducted numerous experiments with zstd in revlogs and have yet
to formalize compression settings and a storage architecture that
I'm confident I won't regret later. In other words, I'm not yet
ready to commit to a new mechanism for using zstd - or any other
compression format - in revlogs.

That being said, having some support for zstd (and other compression
formats) in revlogs in core is beneficial. It can allow others to
conduct experiments.

This patch introduces *highly experimental* support for non-zlib
compression formats in revlogs. Introduced is a config option to
control which compression engine to use. Also introduced is a namespace
of "exp-compression-*" requirements to denote support for non-zlib
compression in revlogs. I've prefixed the namespace with "exp-"
(short for "experimental") because I'm not confident of the
requirements "schema" and in no way want to give the illusion of
supporting these requirements in the future. I fully intend to drop
support for these requirements once we figure out what we're doing
with zstd in revlogs.

A good portion of the patch is teaching the requirements system
about registered compression engines and passing the requested
compression engine as an opener option so revlogs can instantiate
the proper compression engine for new operations.

That's a verbose way of saying "we can now use zstd in revlogs!"

On an `hg pull` conversion of the mozilla-unified repo with no extra
redelta settings (like aggressivemergedeltas), we can see the impact
of zstd vs zlib in revlogs:

$ hg perfrevlogchunks -c
! chunk
! wall 2.032052 comb 2.040000 user 1.990000 sys 0.050000 (best of 5)
! wall 1.866360 comb 1.860000 user 1.820000 sys 0.040000 (best of 6)

! chunk batch
! wall 1.877261 comb 1.870000 user 1.860000 sys 0.010000 (best of 6)
! wall 1.705410 comb 1.710000 user 1.690000 sys 0.020000 (best of 6)

$ hg perfrevlogchunks -m
! chunk
! wall 2.721427 comb 2.720000 user 2.640000 sys 0.080000 (best of 4)
! wall 2.035076 comb 2.030000 user 1.950000 sys 0.080000 (best of 5)

! chunk batch
! wall 2.614561 comb 2.620000 user 2.580000 sys 0.040000 (best of 4)
! wall 1.910252 comb 1.910000 user 1.880000 sys 0.030000 (best of 6)

$ hg perfrevlog -c -d 1
! wall 4.812885 comb 4.820000 user 4.800000 sys 0.020000 (best of 3)
! wall 4.699621 comb 4.710000 user 4.700000 sys 0.010000 (best of 3)

$ hg perfrevlog -m -d 1000
! wall 34.252800 comb 34.250000 user 33.730000 sys 0.520000 (best of 3)
! wall 24.094999 comb 24.090000 user 23.320000 sys 0.770000 (best of 3)

Only modest wins for the changelog. But manifest reading is
significantly faster. What's going on?

One reason might be data volume. zstd decompresses faster. So given
more bytes, it will put more distance between it and zlib.

Another reason is size. In the current design, zstd revlogs are
*larger*:

debugcreatestreamclonebundle (size in bytes)
zlib: 1,638,852,492
zstd: 1,680,601,332

I haven't investigated this fully, but I reckon a significant cause of
larger revlogs is that the zstd frame/header has more bytes than
zlib's. For very small inputs or data that doesn't compress well, we'll
tend to store more uncompressed chunks than with zlib (because the
compressed size isn't smaller than original). This will make revlog
reading faster because it is doing less decompression.

Moving on to bundle performance:

$ hg bundle -a -t none-v2 (total CPU time)
zlib: 102.79s
zstd:  97.75s

So, marginal CPU decrease for reading all chunks in all revlogs
(this is somewhat disappointing).

$ hg bundle -a -t <engine>-v2 (total CPU time)
zlib: 191.59s
zstd: 115.36s

This last test effectively measures the difference between zlib->zlib
and zstd->zstd for revlogs to bundle. This is a rough approximation of
what a server does during `hg clone`.

There are some promising results for zstd. But not enough for me to
feel comfortable advertising it to users. We'll get there...
Yuya Nishihara - Jan. 15, 2017, 9:33 a.m.
On Fri, 13 Jan 2017 20:45:57 -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1484367416 28800
> #      Fri Jan 13 20:16:56 2017 -0800
> # Node ID 01dce0ba83c49989c2e75bbda111f2816b1eb61e
> # Parent  081a7a0c0d5665056476ed35875d663ea5eaac73
> localrepo: experimental support for non-zlib revlog compression

> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -284,6 +284,12 @@ class localrepository(object):
>          else:
>              self.supported = self._basesupported
>  
> +        # Add compression engines.
> +        for name in util.compengines:
> +            engine = util.compengines[name]
> +            if engine.revlogheader:

Perhaps this should be "if engine.revlogheader()". Maybe it can be fixed
in flight?
Pierre-Yves David - Jan. 15, 2017, 10:06 a.m.
On 01/15/2017 10:33 AM, Yuya Nishihara wrote:
> On Fri, 13 Jan 2017 20:45:57 -0800, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1484367416 28800
>> #      Fri Jan 13 20:16:56 2017 -0800
>> # Node ID 01dce0ba83c49989c2e75bbda111f2816b1eb61e
>> # Parent  081a7a0c0d5665056476ed35875d663ea5eaac73
>> localrepo: experimental support for non-zlib revlog compression
>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -284,6 +284,12 @@ class localrepository(object):
>>          else:
>>              self.supported = self._basesupported
>>
>> +        # Add compression engines.
>> +        for name in util.compengines:
>> +            engine = util.compengines[name]
>> +            if engine.revlogheader:
>
> Perhaps this should be "if engine.revlogheader()". Maybe it can be fixed
> in flight?

Good catch, I've fixed the version on the hg-committed repo.

My python Programmer sense are wondering why this isn't an 
attribute/property. This seems like a "static" property of the object 
(actually even of the class for most of them) and would seems more 
natural to me written "engine.revlogheader" in Python. In addition, 
given this value is boolean, it would help prevent issue as the one yuya 
just caught.
What do you think?

Cheers,
Gregory Szorc - Jan. 15, 2017, 5:57 p.m.
On Sun, Jan 15, 2017 at 2:06 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 01/15/2017 10:33 AM, Yuya Nishihara wrote:
>
>> On Fri, 13 Jan 2017 20:45:57 -0800, Gregory Szorc wrote:
>>
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc@gmail.com>
>>> # Date 1484367416 28800
>>> #      Fri Jan 13 20:16:56 2017 -0800
>>> # Node ID 01dce0ba83c49989c2e75bbda111f2816b1eb61e
>>> # Parent  081a7a0c0d5665056476ed35875d663ea5eaac73
>>> localrepo: experimental support for non-zlib revlog compression
>>>
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>>> --- a/mercurial/localrepo.py
>>> +++ b/mercurial/localrepo.py
>>> @@ -284,6 +284,12 @@ class localrepository(object):
>>>          else:
>>>              self.supported = self._basesupported
>>>
>>> +        # Add compression engines.
>>> +        for name in util.compengines:
>>> +            engine = util.compengines[name]
>>> +            if engine.revlogheader:
>>>
>>
>> Perhaps this should be "if engine.revlogheader()". Maybe it can be fixed
>> in flight?
>>
>
> Good catch, I've fixed the version on the hg-committed repo.
>
> My python Programmer sense are wondering why this isn't an
> attribute/property. This seems like a "static" property of the object
> (actually even of the class for most of them) and would seems more natural
> to me written "engine.revlogheader" in Python. In addition, given this
> value is boolean, it would help prevent issue as the one yuya just caught.
> What do you think?
>

I agree it feels like an attribute/property and not a method. But there are
a few reasons I chose to use methods:

a) consistency (everything is a method)
b) easier to document the API (see detailed docstrings in compressionengine
class)
c) makes it easier to implement complex logic (I don't like @property
because at the point you are executing a function, you might as well have
the caller specify "()" as a signal that "this isn't a simple lookup").
(See the common performance badness that localrepository.changelog has
caused over the years.)

Given we now have 4 methods defining engine properties and only use them
during engine registration, I'm tempted to refactor the whole mechanism
into a "capabilities()" method that returns a dict. That feels a bit more
extensible and future proof. I could do this today if wanted.
Pierre-Yves David - Jan. 17, 2017, 6:32 a.m.
On 01/15/2017 06:57 PM, Gregory Szorc wrote:
> On Sun, Jan 15, 2017 at 2:06 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>
>
>     On 01/15/2017 10:33 AM, Yuya Nishihara wrote:
>
>         On Fri, 13 Jan 2017 20:45:57 -0800, Gregory Szorc wrote:
>
>             # HG changeset patch
>             # User Gregory Szorc <gregory.szorc@gmail.com
>             <mailto:gregory.szorc@gmail.com>>
>             # Date 1484367416 28800
>             #      Fri Jan 13 20:16:56 2017 -0800
>             # Node ID 01dce0ba83c49989c2e75bbda111f2816b1eb61e
>             # Parent  081a7a0c0d5665056476ed35875d663ea5eaac73
>             localrepo: experimental support for non-zlib revlog compression
>
>
>             diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>             --- a/mercurial/localrepo.py
>             +++ b/mercurial/localrepo.py
>             @@ -284,6 +284,12 @@ class localrepository(object):
>                      else:
>                          self.supported = self._basesupported
>
>             +        # Add compression engines.
>             +        for name in util.compengines:
>             +            engine = util.compengines[name]
>             +            if engine.revlogheader:
>
>
>         Perhaps this should be "if engine.revlogheader()". Maybe it can
>         be fixed
>         in flight?
>
>
>     Good catch, I've fixed the version on the hg-committed repo.
>
>     My python Programmer sense are wondering why this isn't an
>     attribute/property. This seems like a "static" property of the
>     object (actually even of the class for most of them) and would seems
>     more natural to me written "engine.revlogheader" in Python. In
>     addition, given this value is boolean, it would help prevent issue
>     as the one yuya just caught.
>     What do you think?
>
>
> I agree it feels like an attribute/property and not a method. But there
> are a few reasons I chose to use methods:
>
> a) consistency (everything is a method)
> b) easier to document the API (see detailed docstrings in
> compressionengine class)
> c) makes it easier to implement complex logic (I don't like @property
> because at the point you are executing a function, you might as well
> have the caller specify "()" as a signal that "this isn't a simple
> lookup"). (See the common performance badness that
> localrepository.changelog has caused over the years.)
>
> Given we now have 4 methods defining engine properties and only use them
> during engine registration, I'm tempted to refactor the whole mechanism
> into a "capabilities()" method that returns a dict. That feels a bit
> more extensible and future proof. I could do this today if wanted.

This seems like a fine option to avoid the trap we fell into this time.
I would be happy to take such patch.

Cheers,
Gregory Szorc - Jan. 17, 2017, 11:56 p.m.
On Mon, Jan 16, 2017 at 10:32 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 01/15/2017 06:57 PM, Gregory Szorc wrote:
>
>> On Sun, Jan 15, 2017 at 2:06 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>> wrote:
>>
>>
>>
>>     On 01/15/2017 10:33 AM, Yuya Nishihara wrote:
>>
>>         On Fri, 13 Jan 2017 20:45:57 -0800, Gregory Szorc wrote:
>>
>>             # HG changeset patch
>>             # User Gregory Szorc <gregory.szorc@gmail.com
>>             <mailto:gregory.szorc@gmail.com>>
>>
>>             # Date 1484367416 28800
>>             #      Fri Jan 13 20:16:56 2017 -0800
>>             # Node ID 01dce0ba83c49989c2e75bbda111f2816b1eb61e
>>             # Parent  081a7a0c0d5665056476ed35875d663ea5eaac73
>>             localrepo: experimental support for non-zlib revlog
>> compression
>>
>>
>>             diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>>             --- a/mercurial/localrepo.py
>>             +++ b/mercurial/localrepo.py
>>             @@ -284,6 +284,12 @@ class localrepository(object):
>>                      else:
>>                          self.supported = self._basesupported
>>
>>             +        # Add compression engines.
>>             +        for name in util.compengines:
>>             +            engine = util.compengines[name]
>>             +            if engine.revlogheader:
>>
>>
>>         Perhaps this should be "if engine.revlogheader()". Maybe it can
>>         be fixed
>>         in flight?
>>
>>
>>     Good catch, I've fixed the version on the hg-committed repo.
>>
>>     My python Programmer sense are wondering why this isn't an
>>     attribute/property. This seems like a "static" property of the
>>     object (actually even of the class for most of them) and would seems
>>     more natural to me written "engine.revlogheader" in Python. In
>>     addition, given this value is boolean, it would help prevent issue
>>     as the one yuya just caught.
>>     What do you think?
>>
>>
>> I agree it feels like an attribute/property and not a method. But there
>> are a few reasons I chose to use methods:
>>
>> a) consistency (everything is a method)
>> b) easier to document the API (see detailed docstrings in
>> compressionengine class)
>> c) makes it easier to implement complex logic (I don't like @property
>> because at the point you are executing a function, you might as well
>> have the caller specify "()" as a signal that "this isn't a simple
>> lookup"). (See the common performance badness that
>> localrepository.changelog has caused over the years.)
>>
>> Given we now have 4 methods defining engine properties and only use them
>> during engine registration, I'm tempted to refactor the whole mechanism
>> into a "capabilities()" method that returns a dict. That feels a bit
>> more extensible and future proof. I could do this today if wanted.
>>
>
> This seems like a fine option to avoid the trap we fell into this time.
> I would be happy to take such patch.
>

I'll try to patchbomb something in the next ~6 hours. Hopefully I can get
this refactor into 4.1 so we don't have to break the API. I'm pretty sure
Facebook won't like having to port lz4revlog to 2 compression engine
APIs...

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -284,6 +284,12 @@  class localrepository(object):
         else:
             self.supported = self._basesupported
 
+        # Add compression engines.
+        for name in util.compengines:
+            engine = util.compengines[name]
+            if engine.revlogheader:
+                self.supported.add('exp-compression-%s' % name)
+
         if not self.vfs.isdir():
             if create:
                 self.requirements = newreporequirements(self)
@@ -397,6 +403,10 @@  class localrepository(object):
         self.svfs.options['aggressivemergedeltas'] = aggressivemergedeltas
         self.svfs.options['lazydeltabase'] = not scmutil.gddeltaconfig(self.ui)
 
+        for r in self.requirements:
+            if r.startswith('exp-compression-'):
+                self.svfs.options['compengine'] = r[len('exp-compression-'):]
+
     def _writerequirements(self):
         scmutil.writerequires(self.vfs, self.requirements)
 
@@ -1994,6 +2004,18 @@  def newreporequirements(repo):
             if ui.configbool('format', 'dotencode', True):
                 requirements.add('dotencode')
 
+    compengine = ui.config('experimental', 'format.compression', 'zlib')
+    if compengine not in util.compengines:
+        raise error.Abort(_('compression engine %s defined by '
+                            'experimental.format.compression not available') %
+                          compengine,
+                          hint=_('run "hg debuginstall" to list available '
+                                 'compression engines'))
+
+    # zlib is the historical default and doesn't need an explicit requirement.
+    if compengine != 'zlib':
+        requirements.add('exp-compression-%s' % compengine)
+
     if scmutil.gdinitconfig(ui):
         requirements.add('generaldelta')
     if ui.configbool('experimental', 'treemanifest', False):
diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -272,6 +272,7 @@  class revlog(object):
         # Mapping of revision integer to full node.
         self._nodecache = {nullid: nullrev}
         self._nodepos = None
+        self._compengine = 'zlib'
 
         v = REVLOG_DEFAULT_VERSION
         opts = getattr(opener, 'options', None)
@@ -288,6 +289,8 @@  class revlog(object):
             if 'aggressivemergedeltas' in opts:
                 self._aggressivemergedeltas = opts['aggressivemergedeltas']
             self._lazydeltabase = bool(opts.get('lazydeltabase', False))
+            if 'compengine' in opts:
+                self._compengine = opts['compengine']
 
         if self._chunkcachesize <= 0:
             raise RevlogError(_('revlog chunk cache size %r is not greater '
@@ -345,7 +348,7 @@  class revlog(object):
 
     @util.propertycache
     def _compressor(self):
-        return util.compengines['zlib'].revlogcompressor()
+        return util.compengines[self._compengine].revlogcompressor()
 
     def tip(self):
         return self.node(len(self.index) - 2)
diff --git a/tests/test-repo-compengines.t b/tests/test-repo-compengines.t
new file mode 100644
--- /dev/null
+++ b/tests/test-repo-compengines.t
@@ -0,0 +1,78 @@ 
+A new repository uses zlib storage, which doesn't need a requirement
+
+  $ hg init default
+  $ cd default
+  $ cat .hg/requires
+  dotencode
+  fncache
+  generaldelta
+  revlogv1
+  store
+
+  $ touch foo
+  $ hg -q commit -A -m 'initial commit with a lot of repeated repeated repeated text to trigger compression'
+  $ hg debugrevlog -c | grep 0x78
+      0x78 (x)  :   1 (100.00%)
+      0x78 (x)  : 110 (100.00%)
+
+  $ cd ..
+
+Unknown compression engine to format.compression aborts
+
+  $ hg --config experimental.format.compression=unknown init unknown
+  abort: compression engine unknown defined by experimental.format.compression not available
+  (run "hg debuginstall" to list available compression engines)
+  [255]
+
+A requirement specifying an unknown compression engine results in bail
+
+  $ hg init unknownrequirement
+  $ cd unknownrequirement
+  $ echo exp-compression-unknown >> .hg/requires
+  $ hg log
+  abort: repository requires features unknown to this Mercurial: exp-compression-unknown!
+  (see https://mercurial-scm.org/wiki/MissingRequirement for more information)
+  [255]
+
+  $ cd ..
+
+#if zstd
+
+  $ hg --config experimental.format.compression=zstd init zstd
+  $ cd zstd
+  $ cat .hg/requires
+  dotencode
+  exp-compression-zstd
+  fncache
+  generaldelta
+  revlogv1
+  store
+
+  $ touch foo
+  $ hg -q commit -A -m 'initial commit with a lot of repeated repeated repeated text'
+
+  $ hg debugrevlog -c | grep 0x28
+      0x28      :  1 (100.00%)
+      0x28      : 98 (100.00%)
+
+  $ cd ..
+
+Specifying a new format.compression on an existing repo won't introduce data
+with that engine or a requirement
+
+  $ cd default
+  $ touch bar
+  $ hg --config experimental.format.compression=zstd -q commit -A -m 'add bar with a lot of repeated repeated repeated text'
+
+  $ cat .hg/requires
+  dotencode
+  fncache
+  generaldelta
+  revlogv1
+  store
+
+  $ hg debugrevlog -c | grep 0x78
+      0x78 (x)  :   2 (100.00%)
+      0x78 (x)  : 199 (100.00%)
+
+#endif