RFR: 8296139: Make GrowableBitMap the base class of all implementations

[Axel Boldt-Christmas]

On Wed, 2 Nov 2022 19:08:15 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> This is how I treat them using the base pointer 'ArenaBitMap*'
>> https://github.com/navyxliu/jdk/commit/cdbc0e1c97f747fd53ad408d59e65ec8adf0fc16#diff-17a9d48a5570e5bcde557c8d1efcbe78d69a5657bc865bed37b22c3c8b5d447d
>> 
>>> The places where VectorSet() is used today, can we not just use ArenaBitMap(Thread::current()->resource_area(), [...]) instead. And have the replacement be VectorSet -> ArenaBitMap
>> 
>> `Thread::current()->resource_area()` returns ResourceArena*. ResourceArena is a subclass of Arena. In this sense, we can use the same ArenaAllocator to handle both ResourceBitMap and ArenaBitMap. That's why I remove 'ResourceAllocator' in this patch. 
>> 
>> I think it is possible to do what you said. Let's say we use ArenaBitMap as 'ResourceBitMap'. Shall we keep ArenaBitMap and ResourceBitMap separated then? In classic object-oriented design, ResourceBitMap 'is-a' ArenaBitMap, isn't it? If we keep 2 classes in parallel, we may end up 2 'ResourceBitMap' with different behaviors. 
>> 
>> In current patch, ResourceBitMap is just an adapter. It supports copy constructor and copy assignment because 'resource arena' is unique.
>
> Correction: s/resource arena/resource area
> 
> This is my understanding. 'Resource area' in HotSpot is a special 'area' whose lifecycle is bound to stacktrace. By default, we still use 'arena' allocation algorithm. That's why I think we can treat ResourceBitMap as a subclass of ArenaBitMap.

I like the type hierarchy but for one thing. Today we have two ways of creating a resource arena
* `ResourceBitmap([...])`
  * Which checks `verify_has_resource_mark` in debug builds
* `ArenaBitmap(some_resource_area, [...])`
  * Which does not check `verify_has_resource_mark`

This would add a third way where `ArenaBitmap(nullptr, [...])` has the same behaviour as `ResourceBitmap`. This can be solved with a new `ArenaBitMap` constructor.
```c++
 protected:
  ArenaBitMap(std::nullptr_t, idx_t size_in_bits, bool clean);

And add a debug assert in the normal ArenaBitMap constructor `precond(arena != nullptr);`

Which would give a behaviour like this
```c++
#ifdef ASSERT
TEST_VM(BitMap, resource_work) {
  ResourceBitMap rbm(0);
  ArenaBitMap abm(Thread::current()->resource_area(),0);
  static_cast<void>(rbm);
  static_cast<void>(abm);
}
TEST_VM_ASSERT(BitMap, nullptr_arena) {
  Arena* ptr = nullptr;
  ArenaBitMap crash(ptr,0);
  // ArenaBitMap does_not_compile(nullptr,0);
  // ArenaBitMap does_not_compile(NULL,0);
  static_cast<void>(crash);
}
#endif


I do not know if the extra branch in the arena allocator ever matters, but if it did you can just store the resource area arena in `_arena` on release and not branch. The only semantical difference then would be that if the [re]allocating functions gets called from different threads. But I would think that a resource allocated bitmap crossing thread boundaries is never an appropriate use. 

But I did preface this discussion with that I do not know how we treat resource allocated objects that just look like arena allocated objects and thus circumvent the `verify_has_resource_mark` check. It happens all over the place in the compiler code, and I am not sure why.

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

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