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
