[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Philip Zeyliger has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12660 Change subject: IMPALA-8250: Clean up JNI warnings. .. IMPALA-8250: Clean up JNI warnings. Using HDFS_OPTS+="-Xcheck:jni" revealed a handful of warnings related to (a) checking for exceptions and (b) leaking local references. Checking for exceptions required sprinkling RETURN_ERROR_IF_EXC left and right. I chose not to expand the JniCall infrastructure to handle this more generally at the moment. The leaky local references are a bit harder. In the logs, they show up as "WARNING: JNI local refs: 2597, exceeds capacity: 35" or similar. A few of these errors seem to be not in our code. The ones that I've found in our code stemmed from HBaseTableScanner::GetRowKey(): this method uses local references and wasn't returning them. Using a JniLocalFrame seems to have taken care of the warnings. I have added code to skip test_large_strings when JNI checking is enabled. This test takes forever (presumably because JNI is checking bounds on strings very aggressively), and times out. The time out also causes some metric-related checks to fail (since a query is still in flight). Debugging this required customizing my JDK to give stack traces when these warnings occurred. The following diff facilitated this. diff -r 76a9c9cf14f1 src/share/vm/prims/jniCheck.cpp --- a/src/share/vm/prims/jniCheck.cpp Tue Jan 15 10:43:31 2019 + +++ b/src/share/vm/prims/jniCheck.cpp Wed Feb 27 11:57:13 2019 -0800 @@ -143,11 +143,30 @@ static const char * fatal_instance_field_mismatch = "Field type (instance) mismatch in JNI get/set field operations"; static const char * fatal_non_string = "JNI string operation received a non-string"; +// thisone: whether to print every time, or maybe, depending on future +// how many future stacks we want printed (totally racy); helps catch +// missing exception handling if there's a way to tickle that code +// reliably. +static inline void dump_native_stack(JavaThread* thr, bool thisone, int future) { + static int fut_stacks = 0; // racy! + if (fut_stacks > 0) { +thisone = true; +fut_stacks--; + } + if (future > 0) fut_stacks = future; + if (thisone) { +frame fr = os::current_frame(); +char buf[6000]; +tty->print_cr("Thread: %s %d", thr->get_thread_name(), thr->osthread()->thread_id()); +print_native_stack(tty, fr, thr, buf, sizeof(buf)); + } +} // When in VM state: static void ReportJNIWarning(JavaThread* thr, const char *msg) { tty->print_cr("WARNING in native method: %s", msg); thr->print_stack(); + dump_native_stack(thr, true, 0); } // When in NATIVE state: @@ -199,11 +218,14 @@ tty->print_cr("WARNING in native method: JNI call made without checking exceptions when required to from %s", thr->get_pending_jni_exception_check()); thr->print_stack(); + dump_native_stack(thr, true, 10); ) thr->clear_pending_jni_exception_check(); // Just complain once } } + + /** * Add to the planned number of handles. I.e. plus current live & warning threshold */ @@ -254,9 +276,12 @@ tty->print_cr("WARNING: JNI local refs: %zu, exceeds capacity: %zu", live_handles, planned_capacity); thr->print_stack(); + dump_native_stack(thr, true, 0); ) // Complain just the once, reset to current + warn threshold add_planned_handle_capacity(handles, 0); + } else { +dump_native_stack(thr, false, 0); } } Change-Id: Idd1709f749a764c1d947704bc64306493863b45f --- M be/src/catalog/catalog.cc M be/src/exec/hbase-table-scanner.cc M be/src/exprs/hive-udf-call.cc M be/src/runtime/hbase-table-factory.cc M be/src/service/frontend.cc M tests/query_test/test_insert.py 6 files changed, 43 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12660/1 -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2348/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 05 Mar 2019 02:07:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 1: (3 comments) I'm seeing a lot of places in the HBase scanner where we call GetByteArrayRegion or GetObjectArrayElement without checking for exceptions (these can both throw ArrayIndexOutOfBoundsExceptions). I'm not sure why -Xcheck:jni wouldn't detect these, but perhaps be should do a scrub of the rest of the Impala code to make sure we are checking for exceptions after each array call (there are several other array-based JNI ops that throw ArrayIndexOutOfBoundsException - https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html). We can do that in a different patch though since it might be a larger change. http://gerrit.cloudera.org:8080/#/c/12660/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12660/1//COMMIT_MSG@9 PS1, Line 9: Using HDFS_OPTS+="-Xcheck:jni" revealed a handful of warnings related to It's LIBHDFS_OPTS right? http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/runtime/hbase-table-factory.cc File be/src/runtime/hbase-table-factory.cc: http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/runtime/hbase-table-factory.cc@97 PS1, Line 97: if (!s.ok()) LOG(INFO) << "Exception when cleaning up HBase" << s; nit: space after HBase http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/service/frontend.cc@116 PS1, Line 116: ABORT_IF_ERROR(JniUtil::LoadJniMethod(jni_env, fe_class_, &(methods[i]))); Might be understanding the code wrong, but shouldn't these be global refs since they are used outside the scope of this method? -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 05 Mar 2019 17:53:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 1: (3 comments) > (3 comments) > > I'm seeing a lot of places in the HBase scanner where we call > GetByteArrayRegion or GetObjectArrayElement without checking for > exceptions (these can both throw ArrayIndexOutOfBoundsExceptions). > I'm not sure why -Xcheck:jni wouldn't detect these, but perhaps be > should do a scrub of the rest of the Impala code to make sure we > are checking for exceptions after each array call (there are > several other array-based JNI ops that throw ArrayIndexOutOfBoundsException > - > https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html). > We can do that in a different patch though since it might be a > larger change. Yeah: this is surprising. I read the jniCheck.cpp code and it should be handling the GetObjectArray... methods. The most likely scenario is that our tests don't have coverage here. Or jniCheck isn't doing its business. I'm going to sprinkle in some of these where it's obvious. I've kicked off a suite of our tests with code coverage on. I'm also going to kick off a set of tests with the JNI checking and 'exhaustive' tests. Maybe there's coverage, but we're not running the queries against HBase. This discussion (from jniCheck.cpp) is also interesting. I didn't see the code that didn't warn about ArrayIndexOutOfBounds, but maybe that's what's happening. /** * Check whether or not a programmer has actually checked for exceptions. According * to the JNI Specification ("jni/spec/design.html#java_exceptions"): * * There are two cases where the programmer needs to check for exceptions without * being able to first check an error code: * * - The JNI functions that invoke a Java method return the result of the Java method. * The programmer must call ExceptionOccurred() to check for possible exceptions * that occurred during the execution of the Java method. * * - Some of the JNI array access functions do not return an error code, but may * throw an ArrayIndexOutOfBoundsException or ArrayStoreException. * * In all other cases, a non-error return value guarantees that no exceptions have been thrown. * * Programmers often defend against ArrayIndexOutOfBoundsException, so warning * for these functions would be pedantic. */ http://gerrit.cloudera.org:8080/#/c/12660/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12660/1//COMMIT_MSG@9 PS1, Line 9: Using HDFS_OPTS+="-Xcheck:jni" revealed a handful of warnings related to > It's LIBHDFS_OPTS right? Done http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/runtime/hbase-table-factory.cc File be/src/runtime/hbase-table-factory.cc: http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/runtime/hbase-table-factory.cc@97 PS1, Line 97: if (!s.ok()) LOG(INFO) << "Exception when cleaning up HBase" << s; > nit: space after HBase Done http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/12660/1/be/src/service/frontend.cc@116 PS1, Line 116: ABORT_IF_ERROR(JniUtil::LoadJniMethod(jni_env, fe_class_, &(methods[i]))); > Might be understanding the code wrong, but shouldn't these be global refs s Based on https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#accessing_fields_and_methods , https://stackoverflow.com/questions/2093112/why-i-should-not-reuse-a-jclass-and-or-jmethodid-in-jni and http://www.latkin.org/blog/2016/02/01/jni-object-lifetimes-quick-reference/ (very nice!) and https://developer.android.com/training/articles/perf-jni.html#local_and_global_references say that jmethodid (which is the type here) doesn't need global references, but is invalidated if the class gets unloaded. Fortunately, the class is kept alive because fe_ stores it. I thought fe_class_ had the problem you suggest (it's a local reference), but it turns out that it's not used outside this method. I removed it from the class to make that obvious. -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 05 Mar 2019 20:34:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Hello Sahil Takiar, Todd Lipcon, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12660 to look at the new patch set (#2). Change subject: IMPALA-8250: Clean up JNI warnings. .. IMPALA-8250: Clean up JNI warnings. Using LIBHDFS_OPTS+="-Xcheck:jni" revealed a handful of warnings related to (a) checking for exceptions and (b) leaking local references. Checking for exceptions required sprinkling RETURN_ERROR_IF_EXC left and right. I chose not to expand the JniCall infrastructure to handle this more generally at the moment. The leaky local references are a bit harder. In the logs, they show up as "WARNING: JNI local refs: 2597, exceeds capacity: 35" or similar. A few of these errors seem to be not in our code. The ones that I've found in our code stemmed from HBaseTableScanner::GetRowKey(): this method uses local references and wasn't returning them. Using a JniLocalFrame seems to have taken care of the warnings. I have added code to skip test_large_strings when JNI checking is enabled. This test takes forever (presumably because JNI is checking bounds on strings very aggressively), and times out. The time out also causes some metric-related checks to fail (since a query is still in flight). Debugging this required customizing my JDK to give stack traces when these warnings occurred. The following diff facilitated this. diff -r 76a9c9cf14f1 src/share/vm/prims/jniCheck.cpp --- a/src/share/vm/prims/jniCheck.cpp Tue Jan 15 10:43:31 2019 + +++ b/src/share/vm/prims/jniCheck.cpp Wed Feb 27 11:57:13 2019 -0800 @@ -143,11 +143,30 @@ static const char * fatal_instance_field_mismatch = "Field type (instance) mismatch in JNI get/set field operations"; static const char * fatal_non_string = "JNI string operation received a non-string"; +// thisone: whether to print every time, or maybe, depending on future +// how many future stacks we want printed (totally racy); helps catch +// missing exception handling if there's a way to tickle that code +// reliably. +static inline void dump_native_stack(JavaThread* thr, bool thisone, int future) { + static int fut_stacks = 0; // racy! + if (fut_stacks > 0) { +thisone = true; +fut_stacks--; + } + if (future > 0) fut_stacks = future; + if (thisone) { +frame fr = os::current_frame(); +char buf[6000]; +tty->print_cr("Thread: %s %d", thr->get_thread_name(), thr->osthread()->thread_id()); +print_native_stack(tty, fr, thr, buf, sizeof(buf)); + } +} // When in VM state: static void ReportJNIWarning(JavaThread* thr, const char *msg) { tty->print_cr("WARNING in native method: %s", msg); thr->print_stack(); + dump_native_stack(thr, true, 0); } // When in NATIVE state: @@ -199,11 +218,14 @@ tty->print_cr("WARNING in native method: JNI call made without checking exceptions when required to from %s", thr->get_pending_jni_exception_check()); thr->print_stack(); + dump_native_stack(thr, true, 10); ) thr->clear_pending_jni_exception_check(); // Just complain once } } + + /** * Add to the planned number of handles. I.e. plus current live & warning threshold */ @@ -254,9 +276,12 @@ tty->print_cr("WARNING: JNI local refs: %zu, exceeds capacity: %zu", live_handles, planned_capacity); thr->print_stack(); + dump_native_stack(thr, true, 0); ) // Complain just the once, reset to current + warn threshold add_planned_handle_capacity(handles, 0); + } else { +dump_native_stack(thr, false, 0); } } Change-Id: Idd1709f749a764c1d947704bc64306493863b45f --- M be/src/catalog/catalog.cc M be/src/exec/hbase-table-scanner.cc M be/src/exprs/hive-udf-call.cc M be/src/runtime/hbase-table-factory.cc M be/src/service/frontend.cc M be/src/service/frontend.h M tests/query_test/test_insert.py 7 files changed, 56 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12660/2 -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2359/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 05 Mar 2019 21:19:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/12660/2/be/src/exec/hbase-table-scanner.cc File be/src/exec/hbase-table-scanner.cc: http://gerrit.cloudera.org:8080/#/c/12660/2/be/src/exec/hbase-table-scanner.cc@562 PS2, Line 562: RETURN_ERROR_IF_EXC(env); I think GetArrayLength does not throw an exception, so this check is probably unnecessary. -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 05 Mar 2019 22:46:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Hello Sahil Takiar, Todd Lipcon, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12660 to look at the new patch set (#3). Change subject: IMPALA-8250: Clean up JNI warnings. .. IMPALA-8250: Clean up JNI warnings. Using LIBHDFS_OPTS+="-Xcheck:jni" revealed a handful of warnings related to (a) checking for exceptions and (b) leaking local references. Checking for exceptions required sprinkling RETURN_ERROR_IF_EXC left and right. I chose not to expand the JniCall infrastructure to handle this more generally at the moment. The leaky local references are a bit harder. In the logs, they show up as "WARNING: JNI local refs: 2597, exceeds capacity: 35" or similar. A few of these errors seem to be not in our code. The ones that I've found in our code stemmed from HBaseTableScanner::GetRowKey(): this method uses local references and wasn't returning them. Using a JniLocalFrame seems to have taken care of the warnings. I have added code to skip test_large_strings when JNI checking is enabled. This test takes forever (presumably because JNI is checking bounds on strings very aggressively), and times out. The time out also causes some metric-related checks to fail (since a query is still in flight). Debugging this required customizing my JDK to give stack traces when these warnings occurred. The following diff facilitated this. diff -r 76a9c9cf14f1 src/share/vm/prims/jniCheck.cpp --- a/src/share/vm/prims/jniCheck.cpp Tue Jan 15 10:43:31 2019 + +++ b/src/share/vm/prims/jniCheck.cpp Wed Feb 27 11:57:13 2019 -0800 @@ -143,11 +143,30 @@ static const char * fatal_instance_field_mismatch = "Field type (instance) mismatch in JNI get/set field operations"; static const char * fatal_non_string = "JNI string operation received a non-string"; +// thisone: whether to print every time, or maybe, depending on future +// how many future stacks we want printed (totally racy); helps catch +// missing exception handling if there's a way to tickle that code +// reliably. +static inline void dump_native_stack(JavaThread* thr, bool thisone, int future) { + static int fut_stacks = 0; // racy! + if (fut_stacks > 0) { +thisone = true; +fut_stacks--; + } + if (future > 0) fut_stacks = future; + if (thisone) { +frame fr = os::current_frame(); +char buf[6000]; +tty->print_cr("Thread: %s %d", thr->get_thread_name(), thr->osthread()->thread_id()); +print_native_stack(tty, fr, thr, buf, sizeof(buf)); + } +} // When in VM state: static void ReportJNIWarning(JavaThread* thr, const char *msg) { tty->print_cr("WARNING in native method: %s", msg); thr->print_stack(); + dump_native_stack(thr, true, 0); } // When in NATIVE state: @@ -199,11 +218,14 @@ tty->print_cr("WARNING in native method: JNI call made without checking exceptions when required to from %s", thr->get_pending_jni_exception_check()); thr->print_stack(); + dump_native_stack(thr, true, 10); ) thr->clear_pending_jni_exception_check(); // Just complain once } } + + /** * Add to the planned number of handles. I.e. plus current live & warning threshold */ @@ -254,9 +276,12 @@ tty->print_cr("WARNING: JNI local refs: %zu, exceeds capacity: %zu", live_handles, planned_capacity); thr->print_stack(); + dump_native_stack(thr, true, 0); ) // Complain just the once, reset to current + warn threshold add_planned_handle_capacity(handles, 0); + } else { +dump_native_stack(thr, false, 0); } } Change-Id: Idd1709f749a764c1d947704bc64306493863b45f --- M be/src/catalog/catalog.cc M be/src/exec/hbase-table-scanner.cc M be/src/exprs/hive-udf-call.cc M be/src/runtime/hbase-table-factory.cc M be/src/service/frontend.cc M be/src/service/frontend.h M tests/query_test/test_insert.py 7 files changed, 55 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/12660/3 -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 2: (1 comment) Yep, good catch. Thanks. http://gerrit.cloudera.org:8080/#/c/12660/2/be/src/exec/hbase-table-scanner.cc File be/src/exec/hbase-table-scanner.cc: http://gerrit.cloudera.org:8080/#/c/12660/2/be/src/exec/hbase-table-scanner.cc@562 PS2, Line 562: RETURN_ERROR_IF_EXC(env); > I think GetArrayLength does not throw an exception, so this check is probab Done -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 06 Mar 2019 18:13:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2375/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 06 Mar 2019 18:59:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 3: Code-Review+1 "I've kicked off a suite of our tests with code coverage on. I'm also going to kick off a set of tests with the JNI checking and 'exhaustive' tests. Maybe there's coverage, but we're not running the queries against HBase." Did anything new come up in this? Patch LGTM. -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 07 Mar 2019 18:47:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 3: > "I've kicked off a suite of our tests with code coverage on. I'm > also going to kick off a set of tests with the JNI checking and > 'exhaustive' tests. Maybe there's coverage, but we're not running > the queries against HBase." > > Did anything new come up in this? > Nothing new. I'm still chasing a sequence of these calls which lead to CallObjectMethod() not having its exception checked... checked_jni_CallVoidMethodV checked_jni_ExceptionOccurred checked_jni_ExceptionClear checked_jni_GetObjectClass checked_jni_FindClass checked_jni_GetMethodID checked_jni_CallObjectMethod I can trigger this with set mem_limit=50m;use functional;insert into table alltypesinsert partition (year, month) /* +noclustered */ select at1.id, at1.bool_col, at1.tinyint_col, at1.smallint_col, at1.int_col, at1.bigint_col, at1.float_col, at1.double_col, at1.date_string_col, at1.string_col, at1.timestamp_col, at1.year, at2.id as month from functional.alltypes at1, functional.alltypes at2; when the sink gets closed after a failure. I don't know where this stack of calls is at this point. I'll keep looking, but I don't want to block on it. -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 07 Mar 2019 23:08:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3891/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 4 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 07 Mar 2019 23:09:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 4: Code-Review+2 Thanks! -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 4 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 07 Mar 2019 23:13:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. Patch Set 4: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 4 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 08 Mar 2019 03:35:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8250: Clean up JNI warnings.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12660 ) Change subject: IMPALA-8250: Clean up JNI warnings. .. IMPALA-8250: Clean up JNI warnings. Using LIBHDFS_OPTS+="-Xcheck:jni" revealed a handful of warnings related to (a) checking for exceptions and (b) leaking local references. Checking for exceptions required sprinkling RETURN_ERROR_IF_EXC left and right. I chose not to expand the JniCall infrastructure to handle this more generally at the moment. The leaky local references are a bit harder. In the logs, they show up as "WARNING: JNI local refs: 2597, exceeds capacity: 35" or similar. A few of these errors seem to be not in our code. The ones that I've found in our code stemmed from HBaseTableScanner::GetRowKey(): this method uses local references and wasn't returning them. Using a JniLocalFrame seems to have taken care of the warnings. I have added code to skip test_large_strings when JNI checking is enabled. This test takes forever (presumably because JNI is checking bounds on strings very aggressively), and times out. The time out also causes some metric-related checks to fail (since a query is still in flight). Debugging this required customizing my JDK to give stack traces when these warnings occurred. The following diff facilitated this. diff -r 76a9c9cf14f1 src/share/vm/prims/jniCheck.cpp --- a/src/share/vm/prims/jniCheck.cpp Tue Jan 15 10:43:31 2019 + +++ b/src/share/vm/prims/jniCheck.cpp Wed Feb 27 11:57:13 2019 -0800 @@ -143,11 +143,30 @@ static const char * fatal_instance_field_mismatch = "Field type (instance) mismatch in JNI get/set field operations"; static const char * fatal_non_string = "JNI string operation received a non-string"; +// thisone: whether to print every time, or maybe, depending on future +// how many future stacks we want printed (totally racy); helps catch +// missing exception handling if there's a way to tickle that code +// reliably. +static inline void dump_native_stack(JavaThread* thr, bool thisone, int future) { + static int fut_stacks = 0; // racy! + if (fut_stacks > 0) { +thisone = true; +fut_stacks--; + } + if (future > 0) fut_stacks = future; + if (thisone) { +frame fr = os::current_frame(); +char buf[6000]; +tty->print_cr("Thread: %s %d", thr->get_thread_name(), thr->osthread()->thread_id()); +print_native_stack(tty, fr, thr, buf, sizeof(buf)); + } +} // When in VM state: static void ReportJNIWarning(JavaThread* thr, const char *msg) { tty->print_cr("WARNING in native method: %s", msg); thr->print_stack(); + dump_native_stack(thr, true, 0); } // When in NATIVE state: @@ -199,11 +218,14 @@ tty->print_cr("WARNING in native method: JNI call made without checking exceptions when required to from %s", thr->get_pending_jni_exception_check()); thr->print_stack(); + dump_native_stack(thr, true, 10); ) thr->clear_pending_jni_exception_check(); // Just complain once } } + + /** * Add to the planned number of handles. I.e. plus current live & warning threshold */ @@ -254,9 +276,12 @@ tty->print_cr("WARNING: JNI local refs: %zu, exceeds capacity: %zu", live_handles, planned_capacity); thr->print_stack(); + dump_native_stack(thr, true, 0); ) // Complain just the once, reset to current + warn threshold add_planned_handle_capacity(handles, 0); + } else { +dump_native_stack(thr, false, 0); } } Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Reviewed-on: http://gerrit.cloudera.org:8080/12660 Reviewed-by: Joe McDonnell Tested-by: Impala Public Jenkins --- M be/src/catalog/catalog.cc M be/src/exec/hbase-table-scanner.cc M be/src/exprs/hive-udf-call.cc M be/src/runtime/hbase-table-factory.cc M be/src/service/frontend.cc M be/src/service/frontend.h M tests/query_test/test_insert.py 7 files changed, 55 insertions(+), 10 deletions(-) Approvals: Joe McDonnell: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Idd1709f749a764c1d947704bc64306493863b45f Gerrit-Change-Number: 12660 Gerrit-PatchSet: 5 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Todd Lipcon