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

[Karen Kinnear]

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)

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?

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 that you assume this is sorted in address order.

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?

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?

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)

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.

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.


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?)

So I was expecting reserve_region to iterate through all the
VMMemRegions and
 1) ignores committed regions and 
 2) checks all of them for duplicate records and
 3) checks all for overlapping reserved records
and then calls insert_record(rec).

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?

What does line 126 do? I was hoping it would remove next_reg - does it?

And is there a check for partial overlap of a committed region and a reserved region?

line 149: "must due" -> "must be due"

6. memSnapshot.cpp
Can you fix "lived" to "live" on line 440 

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?

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