Code review request: three native memory tracking related bugs

[Thomas Stüfe]

On Sat, Jul 14, 2012 at 8:57 AM, David Holmes <david.holmes at oracle.com> wrote:
> On 14/07/2012 5:43 AM, Zhengyu Gu wrote:
>>
>> 7181989: NMT ON: Assertion failure when NMT checks thread's native
>> CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181989
>> Webrev: http://cr.openjdk.java.net/~zgu/7181989/webrev.00/
>>
>> We try to assert Thread's stack base to ensure that
>> Thread::record_stack_base_and_size() to record native stack to NMT, but
>> there is scenario that the thread fails to start, which no native stack
>> is created and should not be asserted.
>
>
> I'm not really clear on why we need to refer to the assert inside
> stack_base(). If any started thread fails to setup the stackbase and
> stacksize then it will likely crash well prior to the Thread destructor. It
> seems to me that all that is needed here is to only call
> MemTracker::record_virtual_memory_release for a thread that actually
> started, and for that you can simplify the check to:
>
> if (os_thr != NULL && os_thr->get_state() > INITIALIZED)
>
> and so the whole thing can be expressed simply as:
>
> // Update NMT on Thread destruction, but only if the Thread
> // actually started
> OSThread* os_thr = osthread();
> if (os_thr != NULL && os_thr->get_state() > INITIALIZED) {
>      MemTracker::record_virtual_memory_release(
>           (stack_base() - stack_size()), stack_size(), this);
>
> }
>

I agree with that.

There are a number of fatal()'s e.g. in Linux::current_stack_region()
which chould kick in if stack dimension querying fails. However, a
guarantee() or fatal() inside Thread::record_stack_base_and_size()
would be nice to document that this should never fail.

On a related note, it feels a bit wrong to me to let
Thread::record_stack_base_and_size() record stack allocation to the
NMT as an unexpected side effect. Also feels asymetric, because
recording stack deallocation is done in ~Thread(), as one would
expect.

I think I see why you did it - there are 8 or so places where
Thread::record_stack_base_and_size() is called. However, the general
pattern seems to be that all children of Thread call factored-out
functions in their constructors, and I also would do this for NMT
stack recording, e.g.:

e.g.
void WatcherThread::run() {
 ....
  this->record_stack_base_and_size();
  this->record_stack_with_NMT();
 ....
}

Kind Regards,
Thomas Stüfe
SAP Germany