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

[Ioi Lam]

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
>