Review Request: 8011161 NMT: Memory leak when encountering out of memory error while initializing memory snapshot

[David Holmes]

On 6/04/2013 12:05 AM, Zhengyu Gu wrote:
> Updated: http://cr.openjdk.java.net/~zgu/8011161/webrev.03/
> <http://cr.openjdk.java.net/%7Ezgu/8011161/webrev.03/>

I have a minor style nit - feel free to ignore. In:

  549   if (_worker_thread == NULL || _worker_thread->has_error()) {
  550     if (_worker_thread != NULL) {
  551       delete _worker_thread;
  552       _worker_thread = NULL;
  553     }
  554     return false;
  555   }

it seems awkward to me to use the || but then have to test again to find 
out which clause got us into the if block. I'd prefer:

    if (_worker_thread == NULL) {
      return false;
    }
    else if (_worker_thread->has_error()) {
      delete _worker_thread;
      _worker_thread = NULL;
      return false;
    }

Cheers,
David

> Thanks,
>
> -Zhengyu
>
> On 4/4/2013 4:35 PM, Zhengyu Gu wrote:
>> Dan,
>>
>> Thanks for the suggestion. I will post updated webrev once java.net is
>> back.
>>
>> -Zhengyu
>>
>> On 4/4/2013 3:14 PM, Daniel D. Daugherty wrote:
>>> On 4/4/13 11:42 AM, Zhengyu Gu wrote:
>>>> Updated webrev: http://cr.openjdk.java.net/~zgu/8011161/webrev.01/
>>>> <http://cr.openjdk.java.net/%7Ezgu/8011161/webrev.01/>
>>>
>>> src/share/vm/services/memTracker.cpp
>>>   Please consider this version:
>>>
>>>   _snapshot = new (std::nothrow)MemSnapshot();
>>>   if (_snapshot != NULL) {
>>>     if (!_snapshot->out_of_memory() && start_worker()) {
>>>       _state = NMT_started;
>>>       NMT_track_callsite = (_tracking_level == NMT_detail &&
>>> can_walk_stack());
>>>       return;
>>>     }
>>>
>>>     delete _snapshot;
>>>     _snapshot = NULL;
>>>   }
>>>
>>>   // fail to start native memory tracking, shut it down
>>>   shutdown(NMT_initialization);
>>>
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>
>>>>
>>>>
>>>> On 4/3/2013 4:35 PM, Calvin Cheung wrote:
>>>>> Not directly related to this fix, I'm seeing
>>>>> shutdown(NMT_initialization) can be called twice if start_worker()
>>>>> has an error before returning false.
>>>>> 552 shutdown(NMT_initialization);
>>>>>
>>>>> again in MemTracker::start():
>>>>> 144 shutdown(NMT_initialization);
>>>>>
>>>>> Perhaps the one at line 552 isn't necessary?
>>>>>
>>>>> Calvin
>>>>>
>>>>> On 4/3/2013 11:01 AM, Zhengyu Gu wrote:
>>>>>> Resend. Could anyone review it?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Zhengyu
>>>>>>
>>>>>> On 4/2/2013 10:09 AM, Zhengyu Gu wrote:
>>>>>>> Fix a memory leak when initializing memory snapshot due to out of
>>>>>>> memory.
>>>>>>> In normal case, memory snapshot is released by worker thread. But
>>>>>>> this case, the worker thread is never started.
>>>>>>>
>>>>>>>
>>>>>>> bug: http://bugs.sun.com/view_bug.do?bug_id=8011161
>>>>>>> <http://bugs.sun.com/view_bug.do?bug_id=8011161>
>>>>>>> webrev: http://cr.openjdk.java.net/~zgu/8011161/webrev.00/
>>>>>>> <http://cr.openjdk.java.net/%7Ezgu/8011161/webrev.00/>
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> -Zhengyu
>>>>>>
>>>>>
>>>>
>>>
>>
>