RFR: 8141501: Problems with BitMap buffer management

[Stefan Karlsson]

Hi all,

I decided to restructure the allocation code a bit to get better 
separation between the bitmap allocation logic and the actual bitmap 
memory allocators.

http://cr.openjdk.java.net/~stefank/8141501/webrev.02.delta
http://cr.openjdk.java.net/~stefank/8141501/webrev.02

BitMap has now the following allocation functions: reallocate, allocate, 
and free. These functions take care of the high-level handling of the 
BitMap allocations: requesting, copying, clearing, and releasing memory. 
The low-level allocation and freeing of memory are handled by the 
ResourceBitMapAllocator, CHeapBitMapAllocator, and ArenaBitMapAllocator 
classes.

I've also moved the implementation of resize, initialize, and 
reinitialize into protected functions in BitMap. The sub-classes then 
expose public version of these functions, and pass down the appropriate 
allocator instances to the protected BitMap versions.

The current patch does not provide resize, initialize and reinitialize 
functions for the ArenaBitMaps, since that requires an extra field in 
all ArenaBitMaps. If we decide that that's not a real problem, we could 
easily introduce that as a separate patch. See:

http://cr.openjdk.java.net/~stefank/8141501/webrev.02.resizableArenaBitMaps/

While implementing the above patches I realized that the ResourceBitMaps 
actually are cleared in the constructor, so the previous comments were 
wrong and I've updated the comments accordingly. But this also means 
that the ResourceBitMap map(size); map.clear(); pattern, which can be 
seen mainly in the compiler code, clears the bitmaps twice. I've created 
the following CR to track that:

https://bugs.openjdk.java.net/browse/JDK-8155638 - Resource allocated 
BitMaps are often cleared twice

Thanks,
StefanK

On 2016-04-27 13:57, Stefan Karlsson wrote:
> Hi all,
>
> Please review this patch to change how the backing storage of BitMaps 
> are managed.
>
> http://cr.openjdk.java.net/~stefank/8141501/webrev.01
> https://bugs.openjdk.java.net/browse/JDK-8141501
>
> The patch changes BitMap into an abstract base class, with concrete 
> sub-classes that manages the underlying bitmap backing storage.
>
> The proposed BitMap classes are:
>
> - BitMap - the abstract base class
> - ResourceBitMap - bitmap with resource area allocated backing storage
> - ArenaBitMap - bitmap with arena allocated backing storage
> - CHeapBitMap - bitmap with CHeap allocated backing storage
> - BitMapView - bitmap without the ownership of the backing storage
>
>  This will hopefully make it less likely to use the BitMaps 
> incorrectly. Previously, it was possible to write the following broken 
> code:
>
> // CHeap allocate.
> BitMap map(BITMAP_SIZE / 2, false);
>
> // Resource allocate.
> // The CHeap memory is leaked.
> map.resize(BITMAP_SIZE);
>
> and:
>
> // Resource allocate.
> BitMap map(BITMAP_SIZE / 2);
>
> // CHeap allocate.
> // CHeap freeing Resource allocated memory => memory stomping
> map.resize(BITMAP_SIZE, false);
>
> The stricter typing of the new BitMap sub-classes prevents these 
> classes of bugs.
>
> Further motivation for this patch can be found in:
> https://bugs.openjdk.java.net/browse/JDK-8141501
>
> Tested with JPRT and ExecuteInternalVMTests.
>
> Thanks Kim for providing offline feedback on different revisions of 
> this patch.
>
> This changes code in mostly the GC and Compiler parts of the JVM, so 
> it would be good to get reviews from those groups.
>
> Thanks,
> StefanK