Re: RFR(xs): 8213834: JVMTI ResourceExhausted should not be posted in CompilerThread

[serguei.spit...@oracle.com]

Hi Thomas, Thank you for taking care about this issue! We recently also saw a couple of problems related to the compiler thread not being hidden. I have several comments to the fix. It would be really nice if there is any chance to have a unit test for this. But I understand that it is not easy to minimize what you have in your agent. http://cr.openjdk.java.net/~stuefe/webrevs/8213834-jvmti-reourceexhausted-shall-not-be-posted-from-compiler-thread/webrev.00/webrev/src/hotspot/share/prims/jvmtiExport.cpp.udiff.html void JvmtiExport::post_resource_exhausted(jint resource_exhausted_flags, const char* description) { + + // Handlers may call back into the JVM as a reaction to this event, e.g. to print out + // a class histogram. Most of those actions require the ability to run java though. So + // lets not post this event in cases where we cannot run java (e.g. when a CompilerThread + // triggers a Metaspace OOM). + Thread* thread = Thread::current_or_null(); + if (thread == NULL || !thread->can_call_java()) { + return; + } + 1. Did you check that your fix does also work with the Graal enabled? The CompilerThread::can_call_java() should return true for JVMCI is used: bool CompilerThread::can_call_java() const { return _compiler != NULL && _compiler->is_jvmci(); } 2. It is better to use the is_hidden_from_external_view() instead of !can_call_java() 3. The 'thread' is already defined in post_resource_exhausted: JavaThread *thread = JavaThread::current();   But it is in a deeper scope. void JvmtiExport::post_resource_exhausted(jint resource_exhausted_flags, const char* description) { EVT_TRIG_TRACE(JVMTI_EVENT_RESOURCE_EXHAUSTED, ("Trg resource exhausted event triggered" )); JvmtiEnvIterator it; for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) { if (env->is_enabled(JVMTI_EVENT_RESOURCE_EXHAUSTED)) { EVT_TRACE(JVMTI_EVENT_RESOURCE_EXHAUSTED, ("Evt resource exhausted event sent" )); JavaThread *thread = JavaThread::current(); I'd suggest to move it up as it is a better place for it anyway. Also, you do not need the extra check: if (thread == NULL || ! Thanks, Serguei On 11/14/18 07:06, Daniel D. Daugherty wrote: Adding serviceability-dev@... Since the proposed solution to the bug is to not post an event, I think the Serviceability Team (which owns JVM/TI) needs to be involved directly in the review. Dan On 11/14/18 9:28 AM, Thomas Stüfe wrote: Dear all, may I please have reviews on this small patch. Note that this is borderline serviceability. I try to avoid crossposting, but if you think this should be looked at by serviceability feel free to forward it there. Issue: https://bugs.openjdk.java.net/browse/JDK-8213834 CR: http://cr.openjdk.java.net/~stuefe/webrevs/8213834-jvmti-reourceexhausted-shall-not-be-posted-from-compiler-thread/webrev.00/webrev/ Short description: we may post ResourceExhausted from Compiler threads. Handlers of this event may call back into the JVM, which may cause problems since we run in the compiler thread. The proposed solution is not to post the event in that case. See previous discussion here: http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-November/025898.html Thanks, Thomas