RFR: 8141501: Problems with BitMap buffer management

[Per Liden]

Hi Stefan,

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

This review only covers the GC and BitMap parts.

vmStructs_cms.hpp
-----------------
   36   nonstatic_field(CMSBitMap,                   _bm, 
               BitMap)                                \


This should be BitMapView instead of BitMap.


g1RegionToSpaceMapper.hpp
-------------------------
  50   size_t _region_granularity;

Pre-existing as far as I can tell, but this is unused and can be removed.

   33 G1RegionToSpaceMapper::G1RegionToSpaceMapper(ReservedSpace rs,
   34                                              size_t used_size,
   35                                              size_t page_size,
   36                                              size_t 
region_granularity,
   37                                              size_t commit_factor,
   38                                              MemoryType type) :

Also pre-existing, but I'd suggest we use the name region_granularity or 
alloc_granularity. Currently we use both names in different contexts to 
refer to the same thing.

bitMap.cpp
----------

   84 typedef BitMapAllocator<ArrayAllocator<BitMap::bm_word_t, 
mtInternal> > CHeapAllocator;
   85 typedef BitMapAllocator<ResourceArrayAllocator> 
ResourceAllocator;

How about calling these CHeapBitMapAllocator/ResourceBitMapAllocator?

  111 ArenaBitMap::ArenaBitMap(Arena* arena, idx_t size_in_bits)
  112     : 
BitMap((bm_word_t*)arena->Amalloc(calc_size_in_bytes(size_in_bits)), 
size_in_bits) {
  113 }

Could we break this out into a ArenaBitMapAllocator, to align to the 
model used for Resource/CHeap allocators?

cheers,
Per

> 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