RFR: 8146523: Incorrect memory tracker used by os::unmap_memory()

[Ioi Lam]

For clarity, I think we should change the comment to this:

   + #ifndef _WINDOWS
   +     Tracker tkr = MemTracker::get_virtual_memory_uncommit_tracker();
   + #else
   +     // os::unmap_memory is only used for CDS. On Windows:
   +     //   + pd_unmap_memory also releases the memory;
   +     //   + there's no corresponding os::release_memory call for addr
   +     //     (see MetaspaceShared::map_shared_spaces);
   +     // so we must use get_virtual_memory_release_tracker here.
          Tracker tkr = MemTracker::get_virtual_memory_release_tracker();
   + #endif

Thanks
- Ioi


On 1/7/16 3:30 PM, Ioi Lam wrote:
> Oops, please ignore most of what I said here. I just realized that 
> os::map_memory/unmap_memory is called only by CDS.
>
> - Ioi
>
>
> On 1/7/16 3:12 PM, Ioi Lam wrote:
>> On 1/7/16 1:30 PM, Jiangli Zhou wrote:
>>>
>>>>>
>>>>> On 1/7/16 2:34 PM, Ioi Lam wrote:
>>>>>> Hi Jiangli,
>>>>>>
>>>>>> I looked at the implementatiion of os::pd_unmap_memory on windows 
>>>>>> but I can't figure out how it would affect the memory tracker:
>>>>>>
>>>>>> os.cpp:
>>>>>>
>>>>>>      bool os::unmap_memory(char *addr, size_t bytes) {
>>>>>>        bool result;
>>>>>>        if (MemTracker::tracking_level() > NMT_minimal) {
>>>>>>   + #ifndef _WINDOWS
>>>>>>   +     Tracker tkr = 
>>>>>> MemTracker::get_virtual_memory_uncommit_tracker();
>>>>>>   + #else
>>>>>>   +     // Windows pd_unmap_memory also does memory release, so use
>>>>>>   +     // the virtual_memory_release_tracker for windows
>>>>>>          Tracker tkr = 
>>>>>> MemTracker::get_virtual_memory_release_tracker();
>>>>>>   + #endif
>>>>>>
>>> Could you please explain why the comment is wrong? On linux/unix 
>>> platforms, we have extra code to do explicit memory ‘reserve’ and 
>>> ‘release’, which calls NMT to track the reserve and release of the 
>>> related memory region. Mapping and unmaping are performed on the 
>>> memory regions that are already ‘reserved’ on those platform.
>>> On windows, there are no explicit calls to do memory ‘reserve’ 
>>> before mapping and ‘release’ after unmapping.
>> This is not true. There's platform-independent code in virtualspace.cpp:
>>
>>     ReservedSpace::initialize -> os::reserve_memory -> 
>> os::pd_reserve_memory.
>>
>> There's no #ifdef WINDOWS on this path and os::pd_reserve_memory() is 
>> actually implemented in os_windows.cpp
>>
>> What you said may be true for CDS:
>>
>>    bool MetaspaceShared::map_shared_spaces(FileMapInfo* mapinfo) {
>>    #ifndef _WINDOWS
>>       // Map in the shared memory and then map the regions on top of it.
>>       // On Windows, don't map the memory here because it will cause the
>>       // mappings of the regions to fail.
>>       ReservedSpace shared_rs = mapinfo->reserve_shared_memory();
>>
>> but that's not true in general. There are plenty of ReservedSpace 
>> used in platform-independent GC code.
>>> So the memory mapping/unmapping APIs have the implicit 
>>> ‘reserve’/‘release’ behaviors. That’s why for windows the 
>>> release_tracker is used to reflect the behavior of unmapping with 
>>> implicit ‘release’, and the linux/unix only needs the uncommit_tracker.
>> Also the comment implies that pd_unmap_memory always calls 
>> pd_release_memory, but this is not true:
>>
>>    bool os::pd_unmap_memory(char* addr, size_t bytes) {
>>       ...
>>       if (mem_info.Protect == PAGE_EXECUTE_READ ||
>>           mem_info.Protect == PAGE_EXECUTE_READWRITE) {
>>         return pd_release_memory(addr, bytes);
>>       }
>>       ...
>>    }
>>
>> Even on Windows, won't we eventually call os::release_memory(), which 
>> would properly record the removal using 
>> get_virtual_memory_release_tracker? Yes, CDS is an exception, but 
>> that should be dealt with in the CDS code, not in the generic os.cpp 
>> code. And, I am not particularly worried about the CDS case, since it 
>> happens only when mapping of some of the CDS regions has failed.
>>
>> Are you sure we need the special case for Windows here? Have you 
>> tested NMT on Windows without the #ifdef?
>>
>> The NMT code is quite messy. I don't want to make it worse by adding 
>> more #ifdefs with comments that are only partially true.
>>
>> Thanks
>> - Ioi
>>
>