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

[Zhengyu Gu]

On 5/31/19 12:37 AM, Calvin Cheung wrote:
> 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/

Yes. Please change Line #4912 to 
MemTracker::record_virtual_memory_reserve_and_commit((address)addr, 
bytes, CALLER_PC);

I made copy-past error in early reply.

> 
> 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.
Right, this is known problem:
https://bugs.openjdk.java.net/browse/JDK-8198226

Thanks,

-Zhengyu

> 
> 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