RFR: 8146523: VirtualMemoryTracker::remove_released_region double count unmapped CDS shared memory

[Jiangli Zhou]

Thanks, Coleen.

Jiangli

> On Jan 12, 2016, at 12:17 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> 
> Yes, this looks safe.  There's precedent for special code in NMT for CDS.
> 
> Coleen
> 
> 
> On 1/12/16 1:28 PM, Jiangli Zhou wrote:
>> After more discussion with Coleen and Ioi, I decided to take a different approach to address the issue. Since the double counting release memory only occurs to unmapped CDS shared memory, fixing the issue for CDS only case is a safer approach.
>> 
>> Here is a new webrev that skips the release tracking for the unmapped CDS regions in VirtualMemoryTracker::remove_released_region(). The CDS shared memory is only tracked once by NMT as released when the memory is released as a whole after unmapping individual shared spaces.
>> 
>> http://cr.openjdk.java.net/~jiangli/8146523/webrev.02/
>> 
>> Thanks,
>> Jiangli
>> 
>>> On Jan 7, 2016, at 3:42 PM, Jiangli Zhou <jiangli.zhou at oracle.com> wrote:
>>> 
>>> Hi Ioi,
>>> 
>>> Got your email while typing my reply. :) I saw your email regarding the comment change as well. Will reflect that in my fix before pushing.
>>> 
>>> Just for the benefit of general public, here are some more details:
>>> 
>>> The reserve/release memory operations do not have platform specific behavior from NMT point of view. The reserve-map/unmap-release operations are handled differently for windows and linux/unix. The usage of reserve-map/unmap-release is only in CDS code currently, which has conditional code that does reserve and release for shared memory on non-windows platform. The fix does not change the original NMT behavior for windows platform.
>>> 
>>> Thanks!
>>> Jiangli
>>> 
>>>> On Jan 7, 2016, at 3:30 PM, Ioi Lam <ioi.lam at oracle.com> 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
>>>>> 
>