RFR(xs) 8223336: Assert in VirtualMemoryTracker::remove_released_region when running the SharedArchiveConsistency.java test with -XX:NativeMemoryTracking=detail

[Calvin Cheung]

Hi Zhengyu,

On 5/30/19, 5:18 PM, Zhengyu Gu wrote:
> Sorry. I think MemTracker::record_virtual_memory_reserve() should be 
> called before ReadFile() call, so it has a record for succeeded case.
My first version of webrev does what you're suggesting. (I can add the 
comment as you listed below.)
http://cr.openjdk.java.net/~ccheung/8223336/webrev.00/

The reason I moved the function call right before release_memory() is 
that the caller will call record_virtual_memory_reserve_and_commit() on 
a successful return. In that case, the 
VirtualMemoryTracker::add_reserved_region() will be called twice. After 
reading the code, add_reserved_region() will try to search the region 
from _reserved_regions. If the region exists, it won't be added. So I 
think it's fine to go with webrev.00.

thanks,
Calvin
>
> E.g.
>     if (base == NULL) {
>       log_info(os)("VirtualAlloc() failed: GetLastError->%ld.", 
> GetLastError());
>       CloseHandle(hFile);
>       return NULL;
>     }
>
>     // Record virtual memory allocation
>     MemTracker::record_virtual_memory_reserve((address)addr, bytes, 
> CALLER_PC);
>
>     DWORD bytes_read;
>     OVERLAPPED overlapped;
>     overlapped.Offset = (DWORD)file_offset;
>     overlapped.OffsetHigh = 0;
>     overlapped.hEvent = NULL;
>     // ReadFile guarantees that if the return value is true, the 
> requested
>     // number of bytes were read before returning.
>     bool res = ReadFile(hFile, base, (DWORD)bytes, &bytes_read, 
> &overlapped) != 0;
>     if (!res) {
>       log_info(os)("ReadFile() failed: GetLastError->%ld.", 
> GetLastError());
>       release_memory(base, bytes);
>       CloseHandle(hFile);
>       return NULL;
>     }
>
>
> -Zhengyu
>
>
> On 5/30/19 7:52 PM, Calvin Cheung wrote:
>> Hi Zhengyu,
>>
>> Thanks for your review.
>>
>> Here's an updated webrev with your suggested change:
>>      http://cr.openjdk.java.net/~ccheung/8223336/webrev.02/
>>
>> thanks,
>> Calvin
>>
>> On 5/30/19, 4:25 PM, Zhengyu Gu wrote:
>>> Hi Calvin,
>>>
>>> Thanks for fixing it.
>>>
>>> Since VirtualAlloc call has MEM_COMMIT flag set, I think it should 
>>> be tracked as committed memory, e.g.
>>>
>>> MemTracker::record_virtual_memory_reserve_and_commit((address)addr, 
>>> bytes, CALLER_PC);
>>>
>>> -Zhengyu
>>>
>>> On 5/30/19 4:04 PM, Calvin Cheung wrote:
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8223336
>>>>
>>>> webrev: http://cr.openjdk.java.net/~ccheung/8223336/webrev.00/
>>>>
>>>> A simple fix is adding the missing call to 
>>>> MemTracker::record_virtual_memory_reserve() in the Windows version 
>>>> of os::pd_map_memory().
>>>> On platforms other than Windows, the os::pd_map_memory() doesn't 
>>>> call release_memory() and the caller (os::map_memory()) calls 
>>>> MemTracker::record_virtual_memory_reserve() if the returned value 
>>>> from os::pd_map_memory() is non-NULL. That's why this bug 
>>>> reproduces only on Windows.
>>>>
>>>> Testing: ran SharedArchiveConsistency.java with 
>>>> -XX:NativeMemoryTracking=detail on Windows;
>>>>               ran all runtime/NMT tests on Windows.
>>>>
>>>> thanks,
>>>> Calvin