RFR: NMT: Don't record uncommits unconditionally

[Zhengyu Gu]

On 01/11/2018 05:28 AM, Stefan Karlsson wrote:
> If we decide that the below change is valid and an improvement, then I 
> have a patch for this:
> 
> http://cr.openjdk.java.net/~stefank/8194926/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8194926
> 8194926: NMT: Clean up usage of get_record_virtual_memory_uncommit/release

Thanks for the cleanup. Looks good to me.

-Zhengyu

> 
> While running the NMT jtreg tests with ZGC, I found that one of the 
> tests used ints to parse size_t values. Here's and upstream patch for that.
> 
> http://cr.openjdk.java.net/~stefank/8194925/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8194925
> 8194925: NMT: SummarySanityCheck test can't parse values > max_jint
> 
> Thanks,
> StefanK
> 
> On 2018-01-11 10:09, Stefan Karlsson wrote:
>> [Looping in Zhengyu]
>>
>> Inlined:
>>
>> On 2018-01-11 07:56, Per Liden wrote:
>>> Hi,
>>>
>>> On 01/10/2018 05:01 PM, Stefan Karlsson wrote:
>>>> Hi all,
>>>>
>>>> Please review this patch to fix a bug in the recent NMT 
>>>> implementation for ZGC.
>>>>
>>>> http://cr.openjdk.java.net/~stefank/zgc/zNMTBugFix/webrev.01
>>>
>>> Change looks good, but the memTracker.cpp part should probably be 
>>> split out into an upstream patch. Also, it feels like we should 
>>> adjust the other user of this in os.cpp to use the new function.
>>
>>
>> Looking at the os.cpp function:
>>
>> bool os::uncommit_memory(char* addr, size_t bytes) {
>>    bool res;
>>    if (MemTracker::tracking_level() > NMT_minimal) {
>>      Tracker tkr = MemTracker::get_virtual_memory_uncommit_tracker();
>>      res = pd_uncommit_memory(addr, bytes);
>>      if (res) {
>>        tkr.record((address)addr, bytes);
>>      }
>>    } else {
>>      res = pd_uncommit_memory(addr, bytes);
>>    }
>>    return res;
>> }
>>
>> Is there a reason why the Tracker object is created before the 
>> pd_uncommit_memory, while the record() and ~Tracker is called after?
>>
>> Could we change the code above to:
>>
>> bool os::uncommit_memory(char* addr, size_t bytes) {
>>    bool res = pd_uncommit_memory(addr, bytes);
>>    if (res && MemTracker::tracking_level() > NMT_minimal) {
>>      Tracker tkr = MemTracker::get_virtual_memory_uncommit_tracker();
>>      tkr.record((address)addr, bytes);
>>    }
>>    return res;
>> }
>>
>> or even:
>>
>> bool os::uncommit_memory(char* addr, size_t bytes) {
>>    bool res = pd_uncommit_memory(addr, bytes);
>>    if (res) {
>>      MemTracker::record_virtual_memory_uncommit(addr, bytes)
>>    }
>>    return res;
>> }
>>
>> Then these functions would look very similar to the commit functions:
>>
>> bool os::commit_memory(char* addr, size_t size, size_t alignment_hint,
>>                                bool executable) {
>>    bool res = os::pd_commit_memory(addr, size, alignment_hint, 
>> executable);
>>    if (res) {
>>      MemTracker::record_virtual_memory_commit((address)addr, size, 
>> CALLER_PC);
>>    }
>>    return res;
>> }
>>
>>
>> I see that Tracker() and ~Tracker() locks and unlocks ThreadCritical. 
>> Do we need to be in "thread critical" while unmapping memory?
>>
>> If we can do the change above, then I think we can get rid of 
>> get_virtual_memory_uncommit_tracker and 
>> get_virtual_memory_release_tracker.
>>
>> Thanks,
>> StefanK
>>
>>>
>>> cheers,
>>> Per
>>>
>>>>
>>>> The uncommit path was executed even when NMT was turned off. Because 
>>>> of this we hit the following assertion when pages were detached:
>>>>
>>>> #  Internal Error (.../services/memTracker.hpp:231), pid=12751, 
>>>> tid=2738
>>>> #  assert(tracking_level() >= NMT_summary) failed: Check by caller
>>>>
>>>> Thanks,
>>>> StefanK