Add support for LZ4 #590

bbockelm · 2017-05-25T20:21:59Z

This PR adds support for the LZ4 compression algorithm (branch has been significantly cleaned up).

In testing with the sample Event object found in the test subdirectory, we found that the resulting files were about 13% larger than the default compression settings. Deserialization speed was 95% of the uncompressed speed.

As discussed with @pcanal

Add lz4 source tarball for built-in version. Tweak Event example to make it easier to compare compression algorithms. Tweak build files for Event example to include TreePlayer (for TTreePerfStats). Until we're sure we need it, no special path for Win32. Add -fPIC flag to compile lz4

phsft-bot · 2017-05-25T20:22:02Z

Can one of the admins verify this patch?

vgvassilev · 2017-05-25T20:22:48Z

@phsft-bot build!

phsft-bot · 2017-05-25T20:23:01Z

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with CMake flags -Dvc=OFF -Dimt=ON -Dccache=ON

vgvassilev

Thanks for working on this!

vgvassilev · 2017-05-25T20:24:15Z

cmake/modules/SearchInstalledSoftware.cmake



+#---Check for LZ4--------------------------------------------------------------------
+if(NOT builtin_lz4)


Can we transform the two similar if-stmts into one if-else?

Not easily: the true-branch of the first if-statement can affect the outcome of the second conditional.

Ah, I see. That part is tricky to read. Maybe we could add a clarification comment for the casual reader.

vgvassilev · 2017-05-25T20:25:00Z

core/lz4/src/ZipLZ4.c

+// Original Author: Brian Bockelman
+
+/*************************************************************************
+ * Copyright (C) 1995-2015, Rene Brun and Fons Rademakers.               *


Could we update the copyright year?

vgvassilev · 2017-05-25T20:26:23Z

core/lz4/src/ZipLZ4.c

+#include <stdint.h>
+
+#if (__GNUC__ >= 3) || defined(__INTEL_COMPILER)
+#if !defined(R__unlikely)


Can't we rely on RConfig that would do this for us?

Yup - cleaning that up as part of the response to clang-tidy's other suggestions.

vgvassilev · 2017-05-25T20:28:45Z

test/MainEvent.cxx


   TFile *hfile;
   TTree *tree;
+   TTreePerfStats *ioperf = NULL;


We should use a nullptr.

phsft-bot · 2017-05-25T20:48:23Z

Build failed on slc6/gcc62.
See console output.

Failing tests:

phsft-bot · 2017-05-25T21:18:16Z

Build failed on mac1012/native.
See console output.

Warnings:

include/TMVA/DNN/Architectures/Cpu/CpuMatrix.h:62:50: warning: instantiation of variable 'TMVA::DNN::TCpuMatrix<double>::fOnes' required here, but no definition is available [-Wundefined-var-template]
include/TMVA/DNN/Architectures/Cpu/CpuMatrix.h:144:4: warning: instantiation of variable 'TMVA::DNN::TCpuMatrix<double>::fPool' required here, but no definition is available [-Wundefined-var-template]
include/TMVA/DNN/Architectures/Cpu/CpuMatrix.h:62:50: warning: instantiation of variable 'TMVA::DNN::TCpuMatrix<float>::fOnes' required here, but no definition is available [-Wundefined-var-template]
include/TMVA/DNN/Architectures/Cpu/CpuMatrix.h:144:4: warning: instantiation of variable 'TMVA::DNN::TCpuMatrix<float>::fPool' required here, but no definition is available [-Wundefined-var-template]

Failing tests:

pcanal · 2017-05-25T21:22:14Z

core/zip/src/Bits.h

-    return;
+     R__zipLZMA(cxlevel, srcsize, src, tgtsize, tgt, irep);
+     return;
+  } else if (compressionAlgorithm == 4) {


please add and use a constant (like kLZMA) ...

pcanal · 2017-05-25T21:24:13Z

test/MainEvent.cxx

      nevent = TMath::Min(nevent,nentries);
      if (read == 1) {  //read sequential
         //by setting the read cache to -1 we set it to the AutoFlush value when writing
+         ioperf = new TTreePerfStats("Perf Stats", tree);


Can we make the use of TTreePerfStats optional?

Why? I don't understand; seems to be harmless enough.

The data collection takes 'some' time and thus if one wants to externally merge 'pure' performance one would want to run without.

also the comment on line 189 belongs next to line 191

pcanal · 2017-05-25T21:24:49Z

test/Makefile

 EVENTO        = Event.$(ObjSuf) EventDict.$(ObjSuf)
 EVENTS        = Event.$(SrcSuf) EventDict.$(SrcSuf)
 EVENTSO       = libEvent.$(DllSuf)
+EVENTLIBS     = -lTreePlayer


An equivalent is needed in the windows section.

Not following: this is in a platform-neutral part of the Makefile, no?

Yes but -l is not platform-neutral :) [So the -l indeed need to be avoided on windows.]

pcanal · 2017-05-25T21:26:12Z

core/zip/src/ZInflate.c

-    fprintf(stderr, "Error R__unzip_header: error in header\n");
-    return 1;
+      !(src[0] == 'C' && src[1] == 'S' && src[2] == Z_DEFLATED) && !(src[0] == 'X' && src[1] == 'Z' && src[2] == 0) &&
+      !(src[0] == 'L' && src[1] == '4')) {


This is becoming unwindly, can we create a named repository of those pairs to use here and the 2 or 3 other places.

bbockelm · 2017-05-25T21:27:46Z

@pcanal - any idea how to debug the ctest failure? Or at least get it to cough up the failure message?

When I run make cleantest directly it succeeds; when run via ctest, it fails.

phsft-bot · 2017-05-25T21:30:58Z

Build failed on slc6/gcc49.
See console output.

Failing tests:

phsft-bot · 2017-05-25T21:32:55Z

Build failed on centos7/gcc49.
See console output.

Failing tests:

pcanal · 2017-05-25T21:45:25Z

the root-io-geo-make error is due to a missing commit that is (new) on the master.

for the other error try:

VERBOSE=true ctest  --output-on-failure --no-compress-output -V -R roottest.root.io.compression

bbockelm · 2017-05-25T21:51:50Z

The ctest failure should be fixed by root-project/roottest#45

phsft-bot · 2017-05-25T21:59:43Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

pcanal · 2017-05-25T22:18:50Z

@bbockelm please, try to be explicit in the git log so that later (much later) we can avoid looking at commit (to find a problem) ... "addressing review comment" does not give (our later selves) any clue of what is the nature of the change. [The only exception to that rule are 'white space' only change that have no functional side-effect]. Thanks.

pcanal · 2017-05-25T22:19:40Z

core/zip/src/ZInflate.c

+   return src[0] == 'C' && src[1] == 'S' && src[2] == Z_DEFLATED;
+}
+
+static int is_valid_header_xz(uch *src) {


is it 'xz' or LZMA?

It is technically xz, I believe. We use lzma elsewhere. Will update.

pcanal · 2017-05-25T22:22:25Z

core/zip/src/ZInflate.c


  /* New zlib format */
-  if (src[0] == 'Z' && src[1] == 'L') {
+  if (is_valid_header_zlib(src)) {


This is as the side effect of adding one comparison ... which actually lead me to ponder what the semantic of the (src[2] == Z_DEFLATED) test is ... and why/whether there isn't an equivalent for LZ4.

That's simply the header, as defined by core/zip/src/Bits.h. The header appears to be custom to ROOT. For LZMA and LZ4, we have just used a two-byte header.

pcanal · 2017-05-26T07:23:48Z

test/MainEvent.cxx

      Int_t nentries = (Int_t)tree->GetEntries();
      nevent = TMath::Min(nevent,nentries);
      if (read == 1) {  //read sequential
+         ioperf = getenv("ENABLE_TTREEPERFSTATS") ? new TTreePerfStats("Perf Stats", tree) : nullptr;


Can we document this in the header?

this is the last change need for merging. [Adding support for configure/make so that we can backport to older version can/should be another merge request]

That's already taken care of, no?

My bad, I had missed it ... but now I also took another look at the commit messages ... for the sake of our future selves can you update the commit message that just say things like "Small fixups from review." into a short description of the functional changes :). Thanks.

pcanal · 2017-05-26T08:54:11Z

test/Makefile

 ifeq ($(PLATFORM),win32)
 EVENTLIB      = libEvent.lib
 else
+EVENTLIBS     = -lTreePlayer


Need to add

EVENTLIBS = $(ROOTSYS)/lib/libTreePlayer.lib

on the windows side.

pcanal · 2017-05-26T08:55:09Z

core/lz4/src/ZipLZ4.c

+   }
+
+   tgt[0] = 'L';
+   tgt[1] = '4';


It would have been 'even' better if those had been abstracted out in the same place as the reading.

That would be difficult - the generic header code and the compression-format specific code is interspersed with the actual implementation of the "old-style" compression algorithm.

I would prefer to defer this for follow-up work.

Fair enough. For the record, I would use a (potential) new header defining a set of strings/arrays ....

Yup, that's what I was thinking - new header to define the constants and rationalize the layout of the files (particularly with an eye towards separating the old C code).

pcanal · 2017-05-27T09:45:07Z

test/MainEvent.cxx

      Int_t nentries = (Int_t)tree->GetEntries();
      nevent = TMath::Min(nevent,nentries);
      if (read == 1) {  //read sequential
+         ioperf = getenv("ENABLE_TTREEPERFSTATS") ? new TTreePerfStats("Perf Stats", tree) : nullptr;


this is the last change need for merging. [Adding support for configure/make so that we can backport to older version can/should be another merge request]

Update copyright date on files new to this branch. Add comment explaining logic in CMakeFile.txt Mostly whitespaces; of note, the re-definition of R__likely/unlikely has been moved back into RConfig.h.

Add LZ4 support to core library.

bbockelm · 2017-05-29T11:47:55Z

@pcanal - the GitHub PR page right now is a bit messy, but the rewording of the commit messages you requested is complete. Ready to go?

pcanal · 2017-05-29T11:49:24Z

Yes, thanks!

pcanal · 2017-05-29T11:50:35Z

Could you open a PR to update the configure/make :) ? Thanks.

dpiparo · 2017-05-30T06:22:47Z

Hi,

the PR broke Arm and fedora builds: could you please check?
http://cdash.cern.ch/index.php?project=ROOT

Cheers,
D

bbockelm · 2017-05-30T14:48:53Z

@dpiparo - is it possible to pull up the logs? Unfortunately, the error message says simply the error message is in a different log file.

dpiparo · 2017-05-30T19:06:48Z

Hi,

I fixed that since the bug affected my development platform. Now the tarball is fetched from the eos based website from where all other tarballs are downloaded. The reason for the bug was the fact that curl could not use https.

bbockelm · 2017-05-30T19:23:02Z

Thanks Danilo!

Yes, that was a TODO item to stage the tarball to the official repo -- you just saved us some work.

bbockelm added 2 commits May 25, 2017 15:18

Hardcode LZ4 tarball URL to point at GitHub.

0ce7cdf

vgvassilev reviewed May 25, 2017

View reviewed changes

bbockelm force-pushed the master-lz4 branch from a425029 to 23bfa9b Compare May 25, 2017 21:06

pcanal reviewed May 25, 2017

View reviewed changes

bbockelm force-pushed the master-lz4 branch from 4c6ca86 to de61bde Compare May 25, 2017 22:43

pcanal reviewed May 26, 2017

View reviewed changes

bbockelm force-pushed the master-lz4 branch from de61bde to 24b5652 Compare May 26, 2017 10:03

pcanal requested changes May 27, 2017

View reviewed changes

bbockelm added 2 commits May 27, 2017 21:36

Touch-up whitespace per clang-tidy.

5324ee6

Update copyright date on files new to this branch. Add comment explaining logic in CMakeFile.txt Mostly whitespaces; of note, the re-definition of R__likely/unlikely has been moved back into RConfig.h.

Utilize constant, not hardcoded integer.

3542706

Add LZ4 support to core library.

bbockelm added 5 commits May 27, 2017 21:38

Refactor common logic into helper functions.

bfc45da

Rename xz to lzma to be self-consistent.

556238d

Make use of TTreePlayer optional.

95f4378

Do not use Unix-specific flag on Windows.

08bbf46

Whitespace-only changes for clang-tidy.

ba6e454

bbockelm force-pushed the master-lz4 branch from 24b5652 to ba6e454 Compare May 28, 2017 02:39

pcanal merged commit 8c50188 into root-project:master May 29, 2017

bbockelm mentioned this pull request Aug 4, 2017

Support for the LZ4 algorithm #59

Closed

phsft-bot mentioned this pull request Dec 1, 2017

[cxxmodules] Also preload TMVA/TreePlayer/Graf #1396

Closed

phsft-bot mentioned this pull request Apr 1, 2018

Autoload canonical namespaces #1547

Merged



		#---Check for LZ4--------------------------------------------------------------------
		if(NOT builtin_lz4)

Add support for LZ4 #590

Add support for LZ4 #590

Uh oh!

Conversation

bbockelm commented May 25, 2017

Uh oh!

phsft-bot commented May 25, 2017

Uh oh!

vgvassilev commented May 25, 2017

Uh oh!

phsft-bot commented May 25, 2017

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented May 25, 2017

Failing tests:

Uh oh!

phsft-bot commented May 25, 2017

Warnings:

Failing tests:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbockelm commented May 25, 2017

Uh oh!

phsft-bot commented May 25, 2017

Failing tests:

Uh oh!

phsft-bot commented May 25, 2017

Failing tests:

Uh oh!

pcanal commented May 25, 2017

Uh oh!

bbockelm commented May 25, 2017

Uh oh!

phsft-bot commented May 25, 2017

Failing tests:

Uh oh!

pcanal commented May 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment