RFR: NMT: Don't record uncommits unconditionally

[Stefan Karlsson]

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

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