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

Mon Nov 26 11:28:40 UTC 2018

On 26/11/2018 6:58 pm, Thomas Stüfe wrote:
> Hi all,
> 
> just a general question, would a
> 
> virtual bool Thread::can_post_JVMTI_events();
> 
> (similar to can_call_java()) not be a clearer solution? It clearly
> expresses what we want, and we can, in every Thread child class,
> explicitly specify the behavior.

That would be good for when we update the specification to actually 
define what threads can post JVM TI events. But I'd prefer for now to 
see just a comment and the can_call_java check.

David

> ..Thomas
> 
> 
> 
> 
> On Mon, Nov 26, 2018 at 9:51 AM Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>>
>> On Mon, Nov 26, 2018 at 4:58 AM serguei.spitsyn at oracle.com
>> <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 think this usually would happen, but there is no guarantee.
>>
>> Speaking for metaspace: Before metaspace OOM happens, we already tried
>> to GC and that failed to recover metaspace so there is no immediate
>> hope and chances are high subsequent metaspace allocations will fail
>> too.
>>
>> If the JIT runs into metaspace OOM, it will cope by bailing out and
>> leave that particular method uncompiled. I argue that this is not
>> observable to the user and therefore ResourceExhausted can be
>> suppressed.
>>
>> If we, as you fear, do not resend ResourceExhausted, that means that
>> no-one else tried to allocate metaspace, and that this JIT compilation
>> was by pure chance the last one to ever want metaspace. Unlikely as it
>> is, in that case I think it okay suppressing ResourceExhausted and
>> going on with our lives.
>>
>> But if Metaspace OOM keep occurring, they most probably will occur
>> from regular java threads too and that will trigger ResourceExhausted.
>>
>> Finally, I am not sure whether a pathological case is possible where
>> only the JIT were to allocate metaspace over and over again, failing
>> each time, silently swallowing all OOMs and just falling back to
>> interpreted. But that problem exists today too.
>>
>> Thanks, Thomas
>>
>>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>
>>>