compressor: Add Brotli Compressor #19549

bi-shun · 2017-12-15T13:57:23Z

Signed-off-by: BI SHUN KE aionshun@livemail.tw

tchaikov · 2017-12-15T16:08:10Z

.gitmodules

@@ -55,3 +55,6 @@
 [submodule "src/rapidjson"]
 	path = src/rapidjson
 	url = https://github.com/ceph/rapidjson
+[submodule "src/brotli"]
+	path = src/brotli
+	url = https://github.com/google/brotli.git


awesome, but i'd suggest add brotli using ExternalProject_Add() and download brotli on the fly, see also https://github.com/ceph/ceph/blob/master/src/os/CMakeLists.txt#L122

bi-shun · 2017-12-19T07:44:25Z

@tchaikov I have modified as you suggested, can you review again, thanks.

tchaikov · 2017-12-24T09:20:51Z

src/CMakeLists.txt

@@ -719,6 +719,7 @@ add_subdirectory(include)
 add_subdirectory(librados)
 add_subdirectory(libradosstriper)

+


drop this change.

tchaikov · 2017-12-24T09:23:01Z

src/compressor/CMakeLists.txt

@@ -18,7 +19,8 @@ endif()
 set(ceph_compressor_libs
    ceph_snappy
    ceph_zlib
-    ceph_zstd)
+	ceph_zstd


could you avoid using tab in CMake scripts?

@bi-shun this comment is not addressed.

tchaikov · 2017-12-24T09:24:02Z

src/compressor/Compressor.cc

