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

[David Holmes]

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.

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