Keith McGuigan said the following on 02/01/11 23:11:
On Jan 31, 2011, at 9:20 PM, David Holmes wrote:
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 bugfix.
Given our allocation routines leave a lot to be desired we should strive
to avoid the need for dynamic allocation as it introduces new abort
points. A program that runs fine today may abort with this fix due to
additional C-Heap usage.
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.
Okay my bad they were supposed to be pointer types (it is a linked-list
after all!)
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).
The point is to avoid the need to dynamically allocate the QueueNodes.
If the elements to be queued are self-linking then you don't need the
extra wrapper objects. Take a look at VM_Operations as an example of
self-linking objects.
It also makes it easy to return a list of events for batch processing,
if you chose - ala VM_Operations again.
And regardless of where you put the link, the event specification has to
be allocated in the C-heap ...
I don't care about where the events are allocated, that is a totally
different issue. The point is to avoid the need to allocate nodes.
Batching is a separate issue altogether. The existing code could be
written to batch-process events using the existing classes.
Yes but I think the batching is simpler if the events themselves are linked.
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.
Sorry I was thinking this was returning a pointer. So this is utilizing
C++ copy-semantics to make a valid copy in the caller when the method
returns. Still seems an odd construct to me but I assume C++ deals with
it correctly.
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.
Ok.
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. ...
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.
enqueue() is not an issue for notifications, it has to do them. The
spurious wakeups are only a performance concern, not a correctness one.
There is a single notifyAll that occurs when flushing is complete. This
is implemented by the flush pseudo-event. (see JvmtiDeferredEvent::post()).
But this is a notifyAll per flush-event, which was my point. And
combined with the single-event per iteration processing you do get a lot
of locking/unlocking (due to the need to unlock when calling event.post()).
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.
My mental model for much of this was the vm-operations queue and the
batch processing we do there. But if events are sparse then this is not
an immediate issue.
David
--
- Keith
