[
http://issues.apache.org/jira/browse/HADOOP-538?page=comments#action_12444950 ]
Sameer Paranjpye commented on HADOOP-538:
-----------------------------------------
Some more comments on the native code:
Makefiles etc.
- We should compile with '-Wall'.
- What about 64-bit vs 32-bit builds, this makefile will generate the OS
default, which may be
incompatible with the JVM used. For instance on a 64-bit machine with a 32-bit
JVM this will generate
a 64-bit library, which will be incompatible with the JVM. We should check the
JVM version and compile
with -m32 or -m64 as appropriate.
-----------
Index: src/native/org/apache/hadoop/io/compress/zlib/ZlibCompressor.c
+JNIEXPORT jlong JNICALL
+Java_org_apache_hadoop_io_compress_zlib_ZlibCompressor_init(
+ JNIEnv *env, jclass class, jint level, jint strategy
+ ) {
+ // Create a z_stream
+ z_stream *stream = malloc(sizeof(z_stream));
+ if (!stream) {
+ THROW(env, "java/lang/OutOfMemoryError", NULL);
+ return (jlong)0;
+ }
+ memset((void*)stream, 0, sizeof(z_stream));
...
...
...
+
+ return (jlong)((int)stream);
+}
+
Is this the only way to return a void* to the JVM? This is a bad, unsafe cast,
not 64-bit safe.
If a cast like this is needed it should be '(jlong)(stream)'.
+JNIEXPORT jint JNICALL
+Java_org_apache_hadoop_io_compress_zlib_ZlibCompressor_deflateBytesDirect(
+ JNIEnv *env, jobject this
+ ) {
+ // Get members of ZlibCompressor
+ z_stream *stream = (z_stream *)
+ ((int)(*env)->GetLongField(env, this,
ZlibCompressor_stream));
...
...
...
+}
Cast to int is not 64-bit safe.
-------------
Index: src/native/org/apache/hadoop/io/compress/zlib/ZlibDecompressor.c
+JNIEXPORT jlong JNICALL
+Java_org_apache_hadoop_io_compress_zlib_ZlibDecompressor_init(
+ JNIEnv *env, jclass cls
+ ) {
+ z_stream *stream = malloc(sizeof(z_stream));
+ memset((void*)stream, 0, sizeof(z_stream));
+
...
...
...
+ return (jint)((int)stream);
+}
Cast is not 64-bit safe.
-----------------
Index:
src/native/org/apache/hadoop/io/compress/zlib/org_apache_hadoop_io_compress_zlib_util.h
+// TODO: Fix the below macro for 64-bit systems
+// Helpful macro to convert a jlong to z_stream*
+#define ZSTREAM(stream) ((z_stream*)((int)stream))
Act on the comment, please. :)
-----------------
Index: src/native/org_apache_hadoop.h
+
+ #define THROW(env, exception_name, message) \
+ { \
+ jclass ecls = (*env)->FindClass(env,
exception_name); \
+ if (ecls) { \
+ (*env)->ThrowNew(env, ecls, message); \
+ } \
+ }
+
This macro creates a local reference to a class in jclass and does not destroy
it, causing a memory leak.
Add a '(*env)->DestroyLocalReference(env, ecls)' statement.
> Implement a nio's 'direct buffer' based wrapper over zlib to improve
> performance of java.util.zip.{De|In}flater as a 'custom codec'
> -----------------------------------------------------------------------------------------------------------------------------------
>
> Key: HADOOP-538
> URL: http://issues.apache.org/jira/browse/HADOOP-538
> Project: Hadoop
> Issue Type: Improvement
> Affects Versions: 0.6.1
> Reporter: Arun C Murthy
> Assigned To: Arun C Murthy
> Fix For: 0.8.0
>
> Attachments: HADOOP-538.patch, HADOOP-538_20061005.tgz,
> HADOOP-538_20061011.tgz, HADOOP-538_20061026.tgz, HADOOP-538_benchmarks.tgz
>
>
> There has been more than one instance where java.util.zip's {De|In}flater
> classes perform unreliably, a simple wrapper over zlib-1.2.3 (latest stable)
> using java.nio.ByteBuffer (i.e. direct buffers) should go a long way in
> alleviating these woes.
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
