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

[serguei.spitsyn at oracle.com]

On 11/25/18 19:58, serguei.spitsyn at oracle.com wrote:
> Hi guys,
>
> On 11/25/18 06:17, Daniel D. Daugherty wrote:
>> 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.
>
> There are many confusions around this.
> Unfortunately, I make the same mistakes. :(
>
>>>> 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.
>
> I've read all the discussion thread.
> Thank you to everyone participating in this discussion - it is very 
> helpful.
> Now, I'm convinced that using can_call_java (not 
> is_hidden_from_external_view)
> is right (thanks a lot to David for defending this point!)
>
> I still have a concern though.
> If we skip a ResourceExhausted event this way (as in the webrev above)
> then can we expect similar event posted on another (executing Java) 
> thread?
> I have a doubt that we skip it correctly (I need to learn the code more).

Please, skip my concern above.
I do not see any issues in the GC code.

Thanks,
Serguei


> Thanks,
> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>
>>
>