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

[Daniel D. Daugherty]

On 11/25/18 6:57 AM, David Holmes wrote:
>
>
> On 25/11/2018 12:41 am, Daniel D. Daugherty wrote:
>> On 11/22/18 5:24 PM, David Holmes wrote:
>>> Hi Thomas,
>>>
>>> On 23/11/2018 2:16 am, Thomas Stüfe wrote:
>>>> Hi all,
>>>>
>>>> would such a patch be acceptable:
>>>>
>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8213834-jvmti-reourceexhausted-shall-not-be-posted-from-compiler-thread/webrev.01/webrev/ 
>>>>
>>>> ?
>>>>
>>>> As has been proposed, I am now using
>>>> thread->is_hidden_from_external_view() to suppress the event.
>>>
>>> I still disagree with this for reason previously outlined. It not 
>>> only excludes the CompilerThreads that can't run Java but all 
>>> CompilerThreads, and so excludes JVMCI compiler threads that do 
>>> everything in Java and so will quite possibly trigger resource 
>>> exhaustion.
>>
>> I thought the CompilerThread is_hidden_from_external_view() is built on
>> top of can_call_java():
>>
>>    // Hide native compiler threads from external view.
>>    bool is_hidden_from_external_view() const      { return 
>> !can_call_java(); }
>>
>> so is_hidden_from_external_view() does not exclude JVMCI...
>
> Yes sorry my mistake.
>
>> Of course, the question of whether JVMCI threads should be participating
>> in JVM/TI events and other stuff is being debugged and explored at the
>> current time.
>>
>>
>>> It also excludes the ServiceThread and CodeCacheSweeperThread. While 
>>> the latter seems insignificant I think the ServiceThread is more 
>>> significant. Those threads are not excluded from posting other 
>>> events AFAICS so I don't see why this event should be treated 
>>> specially for them.
>>
>> Hmmm... I don't think the ServiceThread should be posting JVM/TI events
>
> But it already does! It's already posting deferred JVM TI events 
> explicitly as part of its job, and I see nothing that excludes it from 
> posting JVM TI events as part of any other Java code it runs.

You're right. I completely forgot about the ServiceThread doing proxy
event posting duty. So I think where we are is switching back to using

     can_call_java()

to restrict the posting of ResourceExhausted events. Less limiting than
!is_hidden_from_external_view(), but still limited. I'm reluctantly okay
with this, but we need to hear from Serguei.

Dan


>
> David
> -----
>
>> either. We definitely don't want the ServiceThread running "arbitrary"
>> event handler code since taking down the ServiceThread could mess up
>> various things that rely on it running.
>>
>> I don't have an opinion about the CodeCacheSweeperThread.
>>
>> dan
>>
>>
>>
>>>
>>>> Side note, this function apparently should only ever be called from
>>>> JavaThreads? To my knowledge we do not guard metaspace against
>>>> allocation from non-java-threads, should we then do that?
>>>
>>> Not sure how it is arranged but certainly Metaspace::allocate 
>>> expects to only be called from a JavaThread as it immediately checks 
>>> for
>>>
>>>  if (HAS_PENDING_EXCEPTION) {
>>>
>>> Cheers,
>>> David
>>> -----
>>>
>>>> I attempted to create a jtreg test for this but this turns out to 
>>>> be difficult:
>>>>
>>>> Attempting to trigger a metaspace OOM from a compiler thread proved
>>>> futile - chances for that to happen are just too low compared to other
>>>> metaspace users. Only way to do this reliably would be to artificially
>>>> allocate a lot of metaspace from the compiler thread - triggered by a
>>>> test switch or similar? That would be very ugly though. And then, I
>>>> would need to add a jvmti agent to jtreg, or adapt
>>>> jtreg/vmTestbase/nsk/jvmti?
>>>>
>>>> Thanks, Thomas
>>>> On Thu, Nov 22, 2018 at 4:22 PM Daniel D. Daugherty
>>>> <daniel.daugherty at oracle.com> wrote:
>>>>>
>>>>> On 11/22/18 2:42 AM, David Holmes wrote:
>>>>>>
>>>>>>
>>>>>> On 22/11/2018 5:36 pm, Thomas Stüfe wrote:
>>>>>>> Hi JC,
>>>>>>>
>>>>>>> On Wed, Nov 21, 2018 at 6:03 PM JC Beyler <jcbeyler at google.com> 
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Hi Thomas,
>>>>>>>>
>>>>>>> <snip>
>>>>>>>>
>>>>>>>> I actually agree with not using the service thread to be honest,
>>>>>>>> resource exhaustion seems to be something you'd want to know 
>>>>>>>> sooner
>>>>>>>> than later.
>>>>>>>>
>>>>>>>> How about we do both?
>>>>>>>>     - Fix it now so that the compiler thread does not post the 
>>>>>>>> event
>>>>>>>> because we'd rather not have crashes and that can get backported
>>>>>>>>     - Put out a RFE that would add the information to 
>>>>>>>> ThreadInfo and
>>>>>>>> work through the process of a CSR/etc. to get it working for 
>>>>>>>> future
>>>>>>>> versions?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jc
>>>>>>>>
>>>>>>>
>>>>>>> Makes sense, sure. But both Dan and Serguei voiced opposition to 
>>>>>>> this
>>>>>>> patch. Dan, Serguei, what do you think?
>>>>>>
>>>>>> I note neither Dan nor Serguei responded to my response to them :)
>>>>>
>>>>> I didn't think a response from me was needed. The last thing I 
>>>>> said was:
>>>>>
>>>>>> I would have no problem suppressing the event from a "hidden" thread
>>>>>> because those threads intentionally try to hide from JVM/TI. That 
>>>>>> would
>>>>>> cover the case for the C1 and C2 CompilerThreads, but I don't know
>>>>>> about these newer JVM/CI Compiler threads that actually run Java 
>>>>>> code...
>>>>>
>>>>> I read your combined response to this as not in conflict with what 
>>>>> I said.
>>>>>
>>>>> The last line of your response:
>>>>>
>>>>>   > So my preferred point solution is still to skip posting 
>>>>> ResourceExhausted
>>>>>   > if the thread can not run Java code.
>>>>>
>>>>> matches what I said since the C1 and C2 compiler threads are 
>>>>> hidden and
>>>>> cannot run Java code. We're in agreement.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>>
>>>>>>> If we do not find an agreement, I would rather close this bug as 
>>>>>>> WNF
>>>>>>> than push it in without consensus. I would then just fix it 
>>>>>>> downstream
>>>>>>> in our port.
>>>>>>>
>>>>>>> Your proposed RFE would still make sense, but not in jdk12 anymore,
>>>>>>> let alone in older releases.
>>>>>>>
>>>>>>> Thanks, Thomas
>>>>>>>
>>>>>>
>>>>>
>>