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

[Ioi Lam]

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