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

[serguei.spitsyn at oracle.com]

Hi David,


On 11/14/18 15:05, David Holmes wrote:
> Hi Serguei,
>
> On 15/11/2018 5:20 am, serguei.spitsyn at oracle.com wrote:
>> 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(); } 
>
> That's exactly why I suggested checking for can_call_Java() - because 
> it would work with JVMCI/Graal.

Does it mean, we want to continue posting such events on the JVMCI/Graal 
compiler threads executing Java?


>> 2. It is better to use the is_hidden_from_external_view() instead of 
>> !can_call_java() 
>
> Why? In terms of being conservative that would be unnecessarily 
> conservative. If the problem is only executing Java code why go beyond 
> that?

   The is_hidden_from_external_view() means the same as !can_call_java() 
CompilerThread's:
   // Hide native compiler threads from external view.
   bool is_hidden_from_external_view() const      { return 
!can_call_java(); }

   It seems the ServiceThread and CodeCacheSweeperThread do not follow 
the above rule.
   But do we want to post any events on the hidden threads?

>
> BTW what exactly is the set of hidden threads these days?

 From my observation the following threads are hidden now:
   ServiceThread
   CodeCacheSweeperThread
   All native CompilerThread's that return false from can_call_java().


Thanks,
Serguei

>> 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 || !
>
> Agreed - Thread::current() can not be NULL in this context.
>
> Cheers,
> David
>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 11/14/18 07:06, Daniel D. Daugherty wrote:
>>> Adding serviceability-dev at ...
>>>
>>> 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
>>>
>>