Code review request: 7199092 NMT: NMT needs to deal overlapped virtual memory ranges

[Karen Kinnear]

Zhengyu,

Thank you for the fixes. I am impressed you found and fixed so many difficult issues.
This looks really good.

A couple of minor questions/comments:

1) os_solaris.cpp
Did you add the new record_virtual_memory_reserve because you were running into the case in which
the anon_mmap returns a value that does not match the requested address and
therefore we call unmap_memory which already calls record_virtual_memory_release? 

In os_linux.cpp, os_bsd.cpp, this is handled by calling anon_munmap inside pd_attempt_reserve_memory_at,
rather than calling out to the platform-independent unmap_memory. So unless you have a strong reason to do
this differently on Solaris, I would recommend that you change it to call pd_unmap_memory instead.


2) thread.cpp
line 4077 "became" -> "becomes"
 line 4078 "not" ->"no" ,"walk" -> "walks"
Question - do you need to do the MemTracker::thread_exiting before the set_safepoint_visible(false) call?

3) I see you added record_stack_base_and_size for the AttachListener.
I am wondering if we also need this for other internal threads:
  compiler threads, low memory detector, service thread, jvmti agent threads, signal handler, 3 JFR threads
--- we should add to our cleanup wishlist to clean up creating internal threads so we can share more common code
and include this as part of that if you want to have this done separately.

4) I don't understand the assertion in memTracker.hpp release_thread_stack,
assert (!thr->is_Java_thread(), "too early") ?

5) memSnapshot.hpp
line 115: "where is" -> "where"
Thank you for simplifying this to just pass in one address

thanks,
Karen




On Oct 18, 2012, at 2:54 PM, Zhengyu Gu wrote:

