Lucene qat support only by ZJie1 · Pull Request #11 · Intel-bigdata/IntelQATCodec

ZJie1 · 2020-02-28T09:38:43Z

No description provided.

winningsix

A few comments left.

winningsix · 2020-02-28T14:24:47Z

lucene_qat_wrapper/lucene_with_qat.diff

@@ -0,0 +1,588 @@
+diff --git a/lucene/build.xml b/lucene/build.xml


This file is intended for Lucence side to apply. We should provide version info to it. You can do it by creating related version number as folder name like what we did for Parquet/Hive.

winningsix · 2020-02-28T14:26:15Z

lucene_qat_wrapper/lucene_with_qat.diff

+  public QatCompressionCompressingCodec() {
+    // we don't worry about zlib block overhead as it's
+    // not bad and try to save space instead:
+    this(61440, 512, false, 1024);


Please avoid magic number here.

winningsix · 2020-02-28T14:30:00Z

lucene_qat_wrapper/lucene_with_qat.diff

+          bytes.length = decompressor.decompress(bytes.bytes, bytes.length, originalLength);
+        } catch (Error e) {
+          String s = e.getMessage();
+          System.out.println(e.getMessage());


Log instead of sys out.

winningsix · 2020-02-28T14:30:49Z

lucene_qat_wrapper/lucene_with_qat.diff

+      final int paddedLength = compressedLength;
+      compressed = ArrayUtil.grow(compressed, paddedLength + 1);
+      in.readBytes(compressed, 0, compressedLength);
+      compressed[compressedLength] = 0; // explicitly set dummy byte to 0


Why do we need to do Padding here? Could you refer a link?

winningsix · 2020-02-28T14:31:43Z

lucene_qat_wrapper/lucene_with_qat.diff

+      // pad with extra "dummy byte": see javadocs for using Inflater(true)
+      // we do it for compliance, but it's unnecessary for years in zlib.
+      final int paddedLength = compressedLength;
+      compressed = ArrayUtil.grow(compressed, paddedLength + 1);


Resizing is very expensive operation for array. What's the main purpose to hold the compressed as a local variable? We can just create it on the fly.

winningsix · 2020-02-28T14:35:42Z

lucene_qat_wrapper/lucene_with_qat.diff

+      compressor.finish();
+
+      int totalCount = 0;
+      for (; ; ) {


You can use while loop with !compressor.finished as the condition.

winningsix · 2020-02-28T14:36:57Z

lucene_qat_wrapper/lucene_with_qat.diff

+      for (; ; ) {
+        final int count = compressor.compress(compressed, totalCount, compressed.length - totalCount);
+        totalCount += count;
+        assert totalCount <= compressed.length;


Why do we need this assert? What's case for compressed array length bigger than total count?

winningsix · 2020-03-14T06:12:18Z

lucene_qat_wrapper/pom.xml

+    <maven-source-plugin.version>3.0.1</maven-source-plugin.version>
+    <maven-clean-plugin.version>3.0.0</maven-clean-plugin.version>
+    <maven-javadoc-plugin.version>2.10.4</maven-javadoc-plugin.version>
+    <!--<maven-assembly-plugin.version>3.0.0</maven-assembly-plugin.version>-20190909 by zj-->


Why do we need to exclude this maven plugin?

winningsix · 2020-03-14T06:13:10Z

lucene_qat_wrapper/src/main/java/com/intel/qat/jni/QatCompressorJNI.java

+import java.nio.ByteBuffer;
+
+
+/**


Remove those comments.

winningsix · 2020-03-14T06:14:23Z

lucene_qat_wrapper/src/main/java/com/intel/qat/jni/QatCompressorJNI.java

+        }
+    }
+
+    private int directBufferSize;


Move those parameters declarations before L49.

winningsix · 2020-03-14T08:04:27Z

lucene_qat_wrapper/src/main/java/com/intel/qat/util/QatNativeCodeLoader.java

+      // Ignore failure to load
+      if(LOG.isDebugEnabled()) {
+        LOG.debug("Failed to load native-qat with error: " + t);
+        LOG.debug("java.library.path=" +


Why need to output this msg here?

winningsix · 2020-03-14T08:05:01Z

lucene_qat_wrapper/src/main/java/com/intel/qat/util/QatNativeCodeLoader.java

+      nativeCodeLoaded = true;
+    } catch (Throwable t) {
+      // Ignore failure to load
+      if(LOG.isDebugEnabled()) {


Remove this condition since it's already debug level.

winningsix · 2020-03-14T08:07:45Z

lucene_qat_wrapper/src/main/java/com/intel/qat/jni/QatCompressorJNI.java

+     * Closes the compressor and discards any unprocessed input.
+     */
+
+    public synchronized void end() {


Please throw unsupport operation exception here.

winningsix · 2020-03-14T08:09:56Z

lucene_qat_wrapper/src/main/java/com/intel/qat/jni/QatCompressorJNI.java

+     * @param len Length
+     */
+
+    public synchronized void setInput(byte[] b, int off, int len) {


Why do we need to make it thread safe? I am thinking it's initialized and will not be reused.

winningsix · 2020-03-14T08:18:39Z

lucene_qat_wrapper/src/main/java/com/intel/qat/jni/QatDecompressorJNI.java

+                uncompressedDirectBuf = ByteBuffer.allocateDirect(directBufferSize);
+            }
+            try {
+                compressedDirectBuf = (ByteBuffer) nativeAllocateBB(directBufferSize,


do we need to use two separated buffer? Is it possible to reuse the compressor in the same time for both de/compression?

winningsix · 2020-03-14T08:20:55Z

lucene_qat_wrapper/src/main/java/com/intel/qat/jni/QatDecompressorJNI.java

+    private Buffer uncompressedDirectBuf = null;
+    private byte[] userBuf = null;
+    private int userBufOff = 0, userBufLen = 0;
+    private boolean finished;


If this is thread-safe, we should make those variables volatile.

winningsix · 2020-03-14T08:34:36Z

lucene_qat_wrapper/src/main/java/com/intel/qat/jni/QatDecompressorJNI.java

+     * @param forcePinned
+     * @param numa
+     */
+    public QatDecompressorJNI(int directBufferSize, boolean useNativeAllocateBB,


We should avoid put too many logics into JNI class. It make it hard to reuse the code.

winningsix · 2020-04-01T07:16:36Z

lucene_qat_wrapper/src/main/java/com/intel/qat/jni/QatCompressorJNI.java


    private static final Logger LOG = LogManager.getLogger(QatCompressorJNI.class);
-    private static final int DEFAULT_DIRECT_BUFFER_SIZE = 640 * 1024;
+    private static final int DEFAULT_DIRECT_BUFFER_SIZE = 64 * 1024;


Did it work in our end-2-end test?

ZJie1 added 9 commits December 6, 2019 17:03

[POAE7-11](add lucene-qat-wrapper,ZhangJie, Reviewed by XuCheng)

0780cba

disable the log and change the end in Decompressor

190e8ad

enable log

a564be2

add the log file to test the content in compressed bytes

de17216

modified the QatDecompressorJNI.c to enable the twice output to the file

e084ce8

the logs for solving error -2

a9e4318

solved all bugs about qat compressing/decompressing

4994bdb

[POAE7-11](finished clean code in lucene_qat_wrapper ,ZhangJie)

0fadd9d

add patch of lucene that added qat

9897406

winningsix suggested changes Feb 28, 2020

View reviewed changes

ZJie1 added 2 commits March 12, 2020 14:05

[POAE7-11](change to using logging.log4j ,ZhangJie)

4ece160

add patch for lucene that added the version infos

cf00286

winningsix suggested changes Mar 14, 2020

View reviewed changes

ZJie1 added 2 commits March 28, 2020 00:22

small changes according comments

0e8abee

change directBuffSize to solve Full GC

bc68800

winningsix reviewed Apr 1, 2020

View reviewed changes

		@@ -0,0 +1,588 @@
		diff --git a/lucene/build.xml b/lucene/build.xml

		import java.nio.ByteBuffer;


		/**

Conversation

ZJie1 commented Feb 28, 2020

Uh oh!

winningsix 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants