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

[Stefan Karlsson]

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