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

[Per Liden]

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