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

[serguei.spitsyn at oracle.com]

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).


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