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

[Calvin Cheung]

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