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

[Zhengyu Gu]

Looks good.

-Zhengyu

On 5/31/19 3:36 PM, Calvin Cheung wrote:
> 
> 
> On 5/31/19, 4:54 AM, Zhengyu Gu wrote:
>>
>>
>> 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.
> Just for the record, updated webrev: 
> http://cr.openjdk.java.net/~ccheung/8223336/webrev.03/
> 
> I also ran all NMT tests via mach5 and they all passed.
>>
>>>
>>> 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 for the pointer to RFE.
> 
> thanks,
> Calvin
>>
>> 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