RFR: NMT: Don't record uncommits unconditionally

[Zhengyu Gu]

Hi Stefan,

> 
> 
> 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?
> 
I can not recall why tracker is created before the pd_uncommit_memory, 
but I don't think it has to.

> 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;
> }

This version looks correct.

> 
> 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;
> }
> 
Humm ... it does not seem to have 
MemTracker::record_virtual_memory_uncommit(addr, bytes).

Tracker thingy seems to be bad, it locks and unlocks ThreadCritical twice.

If you want to file a RFE, I am glad to clean it up.

Thanks,

-Zhengyu


> 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