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

[Xin Liu]

On Thu, 3 Nov 2022 08:36:02 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

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

First of all, I revert back ResourceBitMap. It's now independent to ArenaBitMap. I think you concern is reasonable. In most cases, I expect users to use ResourceBitMap. ArenaBitmap does accept ResourceArea* as the underlying arena because Unique_Node_List uses it in this way. 

I don't think we need to handle ArenaBitMap::_arena == nullptr like you said. ResourceArea is still an Arena, but is managed by ResourceMark automatically. As a result, I introduce a boolean flag `Arena::_mark_managed`. ArenaBitMap doesn't mess with the underlying storage if Arena::mark_managed() is true. 

Here is my experimental patch.
https://github.com/navyxliu/jdk/commit/88063064a3bc0b8e65d3834f13e5ea8c6576eab0#diff-17a9d48a5570e5bcde557c8d1efcbe78d69a5657bc865bed37b22c3c8b5d447d

We have to support copy assignment for Unique_Node_List. It won't introduce memory leak because we only use ArenaBitMap in certain scenarios, see the assert. Besides ResourceArena, Unique_Node_List only accepts Compile::comp_arena() || Compiler::node_arena(). The lifecycle of them are same as Compile object. once a compilation unit is done, both two arenas are released completely.

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

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