subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject Re: svn commit: r1801940 - in /subversion/trunk: ./ notes/ subversion/include/ subversion/include/private/ subversion/libsvn_delta/ subversion/libsvn_fs_fs/ subversion/libsvn_subr/ subversion/tests/libsvn_delta/ subversion/tests/libsvn_subr/
Date Mon, 17 Jul 2017 14:00:13 GMT
kotkov@apache.org wrote on Fri, 14 Jul 2017 11:13 +0000:
> Author: kotkov
> Date: Fri Jul 14 11:13:47 2017
> New Revision: 1801940
> 
> URL: http://svn.apache.org/viewvc?rev=1801940&view=rev
> Log:
> fsfs: Add initial support for LZ4 compression.
> 
> This can significantly (up to 3 times) improve the speed of commits and
> other operations with large binary or incompressible files, while still
> maintaining a decent compression ratio.

> From the client perspective, the patch starts using LZ4 compression
> only for file:// protocol, and the support/negotiation of the use of svndiff2
> with LZ4 compression for http:// and svn:// can be added later.

To be clear, ra_svn in current trunk is interoperable with 1.9, right?
I.e., it doesn't use svndiff2 over the wire.

> With this patch, LZ4 compression will be enabled for fsfs repositories which
> specify compression-level=1 in fsfs.conf.
> 
> +++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Fri Jul 14 11:13:47 2017
> @@ -2159,6 +2159,38 @@ rep_write_cleanup(void *data)
>    return APR_SUCCESS;
>  }
>  
> +static void
> +txdelta_to_svndiff(svn_txdelta_window_handler_t *handler,
> +                   void **handler_baton,
> +                   svn_stream_t *output,
> +                   svn_fs_t *fs,
> +                   apr_pool_t *pool)
> +{
> +  fs_fs_data_t *ffd = fs->fsap_data;
> +  int svndiff_version;
> +  int svndiff_compression_level;
> +
> +  if (ffd->delta_compression_level == 1 &&
> +      ffd->format >= SVN_FS_FS__MIN_SVNDIFF2_FORMAT)
> +    {
> +      svndiff_version = 2;
> +      svndiff_compression_level = 0;
> +    }
> +  else if (ffd->format >= SVN_FS_FS__MIN_SVNDIFF1_FORMAT)
> +    {
> +      svndiff_version = 1;
> +      svndiff_compression_level = ffd->delta_compression_level;
> +    }
> +  else
> +    {
> +      svndiff_version = 0;
> +      svndiff_compression_level = 0;
> +    }
> +
> +  svn_txdelta_to_svndiff3(handler, handler_baton, output, svndiff_version,
> +                          svndiff_compression_level, pool);
> +}

I'm a bit uncomfortable with this logic.

1. It violates the principle of least surprise: compression-level=9
means 'gzip -9', compression-level=5 means 'gzip -5', but
compression-level=1 means LZ4 (with the default acceleration_factor)
rather than 'gzip -1'.

2. It leaves no way to use zlib level 1 in f8 filesystems.  This seems
like a decision that should be left to the admin, rather than hardcoded
into the library.

3. What if somebody wanted to add a backend with, say, xz compression.
(xz compression also takes levels like gzip.)  Would it make sense to have
two tunables:
.
    compression-backend = { lz4 | zlib }
    compression-level = {1..N for lz4, 1..9 for zlib}
.
and then other compression backends could be easily added?

This would also allow admins to set the 'acceleration_factor' of lz4.

> The interoperability is implemented by bumping the format of svndiff
> to 2 and the repository file system format to 8.

Cheers,

Daniel

Mime
View raw message