RFR: 8248132: ZGC: Unify handling of all OopStorage instances in root processing

[Stefan Karlsson]

Hi Per,

Good point about the strong_count being statically known. I wasn't 
entirely happy about the type fiddling in the proposed change below, so 
I experimented with alternative implementations. The proposal I current 
have is this:
https://cr.openjdk.java.net/~stefank/8248132/webrev.07/

The patch adds a few utility classes:
- ValueObjBlock stamps out a number of instances:
- ValueObjArray provides an array over those instances

With this we can now stamp out the OopStorage::ParState instances into 
OopStorageSetParState without dynamic allocation and without type casting.

A version without the gtest and with the out-of-bounds check was tested 
in tier1-3.

Thanks,
StefanK

On 2020-06-23 16:29, Per Liden wrote:
> Hi,
>
> On 6/23/20 4:09 PM, Kim Barrett wrote:
>>> On Jun 23, 2020, at 9:09 AM, Per Liden <per.liden at oracle.com> wrote:
>>>
>>> Hi Stefan,
>>>
>>> On 6/23/20 10:10 AM, Stefan Karlsson wrote:
>>>> Hi all,
>>>> Please review this patch to unify handling of all OopStorage 
>>>> instances in root processing.
>>>> https://cr.openjdk.java.net/~stefank/8248132/webrev.01/
>>>
>>> I note that the size of the dynamic allocation is always known at 
>>> compile-time. So how about we just avoid it altogether with 
>>> something like this?
>>>
>>> diff --git a/src/hotspot/share/gc/shared/oopStorageSetParState.hpp 
>>> b/src/hotspot/share/gc/shared/oopStorageSetParState.hpp
>>> --- a/src/hotspot/share/gc/shared/oopStorageSetParState.hpp
>>> +++ b/src/hotspot/share/gc/shared/oopStorageSetParState.hpp
>>> @@ -26,16 +26,16 @@
>>> #define SHARE_GC_SHARED_OOPSTORAGESETPARSTATE_HPP
>>>
>>> #include "gc/shared/oopStorageParState.hpp"
>>> +#include "gc/shared/oopStorageSet.hpp"
>>>
>>> template <bool concurrent, bool is_const>
>>> class OopStorageSetStrongParState {
>>> private:
>>>    typedef OopStorage::ParState<concurrent, is_const> ParStateType;
>>>
>>> -  ParStateType* _par_states;
>>> +  char _par_states[sizeof(ParStateType) * 
>>> OopStorageSet::strong_count];
>>
>> (Not a review, just a drive-by comment.)
>>
>> This doesn't guarantee proper alignment of _par_states; with
>> _par_states being the only member, it only requires char alignment.
>> (It might happen to work because of vagaries of heap allocators and
>> stack alignment requirements, possibly always on some platforms, but
>> that's not guaranteed.)
>>
>> -  ParStateType* _par_states;
>> +  char _par_states[sizeof(ParStateType) * OopStorageSet::strong_count];
>
> You're right, but I'm thinking ATTRIBUTE_ALIGNED should work here.
>
> cheers,
> Per
>
>
>>
>> Doing that correctly is a bit annoying, which is why C++11 added
>> std::aligned_storage.
>>