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

[Thomas Stüfe]

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.

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