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

[coleen.phillimore at oracle.com]

Hi, This looks really good.

http://cr.openjdk.java.net/~stefank/8196405/webrev.01/src/hotspot/share/services/virtualMemoryTracker.cpp.udiff.html

+ // It would have made sense to use rgn->equals(...), but equals 
returns true for overlapping regions.


Does anything call the Region<>::equals functions?  I stumbled over this 
myself a while ago.  Can they be removed with this patch?

Thank you for fixing this.   The gtest looks great.   There was another 
less comprehensive test in the NMT directory for this that hopefully you 
ran also with the runtime NMT tests.

Thanks,
Coleen

On 2/15/18 9:05 AM, 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