Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/8150 )
Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression ...................................................................... Patch Set 2: (8 comments) Didn't see the test class, but got some comments on the logic. http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc File be/src/service/fe-support.cc: http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@450 PS2, Line 450: // whether to compress or decompress the 'src' into 'dst'. May be worth commenting about GetPrimitiveArrayCritical() and lifecycle of the buffer. Looks like it is up to the JVM to GC it eventually. http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@451 PS2, Line 451: extern "C" Is extern required for this method? Also, no return type? http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@453 PS2, Line 453: jbyteArray dst, jint dst_off, jint dst_len, jboolean compress) { DCHECK_GE(dst_len, LZ4_compressBound(src_size))? http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@454 PS2, Line 454: 0 nit: Not sure if this was intentional, but you are passing a nullptr argument. JDK can handle it ok [1], but looks like you need to pass a pointer to 0/1. Also, we should probably make sure the (isCopy==false) (after the call), incase the JVM does a copy for some reason, we should free the copied native bytes. [1] http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/87ee5ee27509/src/share/vm/prims/jni.cpp#l4244 http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@455 PS2, Line 455: if (src_data == nullptr) return -1; Do we still need to call ReleasePrimitiveArrayCritical() in this case? Also, I see that you implemented a ScopedArrayCritical class to do that (much cleaner). Wondering why you've undone the changes? (same below) http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java File fe/src/main/java/org/apache/impala/service/FeSupport.java: http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java@285 PS2, Line 285: */ Probably worth commenting about the format of the output buffer. http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java@289 PS2, Line 289: if (dstLen <= 0) { : throw new InternalException( : "Payload is too big for LZ4 compression: " + payload.length); : } Move this below L294? http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java@306 PS2, Line 306: ByteBuffer.wrap(dst).putInt(0, payload.length); : ByteBuffer.wrap(dst).putInt(4, compressedSize); Can't we return the resized buffer directly here? Basically merge the logic of Lz4CompressToByteBuffer into this method? -- To view, visit http://gerrit.cloudera.org:8080/8150 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e Gerrit-Change-Number: 8150 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Thu, 28 Sep 2017 20:51:44 +0000 Gerrit-HasComments: Yes
