RFR: 8230566: ZGC: Don't substitute klass pointer during array clearing

[Per Liden]

Thanks Stefan!

/Per

On 9/10/19 9:20 AM, Stefan Karlsson wrote:
> Looks good.
> 
> StefanK
> 
> On 2019-09-10 09:16, Per Liden wrote:
>> Thanks for reviewing. Updated webrev:
>>
>> http://cr.openjdk.java.net/~pliden/8230566/webrev.1-diff
>> http://cr.openjdk.java.net/~pliden/8230566/webrev.1
>>
>> /Per
>>
>> On 9/6/19 3:51 PM, Stefan Karlsson wrote:
>>> Talked to Per and Erik offline about follow_array_object. We should 
>>> be able to skip following the klass pointer when follow is false. 
>>> Just like objects in "allocating" regions don't get their klass 
>>> pointer followed. Other than that, this looks good to me.
>>>
>>> Thanks,
>>> StefanK
>>>
>>>
>>>
>>> On 2019-09-04 19:15, Per Liden wrote:
>>>> Hi Erik,
>>>>
>>>> On 9/4/19 5:12 PM, Erik Österlund wrote:
>>>>> Hi Per,
>>>>>
>>>>> In  src/hotspot/share/gc/z/zHeapIterator.cpp the only change made 
>>>>> here is to mess up the indentation:
>>>>>
>>>>>   195   // Push roots to visit
>>>>>   196   push_roots<ZRootsIterator,                    false /* 
>>>>> Concurrent */, false /* Weak */>();
>>>>>   197   push_roots<ZConcurrentRootsIteratorClaimOther, true  /* 
>>>>> Concurrent */, false /* Weak */>();
>>>>>
>>>>> Should probably change that back.
>>>>
>>>> Will fix!
>>>>
>>>>>
>>>>> One possible solution to the problem of passing around all this 
>>>>> context information in the templates,
>>>>> is to wrap them in a template config type, like this:
>>>>>
>>>>> template <boolfollow, bool finalizable, bool publish>
>>>>> struct ZMarkConfig {
>>>>>    static const bool _follow = follow;
>>>>>    static const bool _finalizable = finalizable;
>>>>>    static const bool _publish  = publish;
>>>>> };
>>>>>
>>>>> Then you can just pass around the ZMarkConfig in the templates all 
>>>>> over the place, instead of the 3 (at this
>>>>> moment) bools. That way, adding and removing bools becomes easier 
>>>>> in the future; no need to change the passing
>>>>> around code to pass more things around. Only the producer and 
>>>>> consumer of the flags need changing.
>>>>>
>>>>> However, I'd propose we do that in a follow-up RFE so we can fix 
>>>>> the bug faster. Having said that, looks good,
>>>>> and don't need a new webrev.
>>>>
>>>> I'll prototype and see how it turns out. If it seems to be worth 
>>>> doing I'll file a separate RFE.
>>>>
>>>> Thanks for reviewing!
>>>>
>>>> cheers,
>>>> Per
>>>>
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>>>
>>>>> On 2019-09-04 14:02, Per Liden wrote:
>>>>>> When allocating large object arrays, ZObjArrayAllocator 
>>>>>> instantiates a non-cleared array of longs and later installs the 
>>>>>> correct klass pointer once the array has been cleared. While this 
>>>>>> might work it's also error prone. For example, there are at least 
>>>>>> two asserts() that will re-loaded the klass pointer and fail. It's 
>>>>>> also easy for someone in the future to make the innocent mistake 
>>>>>> of re-loading the klass pointer in some sensitive path, which 
>>>>>> would lead to new problems. We can avoid all these problems by not 
>>>>>> substituting with the klass pointer, and instead have another 
>>>>>> mechanism to tell GC marker threads to not follow the elements in 
>>>>>> not-yet-cleared arrays.
>>>>>>
>>>>>> This patch adds a new bit to ZMarkStackEntry, to signal to marking 
>>>>>> threads that the object array should not be followed, just mark it 
>>>>>> as live and visit its klass.
>>>>>>
>>>>>> The patch is large-ish because of the need to propagate the 
>>>>>> "follow" flag down through a few of layers. We might want to think 
>>>>>> about converting the flags (follow/finalizable/publish) into a 
>>>>>> single ZBarrierFlags-thing (perhaps similar to DecoratorSet) to 
>>>>>> reduce the noise and make it easier to add/adjust/remove flags in 
>>>>>> the future. But such an enhancements could come later.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8230566
>>>>>> Webrev: http://cr.openjdk.java.net/~pliden/8230566/webrev.0
>>>>>>
>>>>>> Testing: tier1-6 on linux + ad hoc runs of tests that provoked the 
>>>>>> problem
>>>>>>
>>>>>> /Per
>>>>>