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

[Stefan Karlsson]

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