RFR: 8141501: Problems with BitMap buffer management

[Per Liden]

Hi Stefan,

On 2016-04-28 15:23, Stefan Karlsson wrote:
> 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

Looks good. I like the new allocators model.

cheers,
Per

>
> 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
>