> The new webrev reflects some suggestions from previous code review, along with:
> 
> 1. Caught more cases that virtual memory operations do not go through os api, which cause mis-matched records that confuse NMT.
> 2. Fixed a bug that JavaThread releases its per-thread recorder too later, so that the record could be misplaced in wrong generation.
> 3. Cleanup. Removed some dead code and unnecessary assertions
> 4. Fixed racing conditions that could mis-count arena sizes. When ResourceMark or HandleMark resets, it should reset arena size first, before release memory chunks.
> 5. Fixed missing call to record native stack on AttachListener thread.
> 6.  Removed unused Arena constructor.
> 
> Webrev: http://cr.openjdk.java.net/~zgu/7199092/webrev.01/
> 
> Thanks,
> 
> -Zhengyu
> 
> 
> On 9/28/2012 4:36 PM, Zhengyu Gu wrote:
>> With PermGen removal integration, virtual memory usage patterns have changed vs. earlier. NMT can not longer just track reserved memory regions, and assume commits and uncommits are performed from the tail end.
>> 
>> NMT now tracks committed memory regions, alone with reserved memory regions, a virtual memory map is maintained by NMT, and it is reported with detail tracking data.
>> 
>> The changes also include some cleanup and enhanced assertion, etc.
>> 
>> CR: http://bugs.sun.com/view_bug.do?bug_id=7199092
>> Webrev:  http://cr.openjdk.java.net/~zgu/7199092/webrev.00/
>> 
>> 
>> An example of virtual memory map:
>> 
>> Virtual memory map:
>> 
>> [0x8f17e000 - 0x8f364000] reserved 1944KB for Thread Stack
>>        from [Thread::record_stack_base_and_size()+0x120]
>>    [0x8f17e000 - 0x8f364000] committed 1944KB from [Thread::record_stack_base_and_size()+0x120]
>> 
>> [0x8f389000 - 0x8f680000] reserved 3036KB for Thread Stack
>>        from [Thread::record_stack_base_and_size()+0x120]
>>    [0x8f389000 - 0x8f680000] committed 3036KB from [Thread::record_stack_base_and_size()+0x120]
>> 
>> [0x8f7dd000 - 0x8f900000] reserved 1164KB for Thread Stack
>>        from [Thread::record_stack_base_and_size()+0x120]
>>    [0x8f7dd000 - 0x8f900000] committed 1164KB from [Thread::record_stack_base_and_size()+0x120]
>> 
>> [0x8fa20000 - 0x8faa1000] reserved 516KB for Thread Stack
>>        from [Thread::record_stack_base_and_size()+0x120]
>>    [0x8fa20000 - 0x8faa1000] committed 516KB from [Thread::record_stack_base_and_size()+0x120]
>> 
>> [0x8fd30000 - 0x914b8000] reserved 24096KB for GC
>>        from [ReservedSpace::initialize(unsigned int, unsigned int, bool, char*, unsigned int, bool)+0x555]
>>    [0x8fd30000 - 0x914b8000] committed 24096KB from [PSVirtualSpace::expand_by(unsigned int)+0x95]
>> 
>> [0x914b8000 - 0x916bc000] reserved 2064KB for Thread Stack
>>        from [Thread::record_stack_base_and_size()+0x120]
>>    [0x914b8000 - 0x916bc000] committed 2064KB from [Thread::record_stack_base_and_size()+0x120]
>> 
>> [0x916bc000 - 0x91860000] reserved 1680KB for GC
>>        from [ReservedSpace::initialize(unsigned int, unsigned int, bool, char*, unsigned int, bool)+0x555]
>>    [0x916bc000 - 0x916c7000] committed 44KB from [PSVirtualSpace::expand_by(unsigned int)+0x95]
>>    [0x91764000 - 0x9176f000] committed 44KB from [CardTableModRefBS::resize_covered_region(MemRegion)+0xebf]
>>    [0x9180b000 - 0x91811000] committed 24KB from [CardTableModRefBS::resize_covered_region(MemRegion)+0xebf]
>>    [0x9185f000 - 0x91860000] committed 4KB from [CardTableModRefBS::CardTableModRefBS(MemRegion, int)+0x2a0]
>> 
>> [0x91860000 - 0xb1060000] reserved 516096KB for Java Heap
>>        from [ReservedSpace::initialize(unsigned int, unsigned int, bool, char*, unsigned int, bool)+0x190]
>>    [0x91860000 - 0x92d50000] committed 21440KB from [PSVirtualSpace::expand_by(unsigned int)+0x95]
>>    [0xa6710000 - 0xa7180000] committed 10688KB from [PSVirtualSpace::expand_by(unsigned int)+0x95]
>>    [0xb0e60000 - 0xb0ea9000] committed 292KB from [VirtualSpace::initialize(ReservedSpace, unsigned int)+0x3e8]
>> 
>> [0xb1069000 - 0xb71e9000] reserved 99840KB for Code
>>        from [ReservedSpace::initialize(unsigned int, unsigned int, bool, char*, unsigned int, bool)+0x555]
>>    [0xb1069000 - 0xb1072000] committed 36KB from [VirtualSpace::initialize(ReservedSpace, unsigned int)+0x3e8]
>>    [0xb11e9000 - 0xb1429000] committed 2304KB from [VirtualSpace::initialize(ReservedSpace, unsigned int)+0x3e8]
>> 
>> [0xb71e9000 - 0xb77e9000] reserved 6144KB for Class
>>        from [ReservedSpace::initialize(unsigned int, unsigned int, bool, char*, unsigned int, bool)+0x555]
>>    [0xb71e9000 - 0xb750a000] committed 3204KB from [VirtualSpace::initialize(ReservedSpace, unsigned int)+0x3e8]
>> 
>> [0xb77e9000 - 0xb783a000] reserved 324KB for Thread Stack
>>        from [Thread::record_stack_base_and_size()+0x120]
>>    [0xb77e9000 - 0xb783a000] committed 324KB from [Thread::record_stack_base_and_size()+0x120]
>> 
>> Tests performed:
>> - JPRT tests
>> 
>> - Linux 32 bit:
>>    vm.quick.testlist
>>    nsk.stress
>>    runtime.ParallelClassLoading
>>    nsk.quick-jdi.testlist
>>    nsk.quick-jvmti.testlist
>> 
>> - Solaris x64
>>    vm.quick.testlist
>>    runtime.ParallelClassLoading
>>    nsk.stress.jck60
>> 
>> - Solaris sparcv9
>>    vm.quick.testlist
>>    runtime.ParallelClassLoading
>>    nsk.stress.jck60
>> 
>> 
>> Thanks,
>> 
>> -Zhengyu