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

Per Liden per.liden at oracle.com
Wed Jun 24 10:01:14 UTC 2020


Hi Stefan,

On 6/24/20 11:54 AM, Stefan Karlsson wrote:
> 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.

Nice! Looks good!

cheers,
Per

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


More information about the hotspot-gc-dev mailing list