[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Request for review, 6766644: Redefinition of compiled method fails with assertion "Can
From: keith.mcguigan () oracle ! com (Keith McGuigan)
Date: 2011-01-31 23:56:02
Message-ID: 29D4617E-925E-463C-9A08-6FAFE81F9227 () oracle ! com
[Download RAW message or body]
Here's a webrev with the changes I said I'd make, for re-review: \
http://cr.openjdk.java.net/~kamg/6766644/webrev.03/
(there was no need for a NULL check for those 'new QueueNode()'
statements as the allocation code called by CHeapObj already does out-
of-memory checking).
--
- Keith
On Jan 30, 2011, at 6:58 PM, David Holmes wrote:
> Hi Keith,
>
> I've been waiting for the dust to settle a little on this before
> commenting. I don't know the semantics of the events enough to
> comment on whether this deferred event processing can impact things,
> but I can comment on the general approach.
>
> This is a pretty major change in the event architecture and I have a
> few comments and concerns - the main concern being "deadlock" as
> it's not obvious exactly what locks can be held when the
> service_lock will be acquired, nor is it obvious that other locks
> won't be acquired while the service-lock is held. Further using one
> thread to wait for and process different kinds of events may
> introduce new interactions that weren't previously possible. How are
> you going to test for potential bad interactions between the low-
> memory detection and compile-event postings? I doubt we have any
> existing tests that try to exercise both bits of functionality.
>
> I'm also a little concerned that changing the LowMemoryDetector
> (LMD) thread in to the Service thread is somewhat disruptive when
> doing stack analysis on running apps or core dumps or crash logs,
> because people have been used to seeing the LMD thread for many
> years and now it has changed its identity.
>
> Looking in jvmtiImpl.cpp
>
> I'd prefer to see the locking in all the JvmtiDeferredEventQueue
> methods rather than having the call-sites do the locking, as it is
> less error-prone. But I see that the main processing loop in the
> service thread makes this impossible due to the dual use of the
> "service lock".
>
> Where we have:
>
> 961 void JvmtiDeferredEventQueue::enqueue(const JvmtiDeferredEvent&
> event) {
> ...
> 967 QueueNode* node = new QueueNode(event);
>
> and
>
> 1007 void JvmtiDeferredEventQueue::add_pending_event(
> 1008 const JvmtiDeferredEvent& event) {
> 1009
> 1010 QueueNode* node = new QueueNode(event);
>
> Will an allocation failure here terminate the VM? I suspect it will
> but I don't think it should. Can we not add a link field to the
> event objects rather than have to create Nodes to hold them?
>
> Minor nit but the canonical pattern typically used for a cas-loop is
> a do-while, eg instead of:
>
> 1012 while (true) {
> 1013 node->set_next((QueueNode*)_pending_list);
> 1014 QueueNode* old_value = (QueueNode*)Atomic::cmpxchg_ptr(
> 1015 (void*)node, (volatile void*)&_pending_list, node->next());
> 1016 if (old_value == node->next()) {
> 1017 break;
> 1018 }
> 1019 }
>
> use:
>
> do {
> node->set_next((QueueNode*)_pending_list);
> } while ( (QueueNode*)Atomic::cmpxchg_ptr(
> (void*)node, (volatile void*)&_pending_list, node->next()) !
> = node->next() );
>
> and similarly in process_pending_events. Though in
> process_pending_events you can just use Atomic::xchg rather than a
> cmpxchg loop.
>
>
> 1076 void JvmtiDeferredEventQueue::wait_for_empty_queue() {
> 1077 MutexLockerEx ml(Service_lock,
> Mutex::_no_safepoint_check_flag);
> 1078 while (has_events()) {
> 1079 Service_lock->notify_all();
> 1080 Service_lock->wait(Mutex::_no_safepoint_check_flag);
> 1081 }
> 1082 }
>
> What is the notify_all for?
>
>
> 1084 void JvmtiDeferredEventQueue::notify_empty_queue() {
> 1085 assert(Service_lock->owned_by_self(), "Must own Service_lock");
> 1086 Service_lock->notify_all();
> 1087 }
>
> This looks suspicious - notification should always occur in
> conjunction with a change of state, which should be atomic with
> respect to the notification. Looking at the usage in the service-
> thread wait-loop I don't understand who is being notified of what.
> Whomever emptied the queue should be doing the notification ie in
> dequeue() if _queue_head == NULL ie at line 997.
>
> In dequeue, this:
>
> 988 if (_queue_head == NULL) {
> 989 // Just in case this happens in product; it shouldn't be
> let's not crash
> 990 return JvmtiDeferredEvent();
> 991 }
>
> doesn't look right. Are we returning a stack allocated instance
> here? (There's also a typo in the comment.)
>
>
> ---
>
> In jvmtiExport.cpp, I don't understand why here:
>
> 832 void JvmtiExport::post_dynamic_code_generated(const char *name,
> const void *code_begin, const void *code_end) {
> ...
> 1841
> 1842 JvmtiDeferredEventQueue::wait_for_empty_queue();
> 1843
>
> we have to wait for an empty queue, particularly as the queue may
> become non-empty at any moment. Normally such a wait must be atomic
> with respect to an action that requires the queue to be empty, but
> that doesn't seem to be the case here.
>
> ---
>
> In serviceThread.cpp, are we sure that it is okay to remain
> _thread_blocked while executing this:
>
> 91 ThreadBlockInVM tbivm(jt);
> 92
> 93 MutexLockerEx ml(Service_lock,
> Mutex::_no_safepoint_check_flag);
> 94 while (!(sensors_changed =
> LowMemoryDetector::has_pending_requests()) &&
> 95 !(has_jvmti_events =
> JvmtiDeferredEventQueue::has_events())) {
> 96 // wait until one of the sensors has pending requests,
> or there is a
> 97 // pending JVMTI event to post
> 98 JvmtiDeferredEventQueue::notify_empty_queue();
> 99 Service_lock->wait(Mutex::_no_safepoint_check_flag);
> 100 }
> 101
> 102 if (has_jvmti_events) {
> 103 jvmti_event = JvmtiDeferredEventQueue::dequeue();
> 104 }
>
> It may be fine but it seems a little strange.
>
> ---
>
> There's no thread.hpp in the webrev but presumably you can now
> delete the LowMemoryDetectorThread class.
>
> ---
>
> Cheers,
> David
>
>
>
> Keith McGuigan said the following on 01/29/11 03:27:
> > Ok, here's my next attempt: http://cr.openjdk.java.net/~kamg/6766644/webrev.02
> > This enqueues the compiled-method-unloaded events so that they the
> > posting of load/unload will be in order. Other changes include
> > locking the nmethod until after the compiled-method-load event is
> > posted, and an alternate mechanism for enqueuing the compiled-
> > method-unload events that are generated at safepoint.
> > --
> > - Keith
> > On Jan 26, 2011, at 5:07 PM, Daniel D. Daugherty wrote:
> > > On 1/26/2011 2:52 PM, Tom Rodriguez wrote:
> > > > On Jan 26, 2011, at 1:39 PM, Daniel D. Daugherty wrote:
> > > >
> > > > > It also looks like there must be order between the load and
> > > > > unload events:
> > > > >
> > > > > CompiledMethodLoad for foo
> > > > > CompiledMethodUnload for foo
> > > > > CompiledMethodLoad for foo (again)
> > > > >
> > > >
> > > > Do you mean we can't have multiple versions of compiled code for
> > > > the same method? I don't think that's true or should be
> > > > required. nmethod freeing is very lazy and there's no guarantee
> > > > that we will have completed unloading of an nmethod before we've
> > > > created a new variation of it. It would also be completely ok
> > > > for a JVM to have multiple versions of the compiled code for a
> > > > method. Obviously the load and unload for a particular nmethod
> > > > must be properly ordered.
> > > >
> > >
> > > That last sentence is what I meant; load and unload for a specific
> > > compiled version of foo (nmethod) must be in order.
> > >
> > > Dan
> > >
> > >
> > >
> > > >
> > > > > which is going to mean coordination between the mechanisms for
> > > > > posting
> > > > > of both CompiledMethodLoad and CompiledMethodUnload events.
> > > > >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic