RFR: 8141501: Problems with BitMap buffer management

[Stefan Karlsson]

Thanks, Per.

StefanK

On 28/04/16 16:51, Per Liden wrote:
> 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
>>