RFR: 8296139: Make GrowableBitMap the base class of all implementations [v2]

[Stefan Karlsson]

On Thu, 3 Nov 2022 20:23:01 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> BitMap is an abstract class. All subclasses provide their own allocators. It is the allocator brings the capability to 'resize' the container. 
>> I would like to add a new 'GrowableBitmap' and make it the common base class. 
>> 
>> To substitute 'VectorSet', we also need to merge ResourceBitMap and ArenaBitMap because VectorSet supports Resource and Arena storage at the same time. 
>> This patch unifies them.  ResourceBitMap is a specialized ArenaBitMap whose allocator is with NULL arena.
>> 
>> All subclasses of GrowableBitMap are elastic. I will add new interface like VectorSet::test_set(index). It will grow to include the new index.
>
> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Refactor GrowableBitMap using C++ CRTP
>   
>   With CPTP, we don't need to have explicit Allocators. Subclasses expose
>   allocate/free member functions upward to GrowableBitMap<BitMapWithAllocator>. This
>   aproach is similar to GrowableArray.

I started to review this PR and added comments, but I think it will be easier if you take a look at this branch which contains all my proposed changes + some small cleanups:
https://github.com/stefank/jdk/tree/pr_10941

src/hotspot/share/utilities/bitMap.cpp line 91:

> 89:   // Reuse reallocate to ensure that the new memory is cleared.
> 90:   bm_word_t* map = reallocate(nullptr, 0, size_in_bits, clear);
> 91:   update(map, size_in_bits);

Both CHeapBitMap and AreanBitMap perform this "initialization". I propose that this instead call the GrowableBitMap::initialize function.

src/hotspot/share/utilities/bitMap.hpp line 170:

> 168: 
> 169:   // Protected constructor and destructor.
> 170:   BitMap(bm_word_t* map, idx_t size_in_bits) : _map(map), _size(size_in_bits) {}

Is there a reason why the verify_size was removed?

src/hotspot/share/utilities/bitMap.hpp line 318:

> 316: };
> 317: 
> 318: // CRTP: BitmapWithAllocator exposes to the following Allocator interfaces upward to GrowableBitMap.

Maybe 'to the'=> 'the'?

src/hotspot/share/utilities/bitMap.hpp line 330:

> 328:   GrowableBitMap(idx_t size_in_bits, bool clear) : BitMap(reallocate(nullptr, 0, size_in_bits, clear), size_in_bits) {
> 329:   }
> 330:   GrowableBitMap() : BitMap(nullptr, 0) {}

GrowableBitMap(idx_t, bool) is dangerous to use because it calls reallocate, which calls the allocate function of the subclass. This works for ResourceBitMap, but not if you tried to use it for CHeapBitMap or ArenaBitMap. I'd suggest that we never let GrowableBitMap allocate memory in the constructor and instead push that responsiblity down into the subclasses.

src/hotspot/share/utilities/bitMap.hpp line 394:

> 392: 
> 393:  public:
> 394:   ResourceBitMap() : ResourceBitMap(0) {}

Should this be declared explicit?

src/hotspot/share/utilities/bitMap.hpp line 399:

> 397:   bm_word_t* allocate(idx_t size_in_words) const {
> 398:     return (bm_word_t*)NEW_RESOURCE_ARRAY(bm_word_t, size_in_words);
> 399:   }

This puts non-trivial code in the header. Could you move this out to the .cpp file?

src/hotspot/share/utilities/bitMap.hpp line 426:

> 424:   void free(bm_word_t* map, idx_t size_in_words) const {
> 425:     ArrayAllocator<bm_word_t>::free(map, size_in_words);
> 426:   }

Non-trivial code in header file. Please move to .cpp file.

-------------

Changes requested by stefank (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10941