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

[Zhengyu Gu]

Karen,

Thanks for reviewing. See comments inline.

On 10/3/2012 1:48 PM, Karen Kinnear wrote:
> Zhengyu,
>
> Great job on fixing this complex virtual memory tracking.
>
> Couple of questions/comments.
>
> 1. allocation.hpp
> Could you possibly add a comment that Number of memory types does not include mtNone
> ( or change it so that it does if it needs to)
>
Actually, it does not include mtDontTrack. I moved mt_number_of_types 
ahead of mtDontTrack, and added comment - mtDontTrack is not included as 
validate memory type

> 2. os_solaris.cpp
> I can't get to a current repository right this instant - so do you have the same code for
> windows and linux and bsd?
This is solaris specific problem. Other unix uses platform specific 
anon_mmap() to reserve memory, and also uses plafrom specific 
anon_munmap() to release memory if requested memory region can not be 
acquired. Somehow, there is not solaris specific anon_munmap() 
implementation. Instead, it calls back to os::unmap_memory(), which will 
create 'release' and this is the record with corresponding 'reserve' record.

> 3. memBaseline.cpp baseline_malloc_details
> Could you possibly add a comment when you sort by callsite pc order or
> sort by address order - as to why you need that specific order for the
> operation coming up?
> And for example for baseline_vm_details could you add a comment in front of
> the header specifying thacppt you assume this is sorted in address order.
Comment added
> Lines 317-325: I was not sure why you were incrementing both reserved and
> commited for each callsite: didn't you split the records into separate committed and
> reserved records?
>
VMMemRegion records keep either reserved or committed information, but 
they are aggregated to VMCallsitePointer records, which represents a 
reserved memory region and committed memory size within this region, and 
this region is reserved from a specific callsite.
Lines 317-325  sums up VMCallsitePointer records with the same callsite.
> 4. memSnapshot.cpp
> split_reserved_region
> The way I read reduce_region, you pass it the address and size of the excluded area, not of
> the area you have left. Or did I read this incorrectly?
>
Correct, it is documented in memPtr.hpp VMMemRegion::reduce_region()

   // reduce a memory region to exclude the specified address range.
   // The excluded memory range has to be on either end of this memory
   // region.

> So line 231 wants to have the first argument be: new_rgn_addr and the second argument be
> new_rgn_size (actually like the correct line 235)
>
No. The code is correct. This section deals with the new region is at 
the beginning of the bigger region.
Line 231 reduce original region (bigger region) to this new region, so 
it excludes the upper bound of the
new region (new_rgn_addr + new_rgn_size)  with size = bigger region size 
- new region size.

> And line 232 would create the new region using the same logic as line 236, i.e. create
> the new region with the new_rgn_addr and new_rgn_size.
> I don't think you need to calculate sz here.
>
A bit confusing ... the original region became 'new region'. Here, we 
actually create remaining region.

> So I think both cases need the same logic:
> reduce_region(new_rgn_addr, new_rgn_size)
> next_rgn(new_rgn_addr, flags, new_rgn_size, pc)
> return insert

> line 239: "splitted" ->  "split"
> The rest of the logic looks good.
Fixed

> 5. memSnapshot.cpp
> reserve_region:
>
> It appears that VMMemRegion contains both reserved and committed regions.
> Are they inserted as they are allocated or is there any attempt at
> ordering the address ranges (I think no ordering  - right?)
>
No. VMMemRegion represents either reserved, or committed region.

virtual memory regions are maintained in memory's base address order.
> So I was expecting reserve_region to iterate through all the
> VMMemRegions and

>   1) ignores committed regions and
VMMemPointerIterator::locate() does this

>   2) checks all of them for duplicate records and
Line 71
>   3) checks all for overlapping reserved records
> and then calls insert_record(rec).
Also, no overlapping reserved regions allowed (if there is, regions are 
already split)

> And I expected commit_region to iterate through all the
> VMMemRegions and
> 1) check for duplicate commit records (you do)
> 2) check that there is a reserved region spanning the committed region
> (I think that is what lines 95-96 are doing - but they should do that
> for all reserved regions, to make sure there is at least one reserved
> region that contains the committed region)
> -- so perhaps remove lines 95-96 and change the loop to
> make sure there is one and the rest looks good
> 3) then do the expand or insertion (which you do)
>
> So in both reserve_region and commit_region there is an assertion
> that (VMMemRegion*)current() is a reserved region. Why is that always true?
>
VMMemPointerIterator::locate() only locates 'reserved' records.

> What does line 126 do? I was hoping it would remove next_reg - does it?
Yes.
> And is there a check for partial overlap of a committed region and a reserved region?
>
> line 149: "must due" ->  "must be due"
>
Fixed
> 6. memSnapshot.cpp
> Can you fix "lived" to "live" on line 440
>
Fixed

> 7. memSnapshot.hpp locate
> Did you change the meaning of locate?
> It used to either return contains or return the next larger address so you
> would insert before it.
> It appears to now actually return contains - or null - or did I misread it?
>
Which one you talk about? VMMemPointerIterator::locate()? it return a 
reserved region that contains the specified memory range, or the 
position nearest to it.

Thanks,

-Zhengyu

> thanks,
> Karen
>
>
> On Sep 28, 2012, at 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