Code review request: CR 7181995 NMT ON: NMT assertion failure assert(cur_vm->is_uncommit_record() || cur_vm->is_deallocation_record

[Zhengyu Gu]

On 9/11/2012 6:40 PM, Karen Kinnear wrote:
> Zhengyu,
>
> Thank you for fixing something so complicated so quickly.
> The code looks good, ok to check it. And many thanks.
>
> Couple of minor questions/comments:
>
> 1. memSnapshot.hpp
>    line 114: "exiting" ->  "existing"
>
Fixed

> 2. memSnapshot.hpp
>    compare actually checks for "contains" - RFE for future: any chance you could either change the name
> or add a comment?
>
Comment added.

> 3. couple of questions on promote_virtual_memory_records
>     1. what happens with a thread stack commit for non-guard pages?
except stack guard pages, we don't commit stack pages, os does without 
going through our os::commit(). We really don't know how much memory is 
committed for stack, but we mark it as committed (we discussed a long 
time ago).
>     2. does this logic assume you have ordered the records by sequence number? If so, could
>     you please add a comment. I saw you appended them during the merge, but
>     I wasn't sure what order they were in originally.
merge phase append them in arbitrary order, but sorted into sequence 
order for promotion.
>     3. - if so, why would you get commit, uncommit, release, tag after a release?
>
>     4. For the multiple reserve case, wasn't that already handled in the first "if" or did I
>      read this incorrectly?
>
After keeping all virtual memory records to promotion phase, 3 & 4 
should not happen (I need to run more tests). I will file RFE to clean 
these up.

Thanks,

-Zhengyu


> thanks,
> Karen
>
> On Sep 11, 2012, at 2:23 PM, Zhengyu Gu wrote:
>
>> This is a pretty significant changes on how NMT deal with virtual memory records. The old logic processed virtual memory records based on memory address order, then sequence number, which was fault, since virtual memory operations do not have to base on the ranges' base addresses. The corrected logic is to process virtual memory records purely based on the sequence number.
>>
>> CR: http://bugs.sun.com/view_bug.do?bug_id=7181995
>> Webrev: http://cr.openjdk.java.net/~zgu/7181995/webrev.00/
>>
>> The changes passed following tests:
>>   - JPRT tests
>>   - vm quick tests
>>   - Parallel classloading tests
>>   - nsk stress tests
>>   - jck60 stress tests
>>   - quick jdi tests
>>   - quick jvmti tests
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>