On 11/21/18 23:42, 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 <jcbey...@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 :
Hi David,
I guess, it is again some issue with the mailing system.
Please, find my and Dan's replies to your email in the attachments.
I'm still thinking about what would be a better choice here.
Thanks,
Serguei
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
--- Begin Message ---
Hi David,
On 11/14/18 15:05, David Holmes wrote:
Hi Serguei,
On 15/11/2018 5:20 am, serguei.spit...@oracle.com wrote:
Hi Thomas,
Thank you for taking care about this issue!
We recently also saw a couple of problems related to the compiler
thread not being hidden.
I have several comments to the fix.
It would be really nice if there is any chance to have a unit test
for this.
But I understand that it is not easy to minimize what you have in
your agent.
http://cr.openjdk.java.net/~stuefe/webrevs/8213834-jvmti-reourceexhausted-shall-not-be-posted-from-compiler-thread/webrev.00/webrev/src/hotspot/share/prims/jvmtiExport.cpp.udiff.html
void JvmtiExport::post_resource_exhausted(jint
resource_exhausted_flags, const char* description) {
+
+ // Handlers may call back into the JVM as a reaction to this event,
e.g. to print out
+ // a class histogram. Most of those actions require the ability to
run java though. So
+ // lets not post this event in cases where we cannot run java (e.g.
when a CompilerThread
+ // triggers a Metaspace OOM).
+ Thread* thread = Thread::current_or_null();
+ if (thread == NULL || !thread->can_call_java()) {
+ return;
+ }
+
1. Did you check that your fix does also work with the Graal enabled?
The CompilerThread::can_call_java() should return true for JVMCI is
used: bool CompilerThread::can_call_java() const { return _compiler
!= NULL && _compiler->is_jvmci(); }
That's exactly why I suggested checking for can_call_Java() - because
it would work with JVMCI/Graal.
Does it mean, we want to continue posting such events on the JVMCI/Graal
compiler threads executing Java?
2. It is better to use the is_hidden_from_external_view() instead of
!can_call_java()
Why? In terms of being conservative that would be unnecessarily
conservative. If the problem is only executing Java code why go beyond
that?
The is_hidden_from_external_view() means the same as !can_call_java()
CompilerThread's:
// Hide native compiler threads from external view.
bool is_hidden_from_external_view() const { return
!can_call_java(); }
It seems the ServiceThread and CodeCacheSweeperThread do not follow
the above rule.
But do we want to post any events on the hidden threads?
BTW what exactly is the set of hidden threads these days?
From my observation the following threads are hidden now:
ServiceThread
CodeCacheSweeperThread
All native CompilerThread's that return false from can_call_java().
Thanks,
Serguei
3. The 'thread' is already defined in post_resource_exhausted:
JavaThread *thread = JavaThread::current();
But it is in a deeper scope.
void JvmtiExport::post_resource_exhausted(jint
resource_exhausted_flags, const char* description) {
EVT_TRIG_TRACE(JVMTI_EVENT_RESOURCE_EXHAUSTED, ("Trg resource
exhausted event triggered" ));
JvmtiEnvIterator it;
for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
if (env->is_enabled(JVMTI_EVENT_RESOURCE_EXHAUSTED)) {
EVT_TRACE(JVMTI_EVENT_RESOURCE_EXHAUSTED, ("Evt resource
exhausted event sent" ));
JavaThread *thread = JavaThread::current();
I'd suggest to move it up as it is a better place for it anyway.
Also, you do not need the extra check:
if (thread == NULL || !
Agreed - Thread::current() can not be NULL in this context.
Cheers,
David
Thanks,
Serguei
On 11/14/18 07:06, Daniel D. Daugherty wrote:
Adding serviceability-dev@...
Since the proposed solution to the bug is to not post an event, I think
the Serviceability Team (which owns JVM/TI) needs to be involved
directly
in the review.
Dan
On 11/14/18 9:28 AM, Thomas Stüfe wrote:
Dear all,
may I please have reviews on this small patch. Note that this is
borderline serviceability. I try to avoid crossposting, but if you
think this should be looked at by serviceability feel free to forward
it there.
Issue: https://bugs.openjdk.java.net/browse/JDK-8213834
CR:
http://cr.openjdk.java.net/~stuefe/webrevs/8213834-jvmti-reourceexhausted-shall-not-be-posted-from-compiler-thread/webrev.00/webrev/
Short description: we may post ResourceExhausted from Compiler
threads. Handlers of this event may call back into the JVM, which may
cause problems since we run in the compiler thread. The proposed
solution is not to post the event in that case.
See previous discussion here:
http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-November/025898.html
Thanks, Thomas
--- End Message ---
--- Begin Message ---
On 11/14/18 6:01 PM, David Holmes wrote:
On 15/11/2018 6:13 am, Daniel D. Daugherty wrote:
I do not have a better idea at the moment.
The ResourceExhausted event is one of many JVM/TI events. So we put in a
special case for this event because an agent out in the world tries
to do
something that is not expressly forbidden by the spec, but it is a bad
thing to do with HotSpot. Okay. What about the next event? What about
the next agent? Once you've added a special case, where do you stop?
I had proposed more broadly not posting any events in threads that can
not run Java but that is potentially more damaging to existing agents
that just happen to work in the compiler thread today.
But it goes beyond just running Java. Can these other threads make JNI
calls, or other JVM TI calls? If they can run Java they can do all
those things. If they can't run Java they may not be able to.
There is no perfect solution here because JVM TI allows too much
freedom. It doesn't put any constraints on the threads that can
execute callbacks, and has very few constraints on what callbacks can
be executed when.
On the other hand an implementation has complete freedom as to when
exactly resource exhaustion events get posted. Does it make sense for
an internal VM thread to post this ResourceExhausted event? Is it just
an accident of implementation that the code path decided to throw OOME
and so also posts the event? You can easily imagine this code not
doing that - and as Thomas points out the compiler thread swallows the
exception anyway!
As there is no way to know what an agent may do ahead of time we are
faced with either:
- breaking some agents and crashing the VM; or
- masking an event
I vote for no breakage.
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...
Dan
Cheers,
David
Dan
On 11/14/18 2:59 PM, Thomas Stüfe wrote:
Hi Dan,
On Wed, Nov 14, 2018 at 8:32 PM Daniel D. Daugherty
<daniel.daughe...@oracle.com> wrote:
I have a philosophical problem with a fix like this.
The fix is making the assumption that the handler for this event
will want
to run Java code and if the event is posted from a Java thread that
cannot
run Java code, then we skip posting the event.
If I happen to have a more conservative agent that does not run
Java code
in its handlers, then my agent won't receive this event even though it
should not cause my agent any problems. That might be an unexpected
change
in behavior on the part of my agent.
Dan
I was thinking about that too. In fact, the JVMTI agent I am trying to
add this fix for exists in two variants. The base form,
airlift/jvmkill, just does a plain kill(2). So not problem there. Only
the forked version by cloudfoundry/jvmkill do this fancy stuff. (Note
that both projects are on github, if you care to look them up. They
are not by me. I am just using them).
But the problem is, the JVMTI spec says nothing about what you can or
cannot do in reaction to a ResourceExhausted event. So what
cloudfoundry/jvmkill does is valid and not forbidden.
Therefore I think suppressing ResourceExhausted in this case is the
only choice we have. One might fine-tune the conditions under which we
suppress sending ResourceExhausted: maybe suppress for
CompilerThreads, or only for CompilerThreads getting MetaspaceOOMs...
But I think we should do something here. By neglecting to add
restrictions to the JVMTI spec, we encouraged JVMTI agents to do these
kind of things. The least we can do is minimize the damage.
And then, usually this will not be the last opportunity for
ResourceExhausted to be posted, no? Chances are high that there will
be more OOMs following, in real java threads.
Finally, I find swallowing the ResourceExhausted for compiler threads
is symmetric to the way the compiler thread swallows the OOME itself.
They do not report the OOME but ignore and clear it.
But as you can see, I see your point. Do you have a better proposal?
..Thomas
On 11/14/18 10:06 AM, Daniel D. Daugherty wrote:
Adding serviceability-dev@...
Since the proposed solution to the bug is to not post an event, I
think
the Serviceability Team (which owns JVM/TI) needs to be involved
directly
in the review.
Dan
On 11/14/18 9:28 AM, Thomas Stüfe wrote:
Dear all,
may I please have reviews on this small patch. Note that this is
borderline serviceability. I try to avoid crossposting, but if you
think this should be looked at by serviceability feel free to
forward
it there.
Issue: https://bugs.openjdk.java.net/browse/JDK-8213834
CR:
http://cr.openjdk.java.net/~stuefe/webrevs/8213834-jvmti-reourceexhausted-shall-not-be-posted-from-compiler-thread/webrev.00/webrev/
Short description: we may post ResourceExhausted from Compiler
threads. Handlers of this event may call back into the JVM, which
may
cause problems since we run in the compiler thread. The proposed
solution is not to post the event in that case.
See previous discussion here:
http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-November/025898.html
Thanks, Thomas
--- End Message ---
