Yeah, I agree that it is hard to know where to log these failures to, although I don't like them being swallowed. We should have a standardized way of logging all JVM failures, warnings, etc. (JEP 158 anyone?)
Anyway, that's unrelated to your patch, which looks good to me. /Staffan On 5 sep 2013, at 16:17, Kevin Walls <[email protected]> wrote: > > Hi Staffan -- > > Yes, I made some attempts but didn't reproduce it, though anything that > allocates can presumably cause an exception if the timing and memory usage is > right (or wrong). > > On the clearing of exceptions... All about not crashing the Service Thread I > suppose, but it may not have been obivous where if anywhere any exceptions > should be logged: should the running app's output be interrupted with > exceptions because somebody attached jconsole to monitor it?... (that's one > way of getting these GC notifications turned on). > > > I'm thinking the call to createGcInfo was just an oversight, the calls such > as for String creation are done with CHECK, and yes any failures there would > get caught and cleared by the caller, sendNotification, as you mention. > > Thanks > Kevin > > > On 05/09/13 14:35, Staffan Larsen wrote: >> I agree that your change looks good, but there are a couple of things that >> bug me. >> >> Why did createGcInfo() fail? Hard to tell if you can't repro the failure, >> though. >> >> The code below will make sure we swallow any exceptions that occurred >> without reporting them. This isn't good practice. I would like these >> exceptions to be logged somehow. This isn't your code or part of your >> change, it just makes me worried. >> >> void GCNotifier::sendNotification(TRAPS) { >> GCNotifier::sendNotificationInternal(THREAD); >> // Clearing pending exception to avoid premature termination of >> // the service thread >> if (HAS_PENDING_EXCEPTION) { >> CLEAR_PENDING_EXCEPTION; >> } >> } >> >> >> /Staffan >> >> On 5 sep 2013, at 15:12, Kevin Walls <[email protected]> wrote: >> >>> Hi, >>> >>> If I have my CHECK and THREAD thinking straight, I have a small review >>> request in gcNotifier, to avoid a crash that came up in testing recently. >>> >>> The report is a hotspot crash in a test, where there's an exception pending >>> when calling java_lang_String::create_from_str, which allocates. >>> >>> We are in GCNotifier::sendNotificationInternal and before the call that >>> crashes, we called createGcInfo which have several allocations can return a >>> null handle if an exception occurs. >>> >>> If we have returned from createGcInfo, which returns null for an exception, >>> we need to return rather than continue in sendNotificationInternal, which >>> will try to allocate again. >>> >>> sendNotificationInternal has a caller that clears any exception. >>> >>> So that should be a very small change: >>> >>> $ hg diff src/share/vm/services/gcNotifier.cpp >>> diff --git a/src/share/vm/services/gcNotifier.cpp >>> b/src/share/vm/services/gcNotifier.cpp >>> --- a/src/share/vm/services/gcNotifier.cpp >>> +++ b/src/share/vm/services/gcNotifier.cpp >>> @@ -209,7 +209,7 @@ >>> GCNotificationRequest *request = getRequest(); >>> if (request != NULL) { >>> NotificationMark nm(request); >>> - Handle objGcInfo = createGcInfo(request->gcManager, >>> request->gcStatInfo, THREAD); >>> + Handle objGcInfo = createGcInfo(request->gcManager, >>> request->gcStatInfo, CHECK); >>> >>> Handle objName = >>> java_lang_String::create_from_str(request->gcManager->name(), CHECK); >>> Handle objAction = java_lang_String::create_from_str(request->gcAction, >>> CHECK); >>> kwalls@klaptop:~/work/bugs/8023478.gcbean/hotspot-rt$ >>> >>> >>> >>> http://bugs.sun.com/view_bug.do?bug_id=8023478 >>> http://cr.openjdk.java.net/~kevinw/8023478/webrev.00/ (the above one line) >>> >>> >>> Thanks >>> Kevin >>> -------- >>> PS >>> As the bug is not available on the bugs site at the moment, this is part of >>> the crashing stack to give some context: >>> >>> V [libjvm.so+0x7069d0] void report_vm_error(const char*,int,const >>> char*,const char*)+0x78;; __1cPreport_vm_error6Fpkci11_v_+0x78 >>> V [libjvm.so+0x63125c] void >>> CollectedHeap::check_for_valid_allocation_state()+0x4c;; >>> __1cNCollectedHeapbGcheck_for_valid_allocation_state6F_v_+0x4c >>> V [libjvm.so+0x911f68] instanceOop >>> InstanceKlass::allocate_instance(Thread*)+0x78;; >>> __1cNInstanceKlassRallocate_instance6MpnGThread__nLinstanceOop__+0x78 >>> V [libjvm.so+0x9ffd7c] Handle >>> java_lang_String::basic_create(int,Thread*)+0x174;; >>> __1cQjava_lang_StringMbasic_create6FipnGThread__nGHandle__+0x174 >>> V [libjvm.so+0xa00520] Handle java_lang_String::create_from_str(const >>> char*,Thread*)+0x40;; >>> __1cQjava_lang_StringPcreate_from_str6FpkcpnGThread__nGHandle__+0x40 >>> V [libjvm.so+0x83eb0c] void >>> GCNotifier::sendNotificationInternal(Thread*)+0x8c;; >>> __1cKGCNotifierYsendNotificationInternal6FpnGThread__v_+0x8c >>> V [libjvm.so+0x83ea4c] void GCNotifier::sendNotification(Thread*)+0xc;; >>> __1cKGCNotifierQsendNotification6FpnGThread__v_+0xc >>> V [libjvm.so+0x103faf8] void >>> ServiceThread::service_thread_entry(JavaThread*,Thread*)+0x4a0;; >>> __1cNServiceThreadUservice_thread_entry6FpnKJavaThread_pnGThread__v_+0x4a0 >>> >>> >
