Re: [systemd-devel] [PATCH 1/2] journal: add LZ4 as optional compressor
Am 08.07.2014 12:22, schrieb Jon Severinsson: Am 06.07.2014 21:47, schrieb Lennart Poettering: BTW, have you checked whether reuseing the XZ context might make the XZ more competitive? On Sun Jul 6 15:01:11 PDT 2014 Reindl Harald wrote: please try a simple test compress 50 MB with XZ and GZ, LZO, LZ4 or BZIP2 - XZ is *magnitudes* slower in any case Actually it is not, or rather, it is only that if you want it to be that. I did a quick comparison using the systemd 214 tar (30 MiB): xz -0 was 28% slower than lz4c -hc, but the result was 19% smaller. xz -0 was even 44% faster than gzip -9, and the result was still 5% smaller. XZ is the wrong compression for anything where user feedback in time matters No, xz -6 is the wrong compression *setting* for that use-case, but at a lower setting xz is quite suitable even for that. even with no care about memory usage which also is part of the game finally xz -0 needs about 3 MiB for compression, hardly a prohibitive requirement you simply missed the benchmarks see topic compress: add benchmark-style test yesterday With 0: XZ: compressed decompressed 2535300963 bytes in 33.01s (73.25MiB/s), ... LZ4: compressed decompressed 2535303543 bytes in 1.31s (1838.75MiB/s), .. signature.asc Description: OpenPGP digital signature ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] journal: add LZ4 as optional compressor
2014-07-08 12:22 GMT+02:00 Jon Severinsson j...@severinsson.net: Am 06.07.2014 21:47, schrieb Lennart Poettering: BTW, have you checked whether reuseing the XZ context might make the XZ more competitive? On Sun Jul 6 15:01:11 PDT 2014 Reindl Harald wrote: please try a simple test compress 50 MB with XZ and GZ, LZO, LZ4 or BZIP2 - XZ is *magnitudes* slower in any case Actually it is not, or rather, it is only that if you want it to be that. I did a quick comparison using the systemd 214 tar (30 MiB): xz -0 was 28% slower than lz4c -hc, but the result was 19% smaller. xz -0 was even 44% faster than gzip -9, and the result was still 5% smaller. XZ is the wrong compression for anything where user feedback in time matters No, xz -6 is the wrong compression *setting* for that use-case, but at a lower setting xz is quite suitable even for that. even with no care about memory usage which also is part of the game finally xz -0 needs about 3 MiB for compression, hardly a prohibitive requirement. For comparison, this is what I get when running it on my laptop for 210 (a tarball which I had lieing around): xz == $ time xz -6 systemd_210.orig.tar real 0m9.573s user 0m9.520s sys 0m0.044s $ du -hs systemd_210.orig.tar.xz 2,6M systemd_210.orig.tar.xz $ time xz -0 systemd_210.orig.tar real 0m0.951s user 0m0.948s sys 0m0.004s $ du -hs systemd_210.orig.tar.xz 3,8M systemd_210.orig.tar.xz gzip === $ time gzip systemd_210.orig.tar real 0m0.590s user 0m0.580s sys 0m0.012s $ du -hs systemd_210.orig.tar.gz 4,1M systemd_210.orig.tar.gz lz4 == $ time lz4 systemd_210.orig.tar Compressed filename will be : systemd_210.orig.tar.lz4 Compressed 28917760 bytes into 6667707 bytes == 23.06% real 0m0.097s user 0m0.080s sys 0m0.012s $ du -hs systemd_210.orig.tar.lz4 6,4M systemd_210.orig.tar.lz4 -- Why is it that all of the instruments seeking intelligent life in the universe are pointed away from Earth? ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] journal: add LZ4 as optional compressor
Am 06.07.2014 21:47, schrieb Lennart Poettering: BTW, have you checked whether reuseing the XZ context might make the XZ more competitive? please try a simple test compress 50 MB with XZ and GZ, LZO, LZ4 or BZIP2 - XZ is *magnitudes* slower in any case, there is simply nothing to optimize - XZ is the wrong compression for anything where user feedback in time matters, even with no care about memory usage which also is part of the game finally signature.asc Description: OpenPGP digital signature ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] journal: add LZ4 as optional compressor
On Sun, Jul 06, 2014 at 01:05:29PM +0200, Ronny Chevalier wrote: 2014-07-05 20:56 GMT+02:00 Zbigniew Jędrzejewski-Szmek zbys...@in.waw.pl: +if (src_size 9) +return -ENOSPC; Also, I am not sure that ENOSPC is appropriate here (and everywhere) ? Why not ENOMEM (since there is no devices involved) ? Goood idea. Thanks for all the comments, I will fix them before next submission. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] journal: add LZ4 as optional compressor
On Sun, Jul 06, 2014 at 02:57:17PM +0200, Zbigniew Jędrzejewski-Szmek wrote: On Sun, Jul 06, 2014 at 01:05:29PM +0200, Ronny Chevalier wrote: 2014-07-05 20:56 GMT+02:00 Zbigniew Jędrzejewski-Szmek zbys...@in.waw.pl: +if (src_size 9) +return -ENOSPC; Also, I am not sure that ENOSPC is appropriate here (and everywhere) ? Why not ENOMEM (since there is no devices involved) ? Goood idea. Actually ENOMEM is used for failed allocations, which we need to distinguish. So it's either E2BIG or ENOSPC. Neither is too good. Thanks for all the comments, I will fix them before next submission. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] journal: add LZ4 as optional compressor
On Sat, 05.07.14 20:56, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
-bool uncompress_blob(const void *src, uint64_t src_size,
- void **dst, uint64_t *dst_alloc_size, uint64_t*
dst_size, uint64_t dst_max) {
+int compress_blob_lz4(const void *src, uint64_t src_size, void *dst,
uint64_t *dst_size) {
+#ifdef HAVE_LZ4
+int r;
+assert(src);
+assert(src_size 0);
+assert(dst);
+assert(dst_size);
+
+/* Returns false if we couldn't compress the data or the
+ * compressed result is longer than the original */
+
+if (src_size 9)
+return -ENOSPC;
+
+r = LZ4_compress_limitedOutput(src, dst + 8, src_size, src_size - 8
- 1);
+if (r = 0)
+return -ENOSPC;
Maybe EAGAIN? Would feel a bit like a kernel API then ;-).
+buf1 = malloc(LZ4_BUFSIZE);
+buf2 = malloc(LZ4_BUFSIZE);
+out = malloc(LZ4_COMPRESSBOUND(LZ4_BUFSIZE));
+if (!buf1 || !buf2 || !out)
+return log_oom();
Shouldn't we just return -ENOMEM here? Feels like a library function, not
a program function, hence it should not log at the anything but debug,
really
+out = malloc(4*LZ4_BUFSIZE);
+if (!out)
+return log_oom();
Same here...
Looks good otheriwse.
BTW, have you checked whether reuseing the XZ context might make the XZ
more competitive?
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] journal: add LZ4 as optional compressor
On Sun, Jul 06, 2014 at 09:47:02PM +0200, Lennart Poettering wrote:
On Sat, 05.07.14 20:56, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
-bool uncompress_blob(const void *src, uint64_t src_size,
- void **dst, uint64_t *dst_alloc_size, uint64_t*
dst_size, uint64_t dst_max) {
+int compress_blob_lz4(const void *src, uint64_t src_size, void *dst,
uint64_t *dst_size) {
+#ifdef HAVE_LZ4
+int r;
+assert(src);
+assert(src_size 0);
+assert(dst);
+assert(dst_size);
+
+/* Returns false if we couldn't compress the data or the
+ * compressed result is longer than the original */
+
+if (src_size 9)
+return -ENOSPC;
+
+r = LZ4_compress_limitedOutput(src, dst + 8, src_size, src_size -
8 - 1);
+if (r = 0)
+return -ENOSPC;
Maybe EAGAIN? Would feel a bit like a kernel API then ;-).
I think it would be confusing... But I see now that there's
EFBIG and ENOBUFS, which seem to fit much better: file to big, no space in
buffer.
+buf1 = malloc(LZ4_BUFSIZE);
+buf2 = malloc(LZ4_BUFSIZE);
+out = malloc(LZ4_COMPRESSBOUND(LZ4_BUFSIZE));
+if (!buf1 || !buf2 || !out)
+return log_oom();
Shouldn't we just return -ENOMEM here? Feels like a library function, not
a program function, hence it should not log at the anything but debug,
really
+out = malloc(4*LZ4_BUFSIZE);
+if (!out)
+return log_oom();
Same here...
This is in the fd streaming code, which outputs error messages on its own.
They
could be moved to the caller, but code would get more complicated with little
gain.
I'll keep it this way for now, since there's only one caller and it doesn't
really
matter.
Looks good otheriwse.
BTW, have you checked whether reuseing the XZ context might make the XZ
more competitive?
No, I haven't. But I seriously doubt how it could improve things enough to
make a difference ( 2 orders).
Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] journal: add LZ4 as optional compressor
Looks good otheriwse. BTW, have you checked whether reuseing the XZ context might make the XZ more competitive? No, I haven't. But I seriously doubt how it could improve things enough to make a difference ( 2 orders). I generated some coredumps, fixed some messages, and things seem to still work... Pushed now, but still disabled by default. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 1/2] journal: add LZ4 as optional compressor
Add liblz4 as an optional dependency when requested with --enable-lz4, and
use it in preference to liblzma for journal blob and coredump compression. To
retain backwards compatibility, XZ is used to decompress old blobs.
Things will function correctly only with lz4-119.
Based on the benchmarks found on the web, lz4 seems to be the best choice for
quick compressors atm. If we decide to go this way, I'll work with upstream
to improve their packaging practices, and at least use pkg-config.
---
Makefile.am | 11 +-
configure.ac | 15 +-
src/journal/compress.c | 382 +++
src/journal/compress.h | 65 +++-
src/journal/coredump.c | 10 +-
src/journal/coredumpctl.c| 2 +-
src/journal/journal-def.h| 28 +++-
src/journal/journal-file.c | 99 ++-
src/journal/journal-file.h | 10 +-
src/journal/journal-verify.c | 51 --
src/journal/sd-journal.c | 36 ++--
src/journal/test-compress.c | 158 +-
12 files changed, 688 insertions(+), 179 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 55a7546eef..c85d66ef32 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3526,14 +3526,12 @@ test_catalog_CPPFLAGS = \
test_catalog_LDADD = \
libsystemd-journal-core.la
-if HAVE_XZ
test_compress_SOURCES = \
src/journal/test-compress.c
test_compress_LDADD = \
libsystemd-journal-internal.la \
libsystemd-shared.la
-endif
libsystemd_journal_core_la_SOURCES = \
src/journal/journald-kmsg.c \
@@ -3617,9 +3615,7 @@ tests += \
test-mmap-cache \
test-catalog
-if HAVE_XZ
tests += test-compress
-endif
pkginclude_HEADERS += \
src/systemd/sd-journal.h \
@@ -3652,10 +3648,10 @@ libsystemd_journal_internal_la_CFLAGS = \
libsystemd_journal_internal_la_LIBADD =
-if HAVE_XZ
libsystemd_journal_internal_la_SOURCES += \
src/journal/compress.c
+if HAVE_XZ
libsystemd_journal_internal_la_CFLAGS += \
$(XZ_CFLAGS)
@@ -3663,6 +3659,11 @@ libsystemd_journal_internal_la_LIBADD += \
$(XZ_LIBS)
endif
+if HAVE_LZ4
+libsystemd_journal_internal_la_LIBADD += \
+ -llz4
+endif
+
if HAVE_GCRYPT
libsystemd_journal_internal_la_SOURCES += \
src/journal/journal-authenticate.c \
diff --git a/configure.ac b/configure.ac
index 93aba06739..7a04ca4fa0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -503,14 +503,24 @@ have_xz=no
AC_ARG_ENABLE(xz, AS_HELP_STRING([--disable-xz], [Disable optional XZ
support]))
if test x$enable_xz != xno; then
PKG_CHECK_MODULES(XZ, [ liblzma ],
-[AC_DEFINE(HAVE_XZ, 1, [Define if XZ is available])
have_xz=yes], have_xz=no)
+[AC_DEFINE(HAVE_XZ, 1, [Define if XZ is available])
have_xz=yes])
if test x$have_xz = xno -a x$enable_xz = xyes; then
-AC_MSG_ERROR([*** Xz support requested but libraries not
found])
+AC_MSG_ERROR([*** XZ support requested but libraries not
found])
fi
fi
AM_CONDITIONAL(HAVE_XZ, [test $have_xz = yes])
#
--
+have_lz4=no
+AC_ARG_ENABLE(lz4, AS_HELP_STRING([--enable-lz4], [Enable optional LZ4
support]))
+AS_IF([test x$enable_lz4 == xyes], [
+AC_CHECK_HEADERS(lz4.h,
+ [AC_DEFINE(HAVE_LZ4, 1, [Define in LZ4 is available])
have_lz4=yes],
+ [AC_MSG_ERROR([*** LZ4 support requested but headers not
found])])
+])
+AM_CONDITIONAL(HAVE_LZ4, [test $have_lz4 = yes])
+
+#
--
AC_ARG_ENABLE([pam],
AS_HELP_STRING([--disable-pam],[Disable optional PAM support]),
[case ${enableval} in
@@ -1266,6 +1276,7 @@ AC_MSG_RESULT([
SECCOMP: ${have_seccomp}
SMACK: ${have_smack}
XZ: ${have_xz}
+LZ4: ${have_lz4}
ACL: ${have_acl}
GCRYPT: ${have_gcrypt}
QRENCODE:${have_qrencode}
diff --git a/src/journal/compress.c b/src/journal/compress.c
index 37c55a8728..f7fc665e9c 100644
--- a/src/journal/compress.c
+++ b/src/journal/compress.c
@@ -23,13 +23,30 @@
#include stdlib.h
#include string.h
#include unistd.h
-#include lzma.h
+
+#ifdef HAVE_XZ
+# include lzma.h
+#endif
+
+#ifdef HAVE_LZ4
+# include lz4.h
+#endif
#include compress.h
#include macro.h
#include util.h
+#include sparse-endian.h
+#include journal-def.h
+
+static const char* const object_compressed_table[_OBJECT_COMPRESSED_MAX] = {
+[OBJECT_COMPRESSED_XZ] = XZ,
+[OBJECT_COMPRESSED_LZ4] = LZ4,
+};
+
+DEFINE_STRING_TABLE_LOOKUP(object_compressed, int);
-bool compress_blob(const void *src, uint64_t src_size, void *dst, uint64_t
*dst_size) {
+int compress_blob_xz(const void *src, uint64_t src_size,
