Request for review, 6766644: Redefinition of compiled method fails with assertion "Can not load classes with the Compiler thread"

Keith McGuigan keith.mcguigan at
Tue Feb 1 05:11:50 PST 2011

On Jan 31, 2011, at 9:20 PM, David Holmes wrote:

>>> 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?
>> I can add a NULL check to those spots.  There's no need for us to  
>> segv if we can't allocate memory for this.  We do need a C-heap  
>> allocated object since we're passing these events from thread-to- 
>> thread.  Adding the link in the event object itself wouldn't help -  
>> it would just mean we have to allocate the event object in C-heap  
>> instead.
> As per your other email NULL check is pointless as "new" will never  
> return null but will instead call vm_exit_out_of_memory to abort the  
> VM - which is exactly my point. I don't believe that a failure to  
> allocate in this code should be considered a fatal error for the VM.

I agree, there may be situations where memory allocation failures  
should be considered non-fatal, and the Hotspot allocation routines  
should offer some support for that.  However, it's not clear to me  
that the various specifications we adhere to have "escape-clause"  
situations where events can be dropped or features scaled back when/if  
we run into memory constraints.  They should, no doubt, but I don't  
recall seeing anything like that at the present.  It's a can of worms  
that is probably worth opening up, but it's a bigger issue than this  

> I don't understand why we would have to allocate the event objects  
> in C-heap, all we want is a link field so that they can form their  
> own queue without the need for wrapper Nodes.
> 447 class JvmtiDeferredEvent VALUE_OBJ_CLASS_SPEC {
> 448   friend class JvmtiDeferredEventQueue;
> 449  private:
>       JvmtiDeferredEvent _next; // for chaining events together
>      public:
>       JvmtiDeferredEvent next() { return _next; }
>       void set_next(JvmtiDeferredEvent e) {
>        assert(_next == NULL, "overwriting linked event!");
>        _next = e;
>       }
> Further this would allow you to dequeue all the events at once  
> instead of the service thread going through pulling them off one at  
> a time.

That code doesn't work in C++.  You can't have a type contain itself.   
How big would it be?  I suppose what you're suggesting is the Java  
semantics where the '_next' field is a reference/pointer.  If so,  
you've just described the 'QueueNode' class.   Could I collapse the  
definitions and have just one instead of both JvmtiDeferredEvent and  
QueueNode?  Sure, but I see value in having them separate.  For one,   
you can have different allocation schemes - QueueNodes go into the C- 
heap and JvmtiDeferredEvents can be value objects.  Another reason is  
to separately encapsulate two different concepts that aren't  
necessarily related (list functionality and event specification).

And regardless of where you put the link, the event specification has  
to be allocated in the C-heap in order to be enqueued on one thread  
(the compiler thread) and consumed on the other (the service thread)  
asynchronously.  If the compiler thread waited for event to be posted  
before continuing, then one could conceivably stack-allocate on the  
compiler thread and pass a reference to the event to the service  
thread, but making the compiler thread wait introduces deadlock  
possibilities when run with -Xbatch.

Batching is a separate issue altogether.  The existing code could be  
written to batch-process events using the existing classes.

>>> 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.)
>> Yes, we're returning a stack-allocated instance, which returns to  
>> the caller as a temp and is copied into a local variable there.   
>> I'll fix the comment.
> It isn't "legal" to return a stack-allocated object from a method. I  
> expect we'll get away with it but still ...
> Couldn't you just allocate a static instance to use in this case?

There's nothing wrong with returning an object by value.

> Ok so in general I like the form of the new architecture but I'm a  
> bit unclear on the details. In general non-service threads will only  
> call:
> - enqueue() to add an event
> - add_pending_event() to add an event in the special case of a  
> safepoint being active
> - flush_queue() to wait for earlier events to get posted
> The service thread will:
> - loop in wait() until there is an event (enqueue will do the  
> notifyAll)
> - dequeue() to pull off that event


> Now it seems that flush_queue is also called by the service thread,  
> but I can't see where from?

It's not called directly, but it could happen if a user's JVMTI  
compiled-method-load/unload event handler method calls JVMTI's  
GenerateEvents(), so we need to code defensively here to prevent any  
inadvertent self-deadlocks.

> I'm a little concerned about all the locking/unlocking that seems to  
> happen; and the fact that there are potentially a lot of spurious  
> notifications. Though this is mitigated if we only expect a couple  
> of threads to be involved, there are ways to restructure things so  
> that event processing can be batched, including a single notifyAll  
> once all flush events are complete.

The enqueue code does not know the state of the service thread (nor  
should it have to), so it will always have to send a notification to  
wake it up if it's sleeping (even if the events are batched) And all  
the waiting code loops checking for conditions, so a spurious  
notification ought to be harmless.

There is a single notifyAll that occurs when flushing is complete.   
This is implemented by the flush pseudo-event. (see  

No doubt this could be restructured a number of different ways, but  
this way is rather straightforward (easy to analyze) and appears  
correct.   The possibility to batch-process the events in the future  
is there if we need it for some reason, but I don't see a need for it  
at the moment.

- Keith
-------------- next part --------------
An HTML attachment was scrubbed...

More information about the hotspot-runtime-dev mailing list