@@ -31,6 +31,7 @@ const char * Compressor::get_comp_alg_name(int a) {
 #ifdef HAVE_LZ4
  case COMP_ALG_LZ4: return "lz4";
 #endif
+  case COMP_ALG_BROTLI: return "brotli";


should guard this case statement with #ifdef HAVE_BROTLI. as "brotli" could be disabled at compile-time.

tchaikov · 2017-12-24T09:24:46Z

src/compressor/Compressor.cc

@@ -46,6 +47,8 @@ boost::optional<Compressor::CompressionAlgorithm> Compressor::get_comp_alg_type(
  if (s == "lz4")
    return COMP_ALG_LZ4;
 #endif
+  if (s == "brotli")


tchaikov · 2017-12-24T09:28:21Z

src/compressor/Compressor.h

@@ -37,6 +37,7 @@ class Compressor {
 #ifdef HAVE_LZ4
    COMP_ALG_LZ4 = 4,
 #endif
+    COMP_ALG_BROTLI = 5,


tchaikov · 2017-12-24T09:53:45Z

src/compressor/brotli/BrotliCompressor.cc

+  size_t available_out = 0;
+  uint8_t* next_out = NULL;
+  if (!s) {
+      BrotliEncoderDestroyInstance(s);


no need to destroy s, if it's not created at all.

@bi-shun this comment is not addressed.

tchaikov · 2017-12-24T09:54:27Z

src/compressor/brotli/BrotliCompressor.cc

+  BrotliEncoderSetParameter(s, BROTLI_PARAM_QUALITY, (uint32_t)9);
+  BrotliEncoderSetParameter(s, BROTLI_PARAM_LGWIN, 22);
+  unsigned char* c_in;
+  for (std::list<buffer::ptr>::const_iterator i = in.buffers().begin(); i != in.buffers().end();) {


nit, use auto

tchaikov · 2017-12-24T09:57:07Z

src/compressor/brotli/BrotliCompressor.cc

+      out.append(ptr, 0, have);
+    } while (available_out == 0);
+    if (BrotliEncoderIsFinished(s)) {
+      BrotliEncoderDestroyInstance(s);


why not just break here?

tchaikov · 2017-12-24T09:58:10Z

src/compressor/brotli/BrotliCompressor.cc

+      have = MAX_LEN - available_out;
+      out.append(ptr, 0, have);
+    } while (available_out == 0);
+    if (BrotliDecoderIsFinished(s)) {


could just break here.

tchaikov · 2017-12-24T10:00:46Z

src/compressor/brotli/CMakeLists.txt

+    COMMAND "true"
+	ALWAYS 1)
+
+add_library(brotlicommon STATIC IMPORTED)


nit, could name it brotli::common and use a loop to set the dependency and the properties of these libraries.

bi-shun · 2017-12-25T09:58:38Z

@tchaikov I have to change the compressor plugin as optional and modify for you suggestions, can you review again, thanks.

tchaikov · 2017-12-25T10:21:20Z

src/test/compressor/test_compression.cc

@@ -330,7 +330,11 @@ INSTANTIATE_TEST_CASE_P(
 #endif
    "zlib/noisal",
    "snappy",
-    "zstd"));
+    "zstd"
+#ifdef HAVE_BROTLI


nit, could add this line before "zstd", less code churn, and better looking this way =)

tchaikov · 2017-12-25T10:21:58Z

src/compressor/brotli/CMakeLists.txt

+    COMMAND "true"
+	ALWAYS 1)
+
+set(bortli_libs brotlicommon brotlidec brotlienc)


s/brotlicommon/brotli::common/
s/brotlidec/brotli::dec/
s/brotlienc/brotli::enc/

tchaikov · 2017-12-25T10:22:54Z

CMakeLists.txt

+option(WITH_BROTLI "Brotli compression support" OFF)
+if(WITH_BROTLI)
+  set(HAVE_BROTLI TRUE)
+endif(WITH_BROTLI)


remove WITH_BROTLI from endif(), as it's a very short if block.

@bi-shun this one.

tchaikov · 2017-12-25T10:23:19Z

CMakeLists.txt

+  set(HAVE_BROTLI TRUE)
+endif(WITH_BROTLI)
+
+


remove this empty line.

@bi-shun and this one.

tchaikov · 2017-12-25T10:58:50Z

src/compressor/CMakeLists.txt

@@ -15,6 +15,10 @@ if (HAVE_LZ4)
  add_subdirectory(lz4)
 endif()

+if (HAVE_BROTLI)


nit, remove the space after if.

@bi-shun here.

bi-shun · 2017-12-25T12:36:19Z

@tchaikov I have fixed the problems.

bi-shun · 2017-12-28T05:38:18Z

@tchaikov Cloud you review again, thanks.

tchaikov · 2017-12-28T12:15:59Z

src/compressor/CMakeLists.txt

@@ -24,6 +28,10 @@ if (HAVE_LZ4)
  list(APPEND ceph_compressor_libs ceph_lz4)
 endif()

+if (HAVE_BROTLI)


tchaikov · 2017-12-28T12:17:33Z

@bi-shun please let me know once all comments are addressed.

bi-shun · 2017-12-28T12:51:04Z

@tchaikov I merge the branch commits into one, and all comments are addressed, thanks.

tchaikov · 2017-12-28T14:43:23Z

src/compressor/brotli/BrotliCompressor.cc

+int BrotliCompressor::compress(const bufferlist &in, bufferlist &out) 
+{
+  BrotliEncoderState* s = BrotliEncoderCreateInstance(nullptr, nullptr, nullptr);
+  auto sg = make_scope_guard([&s] { BrotliEncoderDestroyInstance(s); });


#19549 (comment) , please move this after if (!s) {} block.

tchaikov · 2017-12-28T14:44:00Z

src/compressor/brotli/BrotliCompressor.cc

+  size_t available_in = 0;
+  const uint8_t* next_in = nullptr;
+  size_t available_out = 0;
+  uint8_t* next_out = nullptr;


#19549 (comment)

tchaikov · 2017-12-28T14:45:29Z

src/compressor/brotli/BrotliCompressor.cc

+  }
+  BrotliEncoderSetParameter(s, BROTLI_PARAM_QUALITY, (uint32_t)9);
+  BrotliEncoderSetParameter(s, BROTLI_PARAM_LGWIN, 22);
+  unsigned char* c_in;


no need to have this variable, IMHO.

tchaikov · 2017-12-28T14:47:05Z

src/compressor/brotli/BrotliCompressor.cc

+  unsigned char* c_in;
+  for (auto i = in.buffers().begin(); i != in.buffers().end();) {
+    c_in = (unsigned char*) i->c_str();
+    size_t len = i->length();


why do we need this variable?

tchaikov · 2017-12-28T14:47:24Z

src/compressor/brotli/BrotliCompressor.cc

+      bufferptr ptr = buffer::create_page_aligned(max_comp_size);
+      next_out = (unsigned char*)ptr.c_str();
+      available_out = max_comp_size;
+      if (!BrotliEncoderCompressStream(s,finish,&available_in, &next_in, &available_out, &next_out, nullptr)) {


add a space after , please

please wrap at 80 chars.

tchaikov · 2017-12-28T14:51:20Z

src/compressor/brotli/CMakeLists.txt

+add_library(ceph_brotli SHARED ${brotli_sources})
+add_dependencies(ceph_brotli ${CMAKE_SOURCE_DIR}/src/ceph_ver.h)
+target_include_directories(ceph_brotli	PRIVATE "${CMAKE_BINARY_DIR}/src/brotli/c/include")
+List(REVERSE bortli_libs)


why reverse the libs list?

Because depend sequence.

nit, if brotli::dec and brotli::enc depend on brotli::common, why not ordering it the right way at https://github.com/ceph/ceph/pull/19549/files#diff-66716f6999869d6458bd6a9251638b70R25 ?

tchaikov · 2017-12-28T14:52:56Z

src/test/compressor/test_compression.cc

+#ifdef HAVE_BROTLI
+    "brotli",
+#endif
+    "zstd"


drop this change.

bi-shun · 2017-12-29T07:36:12Z

@tchaikov Thank you very much for your helpful comments and corrections. I tried to correct according to your comments . Could you review again, thanks.

tchaikov

thanks! lgtm, would be great if you can address the comments.

tchaikov · 2017-12-29T09:29:32Z

src/compressor/brotli/CMakeLists.txt

+add_library(ceph_brotli SHARED ${brotli_sources})
+add_dependencies(ceph_brotli ${CMAKE_SOURCE_DIR}/src/ceph_ver.h)
+target_include_directories(ceph_brotli	PRIVATE "${CMAKE_BINARY_DIR}/src/brotli/c/include")
+List(REVERSE bortli_libs)


nit, if brotli::dec and brotli::enc depend on brotli::common, why not ordering it the right way at https://github.com/ceph/ceph/pull/19549/files#diff-66716f6999869d6458bd6a9251638b70R25 ?

tchaikov · 2017-12-29T09:30:18Z

src/compressor/brotli/CMakeLists.txt

+target_include_directories(ceph_brotli	PRIVATE "${CMAKE_BINARY_DIR}/src/brotli/c/include")
+List(REVERSE bortli_libs)
+target_link_libraries(ceph_brotli ${bortli_libs})
+set_target_properties(ceph_brotli PROPERTIES VERSION 2.0.0 SOVERSION 2)


I don't think we need to version the plugin.

tchaikov · 2017-12-29T09:36:53Z

src/compressor/brotli/CMakeLists.txt

+  COMMAND "true"
+  ALWAYS 1)
+
+set(bortli_libs brotli::common brotli::dec brotli::enc)


nit, could avoid the string manipulation, and dedup the brotli:: prefix, see

ceph/cmake/modules/BuildDPDK.cmake

Line 88 in 28c79fa

foreach(c

tchaikov · 2017-12-29T09:37:18Z

src/compressor/brotli/CMakeLists.txt

+
+if(WITH_EMBEDDED)
+  add_library(cephd_compressor_brotli STATIC ${brotli_sources})
+  target_include_directories(cephd_compressor_brotli  PRIVATE "${CMAKE_BINARY_DIR}/src/brotli/c/include")


remove the extra space.

tchaikov · 2017-12-29T09:39:04Z

src/compressor/brotli/CompressionPluginBrotli.h

+
+  virtual int factory(CompressorRef *cs, std::ostream *ss)
+  {
+    if (compressor == 0) {


could just put

if (compressor) { // ... }

or s/0/nullptr/ if you prefer to be explicit.

tchaikov · 2017-12-29T09:43:12Z

the change looks good, could you please squash these two commits into a single one?

Signed-off-by: BI SHUN KE <aionshun@livemail.tw>

bi-shun · 2017-12-29T10:43:03Z

@tchaikov Sure, last the comments, I have corrected and squash it into one, thanks.

yuriw · 2018-01-02T17:11:53Z

wip-yuri4-testing-2018-01-02-1711

tchaikov reviewed Dec 15, 2017

View reviewed changes

bi-shun force-pushed the brotli-compressor branch 2 times, most recently from c44e005 to c7cd84d Compare December 16, 2017 21:09

batrick added core needs-review labels Dec 20, 2017

tchaikov reviewed Dec 24, 2017

View reviewed changes

bi-shun force-pushed the brotli-compressor branch 3 times, most recently from 7033bfa to 081a3f8 Compare December 25, 2017 08:14

tchaikov reviewed Dec 25, 2017

View reviewed changes

tchaikov reviewed Dec 28, 2017

View reviewed changes

bi-shun force-pushed the brotli-compressor branch from 8a67338 to f03292e Compare December 28, 2017 12:40

bi-shun force-pushed the brotli-compressor branch 3 times, most recently from f4b09f3 to db1b72f Compare December 28, 2017 13:38

tchaikov reviewed Dec 28, 2017

View reviewed changes

bi-shun force-pushed the brotli-compressor branch 4 times, most recently from 1c3e2f1 to 7a4dd35 Compare December 28, 2017 21:37

bi-shun force-pushed the brotli-compressor branch from 7a4dd35 to a496ad7 Compare December 29, 2017 06:48

tchaikov approved these changes Dec 29, 2017

View reviewed changes

tchaikov added feature needs-qa and removed needs-review labels Dec 29, 2017

Compressor: Add Brotli Compressor

f0aa226

Signed-off-by: BI SHUN KE <aionshun@livemail.tw>

bi-shun force-pushed the brotli-compressor branch from a496ad7 to f0aa226 Compare December 29, 2017 10:23

liewegas changed the title ~~Compressor: Add Brotli Compressor~~ compressor: Add Brotli Compressor Dec 29, 2017

yuriw added the wip-yuri4-testing label Jan 2, 2018

yuriw merged commit 392314d into ceph:master Jan 6, 2018

bi-shun deleted the brotli-compressor branch January 12, 2018 10:10

		@@ -719,6 +719,7 @@ add_subdirectory(include)
		add_subdirectory(librados)
		add_subdirectory(libradosstriper)

compressor: Add Brotli Compressor #19549

compressor: Add Brotli Compressor #19549

Conversation

bi-shun commented Dec 15, 2017

Choose a reason for hiding this comment

bi-shun commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bi-shun commented Dec 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bi-shun commented Dec 25, 2017

bi-shun commented Dec 28, 2017

Choose a reason for hiding this comment

tchaikov commented Dec 28, 2017

bi-shun commented Dec 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bi-shun Dec 28, 2017 • edited

Choose a reason for hiding this comment

tchaikov Dec 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bi-shun commented Dec 29, 2017

tchaikov left a comment • edited

Choose a reason for hiding this comment

tchaikov Dec 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Dec 29, 2017

bi-shun commented Dec 29, 2017

yuriw commented Jan 2, 2018

bi-shun Dec 28, 2017 •

edited

tchaikov Dec 29, 2017 •

edited

tchaikov left a comment •

edited

tchaikov Dec 29, 2017 •

edited