RFR: 8196405: [REDO] NMT: add_committed_regions doesn't merge succeeding regions

[Erik Österlund]

Hi Stefank,

It is much clearer when and why merging happens now, which makes it 
easier to read and understand the code.
Looks good.

Thanks,
/Erik

On 2018-02-19 12:26, Stefan Karlsson wrote:
> Hi all,
>
> I've an updated version of this patch:
>  http://cr.openjdk.java.net/~stefank/8196405/webrev.02.delta
>  http://cr.openjdk.java.net/~stefank/8196405/webrev.02
>
> Erik Ö reviewed this offline and suggested that we simplified the code 
> by removing the second loop, and setting up prev to point to the first 
> fully preceding region, and also record prev's next region.
>
> We would then have to make sure we could deal with these situations:
>
> Merge with prev:
>   [prev][new]
>   [prev][new]  [next]
>
> Merge with prev and next:
>   [prev][new][next]
>
> Mege with next:
> [prev]  [new][next]
>         [new][next]
>
> No Merge:
> [prev]  [new]  [next]
> [prev]  [new]
>         [new]  [next]
>         [new]
>
> And this translates to the following code in the patch:
>
>   // Try to merge with prev and possibly next.
>   if (try_merge_with(prev, addr, size, stack)) {
>     if (try_merge_with(prev, next)) {
>       // prev was expanded to contain the new region
>       // and next, need to remove next from the list
>       _committed_regions.remove_after(prev);
>     }
>
>     return true;
>   }
>
>   // Didn't merge with prev, try with next.
>   if (try_merge_with(next, addr, size, stack)) {
>     return true;
>   }
>
>   // Couldn't merge with any regions - create a new region.
>   return add_committed_region(CommittedMemoryRegion(addr, size, stack));
>
> Thanks,
> StefanK
>
> On 2018-02-15 15:05, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to fix the merging of committed regions in 
>> NMT's VirtualMemoryTracker.
>>
>> http://cr.openjdk.java.net/~stefank/8196405/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8196405
>>
>> The previous attempt to fix this didn't deal with overlapping 
>> committed regions correctly. This patch solves that case by first 
>> removing regions, and parts of regions, that intersect with the newly 
>> committed region. After that, the new region can be added and/or 
>> merged with any adjacent regions.
>>
>> There used to be a special-case for code paths that both reserve and 
>> commit memory at the same time. ReservedMemoryRegion had an 
>> all_committed boolean to track that state, and subsequent commits in 
>> the reserved region were ignored. There were a number of bugs cause 
>> by this special-handeling, when regions were committed or uncommitted 
>> inside an all_committed region. This patch removes the all_committed 
>> property and instead queries the _committed_regions list where 
>> necessary.
>>
>> The patch removes special case handling for mtThreadStacks. The 
>> intention is that the add_committed_region and 
>> remove_uncomitted_region functions should be correct, so that we 
>> don't need that kind of special-case handling.
>>
>> The overlap_region function uses 'sz - 1', which would be broken if 
>> sz ever was 0. I added asserts to check for this.
>>
>> I've added a small jtreg test for the initial problem that this bug 
>> report was intending to fix. But I realized that the WhiteBox API 
>> didn't support setting up and testing different call stacks for the 
>> memory reservations and commits, so I introduced a C++ gtest. This 
>> test is both faster to execute, easier to debug, and more flexible, 
>> so I hope this is appreciated. :)
>>
>> I've tested this with the added unit tests, short run with 
>> Kitchensink, and mach5 hs-tier1,hs-tier2. Anything else that I should 
>> be running?
>>
>> While working on this I found two other bugs:
>>
>>   https://bugs.openjdk.java.net/browse/JDK-8198225
>>   os::attempt_reserve_memory_at records memory as committed
>>
>>   https://bugs.openjdk.java.net/browse/JDK-8198226
>>   os::attempt_reserve_memory_at records reserved memory twice on windows
>>
>> Thanks,
>> StefanK