Looks ok.
Minor style nit: please add spaces around operators eg i=0, i<_count
Thanks,
David
On 6/09/2012 2:26 AM, Kevin Walls wrote:
Hi again,
I found a memory leak while testing this. thread->name() uses the
ResourceArea. I don't think we've been able to call this enough in the
past to notice the leak, but now we can... Adding a ResourceMark
(management.cpp line 1859) there's no leak.
I updated the webrev in the same place:
http://cr.openjdk.java.net/~kevinw/7196045/webrev.01/
Thanks
Kevin
On 05/09/2012 11:34, David Holmes wrote:
On 5/09/2012 7:16 PM, Kevin Walls wrote:
Hi David,
We could allocate some tens of kb... more if there are several thousand
threads with very long names... It's not a commonly called method, but
that's why the deadlock hasn't been exposed before. Yes you'd need to be
very near that existing cliff-edge...
Just thinking that yes if we are low on memory and don't die from the
NEW_C_HEAP_ARRAY, then the strdup could fail. I'll add a check that we
have something to free() in case that fails on some platforms. In the
loop below if _names_chars[i] was null, the app will get a null string,
again probably the least of it's worries at that point.
It's okay to call free(NULL) so no need to check for it explicitly.
And the NULL is handled by create_from_str.
But the CHECK macro will cause us to return if an exception is posted
and we will leak the dup'd string in that case.
David
Thanks
Kevin
for (int i=0; i<_count; i++) {
Handle s = java_lang_String::create_from_str(_names_chars[i], CHECK);
_names_strings->obj_at_put(i, s());
if (_names_chars[i] != NULL) {
free(_names_chars[i]);
}
}
On 05/09/12 02:51, David Holmes wrote:
Hi Kevin,
On 5/09/2012 8:35 AM, Kevin Walls wrote:
I'd like to get this reviewed, it's a deadlock that can happen due to
Java objects being allocated while holding Threads_lock.
We need to collect just the characters of the thread names while
holding
the lock, and create the String objects when it's released. You do
need
to hit the (non-public) HotspotInternal MBean very rapidly to trigger
this reliably, but there's a small chance the deadlock could happen
when
it's called by the tool that is meant to call it.
http://cr.openjdk.java.net/~kevinw/7196045/webrev.01/
The only thing I don't like here is that the use of NEW_C_HEAP_ARRAY
will cause an abort on out-of-memory and I'm not sure how big this
array might be. If you are running out of C-Heap then you're on the
edge of the cliff anyway and likely to fall at any minute, but I still
dislike seeing more cases added to the code. I don't see an
alternative though. :(
David
Thanks!
Kevin
